Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932617Ab2HQGGa (ORCPT ); Fri, 17 Aug 2012 02:06:30 -0400 Received: from mga09.intel.com ([134.134.136.24]:59153 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754681Ab2HQGG1 (ORCPT ); Fri, 17 Aug 2012 02:06:27 -0400 MIME-Version: 1.0 In-Reply-To: <1345145332.3402.13.camel@falcor.watson.ibm.com> References: <66e0f332c823b82701a48aba320f589cec27e904.1345055639.git.dmitry.kasatkin@intel.com> <1345145332.3402.13.camel@falcor.watson.ibm.com> Date: Fri, 17 Aug 2012 09:06:25 +0300 Message-ID: Subject: Re: [RFC v2 3/7] integrity: create and inititialize a keyring with builtin public key From: "Kasatkin, Dmitry" To: Mimi Zohar Cc: Josh Boyer , jmorris@namei.org, rusty@rustcorp.com.au, dhowells@redhat.com, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Mimi Zohar Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4964 Lines: 125 On Thu, Aug 16, 2012 at 10:28 PM, Mimi Zohar wrote: > On Thu, 2012-08-16 at 14:37 -0400, Josh Boyer wrote: >> On Wed, Aug 15, 2012 at 2:43 PM, Dmitry Kasatkin >> wrote: >> > From: Mimi Zohar >> > >> > 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 >> > Signed-off-by: Dmitry Kasatkin >> > --- >> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/