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

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 16 Aug 2012 14:37:52 -0400
From:	Josh Boyer <jwboyer@...il.com>
To:	Dmitry Kasatkin <dmitry.kasatkin@...el.com>
Cc:	zohar@...ux.vnet.ibm.com, jmorris@...ei.org, rusty@...tcorp.com.au,
	dhowells@...hat.com, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, Mimi Zohar <zohar@...ibm.com>
Subject: Re: [RFC v2 3/7] integrity: create and inititialize a keyring with
 builtin public key

On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin
<dmitry.kasatkin@...el.com> wrote:
> From: Mimi Zohar <zohar@...ux.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@...ibm.com>
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@...el.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.

> +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/