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

From: Josh Boyer
Date: Thu Aug 16 2012 - 14:37:47 EST


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.

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

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

> + }
> +
> + 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
--
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/