| 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
| ||
|
Message-ID: <CALLzPKZbedaJPiZL+86CiJ11np4gV_f_B1dJ6zdZmMdeq9pO5g@mail.gmail.com>
Date: Fri, 17 Aug 2012 09:06:25 +0300
From: "Kasatkin, Dmitry" <dmitry.kasatkin@...el.com>
To: Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc: Josh Boyer <jwboyer@...il.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 10:28 PM, Mimi Zohar <zohar@...ux.vnet.ibm.com> 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@...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.
>
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists