linux-kernel - Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20181008132945.GV3831@linux-l9pv.suse>
Date:   Mon, 8 Oct 2018 21:29:45 +0800
From:   joeyli <jlee@...e.com>
To:     Andy Lutomirski <luto@...nel.org>, Jann Horn <jannh@...gle.com>
Cc:     Jann Horn <jannh@...gle.com>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        keyrings@...r.kernel.org, David Howells <dhowells@...hat.com>,
        joeyli.kernel@...il.com, "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>,
        LKML <linux-kernel@...r.kernel.org>, linux-pm@...r.kernel.org,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Chen Yu <yu.c.chen@...el.com>, oneukum@...e.com,
        yu.chen.surf@...il.com, ggherdovich@...e.cz
Subject: Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

Hi Any, Jann,

On Wed, Oct 03, 2018 at 03:08:12PM -0700, Andy Lutomirski wrote:
> On Tue, Oct 2, 2018 at 12:36 PM Jann Horn <jannh@...gle.com> wrote:
> >
> > +Andy for opinions on things in write handlers
> > +Mimi Zohar as EVM maintainer
> >
> > On Tue, Oct 2, 2018 at 9:55 AM joeyli <jlee@...e.com> wrote:
> > > On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote:
> > > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@...il.com> wrote:
> > > > > This patch adds a snapshot keys handler for using the key retention
> > > > > service api to create keys for snapshot image encryption and
> > > > > authentication.
> > > [...snip]
> > > > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > > > +                             const char *buf, size_t n)
> > > > > +{
> > > > > +       int error = 0;
> > > > > +       char *p;
> > > > > +       int len;
> > > > > +
> > > > > +       if (!capable(CAP_SYS_ADMIN))
> > > > > +               return -EPERM;
> > > >
> > > > This is wrong, you can't use capable() in a write handler. You'd have
> > > > to use file_ns_capable(), and I think sysfs currently doesn't give you
> > > > a pointer to the struct file.
> > > > If you want to do this in a write handler, you'll have to either get
> > > > rid of this check or plumb through the cred struct pointer.
> > > > Alternatively, you could use some interface that doesn't go through a
> > > > write handler.
> > > >
> > >
> > > Thank you for point out this problem.
> > >
> > > Actually the evm_write_key() is the example for my code. The
> > > difference is that evm creates interface file on securityfs, but my
> > > implementation is on sysfs:
> > >
> > > security/integrity/evm/evm_secfs.c
> > >
> > > static ssize_t evm_write_key(struct file *file, const char __user *buf,
> > >                              size_t count, loff_t *ppos)
> > > {
> > >         int i, ret;
> > >
> > >         if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP))
> > >                 return -EPERM;
> 
> Yeah, that's a bug.
> 
> > > ...
> > >
> > > On the other hand, the writing handler of /sys/power/wake_lock also
> > > uses capable() to check the CAP_BLOCK_SUSPEND capability:
> > >
> > > kernel/power/main.c
> > > static ssize_t wake_lock_store(struct kobject *kobj,
> > >                                struct kobj_attribute *attr,
> > >                                const char *buf, size_t n)
> > > {
> > >         int error = pm_wake_lock(buf);
> > >         return error ? error : n;
> > > }
> > > power_attr(wake_lock);
> > >
> > > kernel/power/wakelock.c
> > > int pm_wake_lock(const char *buf)
> > > {
> > > ...
> > >         if (!capable(CAP_BLOCK_SUSPEND))
> > >                 return -EPERM;
> > > ...
> 
> Also a bug.
> 
> > >
> > >
> > > So I confused for when can capable() be used in sysfs interface? Is
> > > capable() only allowed in reading handler? Why the writing handler
> > > of securityfs can use capable()?
> >
> > They can't, they're all wrong. :P All of these capable() checks in
> > write handlers have to be assumed to be ineffective. But it's not a
> > big deal because you still need UID 0 to access these files.
> 
> Why are there capability checks at all?
> 
> >
> > > > > +static int user_key_init(void)
> > > > > +{
> > > > > +       struct user_key_payload *ukp;
> > > > > +       struct key *key;
> > > > > +       int err = 0;
> > > > > +
> > > > > +       pr_debug("%s\n", __func__);
> > > > > +
> > > > > +       /* find out swsusp-key */
> > > > > +       key = request_key(&key_type_user, skey.key_name, NULL);
> > > >
> > > > request_key() looks at current's keyring. That shouldn't happen in a
> > > > write handler.
> > > >
> > >
> > > The evm_write_key() also uses request_key() but it's on securityfs. Should
> > > I move my sysfs interface to securityfs?
> >
> > I don't think you should be doing this in the context of any
> > filesystem. If EVM does that, EVM is doing it wrong.
> 
> EVM sounds buggy.
> 
> In general if you look at current *at all* in an implementation of
> write() *in any filesystem*, you are doing it wrong.

I have read CVE-2013-1959... Thanks for Jann and Andy's review.

I will create the sysfs interface through other way, then using 
file_ns_capable() for capability checking in next version.

Thanks a lot!
Joey Lee