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

[Mimi Zohar]

On Tue, 2018-10-02 at 21:36 +0200, Jann Horn 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@xxxxxxxx> 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@xxxxxxxxx> 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;
> > ...
> >
> > 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;
> > ...
> >
> >
> > 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.

Thanks, Jann. ÂAt least EVM should be updated to replace the existing
"capable" check with "if (file_ns_capable(file, &init_user_ns,
CAP_SYS_ADMIN))".Â

> 
> > > > +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.

This is being executed in the initramfs before pivoting root.

> 
> > > > +       if (IS_ERR(key)) {
> > > > +               pr_err("Request key error: %ld\n", PTR_ERR(key));
> > > > +               err = PTR_ERR(key);
> > > > +               return err;
> > > > +       }
> > > > +
> > > > +       down_write(&key->sem);
> > > > +       ukp = user_key_payload_locked(key);
> > > > +       if (!ukp) {
> > > > +               /* key was revoked before we acquired its semaphore */
> > > > +               err = -EKEYREVOKED;
> > > > +               goto key_invalid;
> > > > +       }
> > > > +       if (invalid_key(ukp->data, ukp->datalen)) {
> > > > +               err = -EINVAL;
> > > > +               goto key_invalid;
> > > > +       }
> > > > +       skey.key_len = ukp->datalen;
> > > > +       memcpy(skey.key, ukp->data, ukp->datalen);
> > > > +       /* burn the original key contents */
> > > > +       memzero_explicit(ukp->data, ukp->datalen);
> > >
> > > You just zero out the contents of the supplied key? That seems very
> > > unidiomatic for the keys subsystem, and makes me wonder why you're
> > > using the keys subsystem for this in the first place. It doesn't look
> > > like normal use of the keys subsystem.
> > >

Right, evm_write_key() is normally called from the initramfs to signal
the kernel that the EVM encrypted key has been loaded onto root's
keyring. ÂThe decrypted EVM key is then copied to kernel memory.
ÂBefore returning, the decrypted EVM key on root's keyring is cleared.
The initramfs then removes the EVM key from the keyring.

For more details about trusted/encrypted keys refer to
Documentation/security/keys/trusted-encrypted.rst.

Mimi


> > Because I want that only one decrypted key in kernel memory. Then hibernation
> > can handle the key more easy. In evm_init_key(), it also burned the key
> > contents after evm key be initialled:
> >
> > security/integrity/evm/evm_crypto.c
> > int evm_init_key(void)
> > {
> > [...snip]
> >         /* burn the original key contents */
> >         memset(ekp->decrypted_data, 0, ekp->decrypted_datalen);
> >         up_read(&evm_key->sem);
> >         key_put(evm_key);
> >         return rc;
> > }
> >
> > The keys subsystem already handles the interactive with userland and TPM.
> > That's the reason for using keys subsystem in hibernation.
> 
> How do you guarantee that the user is allowed to overwrite that key? I
> don't understand the keys subsystem very well - could this be a key on
> the trusted keyring, or something like that?
>