Re: [RFC v2 3/7] integrity: create and inititialize a keyring with builtin public key

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Thu, 2012-08-16 at 14:37 -0400, Josh Boyer wrote:
> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
> <dmitry.kasatkin@intel.com> wrote:
> > From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >
> > Create and initialize a keyring with the builtin public key. This could
> > be an ephemeral key, created and destroyed during module install for
> > custom built kernels, or a key used to label the entire filesystem for
> > EVM/IMA-appraisal. Uses .incbin based on David Howell's post.
> >
> > Load the builtin public key on the specified keyring, creating the
> > keyring if it doesn't already exist.
> >
> > Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > ---
> >  security/integrity/Kconfig         |   10 ++++
> >  security/integrity/Makefile        |   17 +++++++
> >  security/integrity/digsig_pubkey.c |   96 ++++++++++++++++++++++++++++++++++++
> >  security/integrity/integrity.h     |   10 ++++
> >  4 files changed, 133 insertions(+)
> >  create mode 100644 security/integrity/digsig_pubkey.c
> >
> > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> > index 5bd1cc1..f789018 100644
> > --- a/security/integrity/Kconfig
> > +++ b/security/integrity/Kconfig
> > @@ -17,5 +17,15 @@ config INTEGRITY_SIGNATURE
> >           This is useful for evm and module keyrings, when keys are
> >           usually only added from initramfs.
> >
> > +config INTEGRITY_PUBKEY
> > +       boolean "Create a keyring and initialize with builtin public key"
> > +       depends on INTEGRITY_SIGNATURE
> > +       default n
> > +       help
> > +         Create and initialize a keyring with the builtin public key. This could
> > +         be an ephemeral key, created and destroyed during module install for
> > +         custom built kernels, or a key used to label the entire filesystem for
> > +         EVM/IMA-appraisal.
> > +
> 
> It could also be a key that is reused explicitly for signing kernels
> and kernel modules but has nothing to do with EVM/IMA filesystem
> labels, right?  E.g. a distro key.  I think the commit log and help
> text is a bit too restrictive here.

Definitely!  The cover letter included the distro scenario, but I missed
updating it here.

> > +int integrity_init_keyring(const unsigned int id)
> > +{
> > +       const struct cred *cred = current_cred();
> > +       struct user_struct *user = cred->user;
> > +       struct key *new_keyring, *key;
> > +       u8 digest[SHA1_DIGEST_SIZE];
> > +       char keyid[20];
> > +       int ret, pubkey_size = pubkey_end - pubkey;
> > +
> > +       if (pubkey_size == 0) {
> > +               pr_info("pubkey is missing, skipping...\n");
> > +               return 0;
> > +       }
> > +
> > +       new_keyring = keyring_alloc(keyring_name[id], user->uid, (gid_t) -1,
> > +                                   cred, KEY_ALLOC_NOT_IN_QUOTA,
> > +                                   user->uid_keyring);
> > +       if (IS_ERR(new_keyring)) {
> > +               ret = PTR_ERR(new_keyring);
> > +               goto out;
> > +       }
> > +
> > +       ret = integrity_calc_digest("sha1", pubkey, pubkey_size, digest);
> > +       if (ret < 0)
> > +               goto out;
> > +
> > +       sprintf(keyid, "%llX", __be64_to_cpup((uint64_t *)(digest+12)));
> 
> keyid is only 20 bytes.  Is there a guarantee somewhere that restricts
> the digest+12 value to be 20 bytes or less and NUL termintated?  If
> not, should you use snprintf?

Correct, and SHA1 shouldn't be hardcoded either, but configurable.

> 
> > +
> > +       key = key_alloc(&key_type_user, keyid, 0, 0, current_cred(),
> > +                       (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > +                       KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_READ,
> > +                       KEY_ALLOC_NOT_IN_QUOTA);
> > +       if (IS_ERR(key)) {
> > +               ret = PTR_ERR(key);
> > +               goto out;
> 
> You leak new_keyring if here, right?

thanks

> > +       }
> > +
> > +       ret = key_instantiate_and_link(key, pubkey, pubkey_end - pubkey,
> > +                                      new_keyring, NULL);
> > +out:
> > +       pr_info("integrity: loaded public key %s on %s %s\n", keyid,
> > +               keyring_name[id], !ret ? "succeeded" : "failed");
> > +       return ret;
> > +}
> 
> josh

Mimi