lkml.org 
[lkml]   [2014]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring
    From
    Date
    On Wed, 2014-05-28 at 21:55 +0300, Dmitry Kasatkin wrote: 
    > On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
    > > Require all keys added to the IMA keyring be signed by an
    > > existing trusted key on the system trusted keyring.
    > >
    > > Changelog v1:
    > > - don't link IMA trusted keyring to user keyring
    > >
    > > Changelog:
    > > - define stub integrity_init_keyring() function (reported-by Fengguang Wu)
    > > - differentiate between regular and trusted keyring names.
    > > - replace printk with pr_info (D. Kasatkin)
    > > - only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
    > > - define stub integrity_init_keyring() definition based on
    > > CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
    > > (reported-by Jim Davis)
    > >
    > > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
    > > ---
    > > security/integrity/digsig.c | 26 +++++++++++++++++++++++++-
    > > security/integrity/ima/Kconfig | 8 ++++++++
    > > security/integrity/ima/ima_appraise.c | 11 +++++++++++
    > > security/integrity/integrity.h | 5 +++++
    > > 4 files changed, 49 insertions(+), 1 deletion(-)
    > >
    > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
    > > index b4af4eb..7da5f9c 100644
    > > --- a/security/integrity/digsig.c
    > > +++ b/security/integrity/digsig.c
    > > @@ -13,7 +13,9 @@
    > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
    > >
    > > #include <linux/err.h>
    > > +#include <linux/sched.h>
    > > #include <linux/rbtree.h>
    > > +#include <linux/cred.h>
    > > #include <linux/key-type.h>
    > > #include <linux/digsig.h>
    > >
    > > @@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
    > > static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
    > > "_evm",
    > > "_module",
    > > +#ifndef CONFIG_IMA_TRUSTED_KEYRING
    > > "_ima",
    > > +#else
    > > + ".ima",
    > > +#endif
    > > };
    > >
    > > int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
    > > @@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
    > >
    > > if (!keyring[id]) {
    > > keyring[id] =
    > > - request_key(&key_type_keyring, keyring_name[id], NULL);
    > > + request_key(&key_type_keyring, keyring_name[id], NULL);
    > > if (IS_ERR(keyring[id])) {
    > > int err = PTR_ERR(keyring[id]);
    > > pr_err("no %s keyring: %d\n", keyring_name[id], err);
    > > @@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
    > >
    > > return -EOPNOTSUPP;
    > > }
    > > +
    > > +int integrity_init_keyring(const unsigned int id)
    > > +{
    > > + const struct cred *cred = current_cred();
    > > +
    > > + keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
    > > + KGIDT_INIT(0), cred,
    > > + ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
    > > + KEY_USR_VIEW | KEY_USR_READ |
    > > + KEY_USR_WRITE | KEY_USR_SEARCH),
    > > + KEY_ALLOC_NOT_IN_QUOTA, NULL);
    >
    > Last parameter "destination" is NULL. It makes keyring "unsearchable"
    > from user space.
    > It prevents loading trusted keys from user-space, e.g. initramfs...
    >
    > Should it be "cred->user->uid_keyring"??

    David extended keyctl with the '%keyring' option. For example,
    "keyctl show %keyring:.ima" returns the .ima keyring id with a list of
    all the keys.

    >
    >
    > > + if (!IS_ERR(keyring[id]))
    > > + set_bit(KEY_FLAG_TRUSTED_ONLY, &keyring[id]->flags);
    > > + else
    > > + pr_info("Can't allocate %s keyring (%ld)\n",
    > > + keyring_name[id], PTR_ERR(keyring[id]));
    >
    > keyring[id] should be set "back" to NULL. Otherwise bad value might be
    > used in other places.

    Good catch, thanks.

    >
    > > + return 0;
    > > +}
    > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
    > > index 81a2797..dad8d4c 100644
    > > --- a/security/integrity/ima/Kconfig
    > > +++ b/security/integrity/ima/Kconfig
    > > @@ -123,3 +123,11 @@ config IMA_APPRAISE
    > > For more information on integrity appraisal refer to:
    > > <http://linux-ima.sourceforge.net>
    > > If unsure, say N.
    > > +
    > > +config IMA_TRUSTED_KEYRING
    > > + bool "Require all keys on the _ima keyring be signed"
    > > + depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
    > > + default y
    > > + help
    > > + This option requires that all keys added to the _ima
    > > + keyring be signed by a key on the system trusted keyring.
    > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
    > > index d3113d4..003ff46 100644
    > > --- a/security/integrity/ima/ima_appraise.c
    > > +++ b/security/integrity/ima/ima_appraise.c
    > > @@ -385,3 +385,14 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
    > > }
    > > return result;
    > > }
    > > +
    > > +#ifdef CONFIG_IMA_TRUSTED_KEYRING
    > > +static int __init init_ima_keyring(void)
    > > +{
    > > + int ret;
    > > +
    > > + ret = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
    > > + return 0;
    > > +}
    > > +late_initcall(init_ima_keyring);
    >
    >
    > late_initcall(init_ima_keyring) ordering competes with late_initcall(init_ima);
    > but we want keyring to be initialized before IMA might use it.

    > In the case when we would load keys from ima kernel initialization
    > code, order is important.
    >
    > we already have init_ima() and ima_init calls().
    > Why not call integrity_init_keyring() from there?
    >
    > Indeed, we have one late_initcall(init_evm) for EVM, and one
    > late_initcall(init_ima) for IMA.
    >
    > It's enough...

    Right, there's no reason to have an additional call.

    thanks,

    Mimi



    \
     
     \ /
      Last update: 2014-05-28 22:01    [W:4.546 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site