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:	Fri, 17 Aug 2012 00:11:34 +0300
From:	"Kasatkin, Dmitry" <dmitry.kasatkin@...el.com>
To:	Josh Boyer <jwboyer@...il.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 Thu, Aug 16, 2012 at 9:37 PM, Josh Boyer <jwboyer@...il.com> wrote:
> 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?
>

No.. Digest here is 20 bytes, but keyid is 8 bytes - it is u64...
It cannot go beyond here...

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