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

[Kasatkin, Dmitry]

On Thu, Aug 16, 2012 at 10:28 PM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxx> wrote:
>> > From: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxx>
>> > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx>
>> > ---
>> >  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
>

Actually we are not leaking.
new_keyring is instantiated and linked to the uid keyring..

Another question is, do we want to leave keyring there for "may be
later" use, or just destroy...

>> > +       }
>> > +
>> > +       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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/