LinuxLists.cc - [PATCH] Keys: Add LSM hooks for key management

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Wed, 5 Oct 2005, David Howells wrote:

> Key management access control through LSM is enabled by CONFIG_SECURITY_KEYS.

Any reason why this is configurable? Why wouldn't someone want this?

- James
--
James Morris
<[email protected]>

2005-10-05 16:49:33

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> > Key management access control through LSM is enabled by
> > CONFIG_SECURITY_KEYS.
>
> Any reason why this is configurable?

Well, I saw that the network stuff was. I can make it non-configurable.

> Why wouldn't someone want this?

Speed/latency? But I suppose that's not really a factor.

What about the security ops for keys that I've made available? Does doing it
that way seem reasonable?

David

2005-10-05 18:41:14

by Serge E. Hallyn

[permalink] [raw]

Subject: Re: [PATCH] Keys: Add LSM hooks for key management

> +int key_task_permission(const key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm)
> +{
> + struct key *key;
> + key_perm_t kperm;
> + int ret;
> +
> + /* let the security module have first say
> + * - it should return:
> + * +ve to grant access
> + * 0 to deny access
> + * -ve to fall back to normal permission checking
> + */
> + ret = security_key_permission(key_ref, context, perm);
> + if (ret >= 0)
> + return ret;

Hmm, my only problem here is that this is nonstandard compared to
expected return values from other security_ authorization hooks.
Could this be switched to

-ve : deny access (and return the error)
0: grant access
+ve: fall back to normal permission checking

Actually that's still nonstandard. On the whole, LSM only
restricts, does not authorize, with capable() being the notable
exception. Is there good reason to allow LSMs to fully authorize
in this case?

thanks,
-serge

2005-10-05 19:32:05

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Wed, 5 Oct 2005, David Howells wrote:

> > Any reason why this is configurable?
>
> Well, I saw that the network stuff was. I can make it non-configurable.
>
> > Why wouldn't someone want this?
>
> Speed/latency? But I suppose that's not really a factor.

Yes, the networking is for performance, especially from when we used to
register Netfilter hooks from within LSM. I don't know of any distros
that enable LSM but disable networking so we should probably think about
removing that as well.

> What about the security ops for keys that I've made available? Does doing it
> that way seem reasonable?

Not sure yet, need to spend some time looking at this from an SELinux
point of view.

- James
--
James Morris
<[email protected]>

2005-10-05 21:10:50

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

* David Howells ([email protected]) wrote:

Might be useful to take one step back and define the actions that users
can take on keys and show that each is mediated by label check. Looks
like key_permission does most of this, but key_type has:

->instantiate
->duplicate
->update
->match
->describe
->read

So the hooks you added control search/read/write via permission, and
then special case instantiate and update. Does this mean you expect the
security module to inspect the key data and set a label based on data?
Does duplicate need to update label to a new context, or should it get
identical lable, and does it need a hook for that?

> diff -uNrp linux-2.6.14-rc3-keys/include/linux/key.h linux-2.6.14-rc3-keys-lsm/include/linux/key.h
> --- linux-2.6.14-rc3-keys/include/linux/key.h 2005-10-05 11:50:22.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/include/linux/key.h 2005-10-05 17:06:42.000000000 +0100
> @@ -23,6 +23,8 @@
>
> #ifdef __KERNEL__
>
> +#undef KEY_DEBUGGING
> +
> /* key handle serial number */
> typedef int32_t key_serial_t;
>
> @@ -31,9 +33,39 @@ typedef uint32_t key_perm_t;
>
> struct key;
>
> +/*****************************************************************************/
> +/*
> + * key reference with possession attribute handling
> + *
> + * NOTE! key_ref_t is a typedef'd pointer to a type that is not actually
> + * defined. This is because we abuse the bottom bit of the reference to carry a
> + * flag to indicate whether the calling process possesses that key in one of
> + * its keyrings.
> + *
> + * the key_ref_t has been made a separate type so that the compiler can reject
> + * attempts to dereference it without proper conversion.
> + *
> + * the three functions are used to assemble and disassemble references
> + */
> +typedef struct __key_reference_with_attributes *key_ref_t;

> +
> #ifdef CONFIG_KEYS
>
> -#undef KEY_DEBUGGING
> +static inline key_ref_t make_key_ref(const struct key *key,
> + unsigned long possession)
> +{
> + return (key_ref_t) ((unsigned long) key | possession);
> +}
> +
> +static inline struct key *key_ref_to_ptr(const key_ref_t key_ref)
> +{
> + return (struct key *) ((unsigned long) key_ref & ~1UL);
> +}
> +
> +static inline unsigned long is_key_possessed(const key_ref_t key_ref)
> +{
> + return (unsigned long) key_ref & 1UL;
> +}
>
> #define KEY_POS_VIEW 0x01000000 /* possessor can view a key's attributes */
> #define KEY_POS_READ 0x02000000 /* possessor can read key payload / view keyring */
> @@ -74,38 +106,6 @@ struct keyring_name;
>
> /*****************************************************************************/
> /*
> - * key reference with possession attribute handling
> - *
> - * NOTE! key_ref_t is a typedef'd pointer to a type that is not actually
> - * defined. This is because we abuse the bottom bit of the reference to carry a
> - * flag to indicate whether the calling process possesses that key in one of
> - * its keyrings.
> - *
> - * the key_ref_t has been made a separate type so that the compiler can reject
> - * attempts to dereference it without proper conversion.
> - *
> - * the three functions are used to assemble and disassemble references
> - */
> -typedef struct __key_reference_with_attributes *key_ref_t;
> -
> -static inline key_ref_t make_key_ref(const struct key *key,
> - unsigned long possession)
> -{
> - return (key_ref_t) ((unsigned long) key | possession);
> -}
> -
> -static inline struct key *key_ref_to_ptr(const key_ref_t key_ref)
> -{
> - return (struct key *) ((unsigned long) key_ref & ~1UL);
> -}
> -
> -static inline unsigned long is_key_possessed(const key_ref_t key_ref)
> -{
> - return (unsigned long) key_ref & 1UL;
> -}
> -
> -/*****************************************************************************/

This move of key_ref_t handling is either unrelated or simple prep work,
yes? Just clutters the patch.

> -/*
> * authentication token / access credential / keyring
> * - types of key include:
> * - keyrings
> @@ -119,6 +119,7 @@ struct key {
> struct key_type *type; /* type of key */
> struct rw_semaphore sem; /* change vs change sem */
> struct key_user *user; /* owner of this key */
> + void *security; /* security data for this key */
> time_t expiry; /* time at which key expires (or 0) */
> uid_t uid;
> gid_t gid;
> diff -uNrp linux-2.6.14-rc3-keys/include/linux/key-ui.h linux-2.6.14-rc3-keys-lsm/include/linux/key-ui.h
> --- linux-2.6.14-rc3-keys/include/linux/key-ui.h 2005-10-03 10:48:38.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/include/linux/key-ui.h 2005-10-05 15:02:55.000000000 +0100
> @@ -38,97 +38,16 @@ struct keyring_list {
> struct key *keys[0];
> };
>
> +extern int key_task_permission(const key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm);
>
> /*
> * check to see whether permission is granted to use a key in the desired way
> */
> static inline int key_permission(const key_ref_t key_ref, key_perm_t perm)
> {
> - struct key *key = key_ref_to_ptr(key_ref);
> - key_perm_t kperm;
> -
> - if (is_key_possessed(key_ref))
> - kperm = key->perm >> 24;
> - else if (key->uid == current->fsuid)
> - kperm = key->perm >> 16;
> - else if (key->gid != -1 &&
> - key->perm & KEY_GRP_ALL &&
> - in_group_p(key->gid)
> - )
> - kperm = key->perm >> 8;
> - else
> - kperm = key->perm;
> -
> - kperm = kperm & perm & KEY_ALL;
> -
> - return kperm == perm;
> -}
> -
> -/*
> - * check to see whether permission is granted to use a key in at least one of
> - * the desired ways
> - */
> -static inline int key_any_permission(const key_ref_t key_ref, key_perm_t perm)
> -{
> - struct key *key = key_ref_to_ptr(key_ref);
> - key_perm_t kperm;
> -
> - if (is_key_possessed(key_ref))
> - kperm = key->perm >> 24;
> - else if (key->uid == current->fsuid)
> - kperm = key->perm >> 16;
> - else if (key->gid != -1 &&
> - key->perm & KEY_GRP_ALL &&
> - in_group_p(key->gid)
> - )
> - kperm = key->perm >> 8;
> - else
> - kperm = key->perm;
> -
> - kperm = kperm & perm & KEY_ALL;
> -
> - return kperm != 0;
> -}

Again patch clutter, since this is just removing unused code AFAICT
(which should definitely be removed). I mention only because it makes it
harder to review since I don't know the key infrastructure as well as you.

> -static inline int key_task_groups_search(struct task_struct *tsk, gid_t gid)
> -{
> - int ret;
> -
> - task_lock(tsk);
> - ret = groups_search(tsk->group_info, gid);
> - task_unlock(tsk);
> - return ret;
> -}
> -
> -static inline int key_task_permission(const key_ref_t key_ref,
> - struct task_struct *context,
> - key_perm_t perm)
> -{
> - struct key *key = key_ref_to_ptr(key_ref);
> - key_perm_t kperm;
> -
> - if (is_key_possessed(key_ref)) {
> - kperm = key->perm >> 24;
> - }
> - else if (key->uid == context->fsuid) {
> - kperm = key->perm >> 16;
> - }
> - else if (key->gid != -1 &&
> - key->perm & KEY_GRP_ALL && (
> - key->gid == context->fsgid ||
> - key_task_groups_search(context, key->gid)
> - )
> - ) {
> - kperm = key->perm >> 8;
> - }
> - else {
> - kperm = key->perm;
> - }
> -
> - kperm = kperm & perm & KEY_ALL;
> -
> - return kperm == perm;
> -
> + return key_task_permission(key_ref, current, perm);

Just curious, from quick look, appears that key_task_permission is
basically always called with current. I found one (which appears to be
a recursive call) that uses rka->context in search_process_keyrings.
What case causes context != current?

> }
>
> extern key_ref_t lookup_user_key(struct task_struct *context,
> diff -uNrp linux-2.6.14-rc3-keys/include/linux/security.h linux-2.6.14-rc3-keys-lsm/include/linux/security.h
> --- linux-2.6.14-rc3-keys/include/linux/security.h 2005-10-03 10:48:39.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/include/linux/security.h 2005-10-05 16:10:53.000000000 +0100
> @@ -30,6 +30,7 @@
> #include <linux/shm.h>
> #include <linux/msg.h>
> #include <linux/sched.h>
> +#include <linux/key.h>
>
> struct ctl_table;
>
> @@ -785,6 +786,54 @@ struct swap_info_struct;
> * @sk_free_security:
> * Deallocate security structure.
> *
> + * Security hooks affecting all Key Management operations
> + *
> + * @key_alloc:
> + * Permit allocation a key and assign security data. Note that key does
> + * not have a serial number assigned at this point.
> + * @key points to the key.
> + * Return 0 if permission is granted, -ve error otherwise.
> + * @key_post_alloc:
> + * Notification of key publication; key has serial number.
> + * @key points to the key.
> + * No return.
> + * @key_free:
> + * Notification of destruction; free security data.
> + * @key points to the key.
> + * No return.
> + * @key_set_security:
> + * Apply security data to a key.
> + * @key points to the key.
> + * @name points to the name of the attribute.
> + * @data points to the data (may be NULL).
> + * @dlen indicates the size of the data (may be 0).
> + * Return 0 if successful, -ve error otherwise.
> + * @key_get_security:
> + * Retrieve security data from a key.
> + * @key points to the key.
> + * @name points to the name of the attribute.
> + * @buffer points to the buffer (may be NULL if size wanted).
> + * @blen indicates the size of the buffer (may be 0 if buffer is NULL).
> + * Load buffer with up to maximum of blen bytes upon successful return.
> + * Return number of bytes retrievable if successful, -ve error otherwise.
> + * @key_permission:
> + * See whether a specific operational right is granted to a process on a
> + * key.
> + * @key_ref refers to the key (key pointer + possession attribute bit).
> + * @context points to the process to provide the context against which to
> + * evaluate the security data on the key.
> + * @perm describes the combination of permissions required of this key.
> + * Return 1 if permission granted, 0 if permission denied and -ve it the
> + * normal permissions model should be effected.
> + * @key_set_data:
> + * Test whether a key may be set to the given data, and adjust the
> + * security information accordingly.
> + * @key points to the key being altered.
> + * @instkey points to the instantiation key if the key is being
> + * instantiated, and NULL if being updated.
> + * @data points to the data.
> + * @dlen indicates the size of the data.
> + *
> * Security hooks affecting all System V IPC operations.
> *
> * @ipc_permission:
> @@ -1213,6 +1262,28 @@ struct security_operations {
> int (*sk_alloc_security) (struct sock *sk, int family, int priority);
> void (*sk_free_security) (struct sock *sk);
> #endif /* CONFIG_SECURITY_NETWORK */
> +
> + /* key management security hooks */
> +#ifdef CONFIG_SECURITY_KEYS
> + int (*key_alloc)(struct key *key);
> + void (*key_post_alloc)(struct key *key);
> + void (*key_free)(struct key *key);
> + long (*key_set_security)(struct key *key, const char __user *name,
> + const void __user *data, size_t dlen);
> + long (*key_get_security)(struct key *key, const char __user *name,
> + void __user *buffer, size_t blen);
> +
> + int (*key_permission)(key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm);
> +
> + int (*key_set_data)(struct key *key,
> + struct key *instkey,
> + const void *data,
> + size_t dlen);
> +
> +#endif /* CONFIG_SECURITY_KEYS */
> +
> };
>
> /* global variables */
> @@ -2763,5 +2834,101 @@ static inline void security_sk_free(stru
> }
> #endif /* CONFIG_SECURITY_NETWORK */
>
> +#ifdef CONFIG_SECURITY_KEYS
> +
> +static inline int security_key_alloc(struct key *key)
> +{
> + return security_ops->key_alloc(key);
> +}
> +
> +static inline void security_key_post_alloc(struct key *key)
> +{
> + security_ops->key_post_alloc(key);
> +}
> +
> +static inline void security_key_free(struct key *key)
> +{
> + security_ops->key_free(key);
> +}
> +
> +static inline long security_key_set_security(struct key *key,
> + const char __user *name,
> + const void __user *data,
> + size_t dlen)
> +{
> + return security_ops->key_set_security(key, name, data, dlen);
> +}
> +
> +static inline long security_key_get_security(struct key *key,
> + const char __user *name,
> + void __user *buffer,
> + size_t blen)
> +{
> + return security_ops->key_get_security(key, name, buffer, blen);
> +}
> +
> +static inline int security_key_permission(key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm)
> +{
> + return security_ops->key_permission(key_ref, context, perm);
> +}
> +
> +static inline int security_key_set_data(struct key *key,
> + struct key *instkey,
> + const void *data,
> + size_t dlen)
> +{
> + return security_ops->key_set_data(key, instkey, data, dlen);
> +}
> +
> +#else
> +
> +static inline int security_key_alloc(struct key *key)
> +{
> + return 0;
> +}
> +
> +static inline void security_key_post_alloc(struct key *key)
> +{
> +}
> +
> +static inline void security_key_free(struct key *key)
> +{
> +}
> +
> +static inline long security_key_set_security(struct key *key,
> + const char __user *name,
> + const void __user *data,
> + size_t dlen)
> +{
> + return 0;
> +}
> +
> +static inline long security_key_get_security(struct key *key,
> + const char __user *name,
> + void __user *buffer,
> + size_t blen)
> +{
> + return -ENODATA;
> +}
> +
> +static inline int security_key_permission(key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm)
> +{
> + return -1;
> +}
> +
> +static inline int security_key_set_data(struct key *key,
> + struct key *instkey,
> + const void *data,
> + size_t dlen)
> +{
> + return 0;
> +}
> +
> +#endif
> +
> #endif /* ! __LINUX_SECURITY_H */
>
> diff -uNrp linux-2.6.14-rc3-keys/security/dummy.c linux-2.6.14-rc3-keys-lsm/security/dummy.c
> --- linux-2.6.14-rc3-keys/security/dummy.c 2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/dummy.c 2005-10-05 16:11:38.000000000 +0100
> @@ -803,6 +803,50 @@ static int dummy_setprocattr(struct task
> return -EINVAL;
> }
>
> +static inline int dummy_key_alloc(struct key *key)
> +{
> + return 0;
> +}
> +
> +static inline void dummy_key_post_alloc(struct key *key)
> +{
> +}
> +
> +static inline void dummy_key_free(struct key *key)
> +{
> +}
> +
> +static inline long dummy_key_set_security(struct key *key,
> + const char __user *name,
> + const void __user *data,
> + size_t dlen)
> +{
> + return 0;
> +}
> +
> +static inline long dummy_key_get_security(struct key *key,
> + const char __user *name,
> + void __user *buffer,
> + size_t blen)
> +{
> + return -ENODATA;
> +}
> +
> +static inline int dummy_key_permission(key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm)
> +{
> + return -1;
> +}
> +
> +static inline int dummy_key_set_data(struct key *key,
> + struct key *instkey,
> + const void *data,
> + size_t dlen)
> +{
> + return 0;
> +}
> +
>
> struct security_operations dummy_security_ops;
>
> @@ -954,5 +998,15 @@ void security_fixup_ops (struct security
> set_to_dummy_if_null(ops, sk_alloc_security);
> set_to_dummy_if_null(ops, sk_free_security);
> #endif /* CONFIG_SECURITY_NETWORK */
> +#ifdef CONFIG_SECURITY_KEYS
> + set_to_dummy_if_null(ops, key_alloc);
> + set_to_dummy_if_null(ops, key_post_alloc);
> + set_to_dummy_if_null(ops, key_free);
> + set_to_dummy_if_null(ops, key_set_security);
> + set_to_dummy_if_null(ops, key_get_security);
> + set_to_dummy_if_null(ops, key_permission);
> + set_to_dummy_if_null(ops, key_set_data);
> +#endif /* CONFIG_SECURITY_KEYS */
> +
> }
>
> diff -uNrp linux-2.6.14-rc3-keys/security/Kconfig linux-2.6.14-rc3-keys-lsm/security/Kconfig
> --- linux-2.6.14-rc3-keys/security/Kconfig 2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/Kconfig 2005-10-05 13:59:45.000000000 +0100
> @@ -74,6 +74,15 @@ config SECURITY_ROOTPLUG
>
> If you are unsure how to answer this question, answer N.
>
> +config SECURITY_KEYS
> + bool "Key Management Security Hooks"
> + depends on SECURITY && KEYS
> + help
> + This enables the key management security hooks.
> + If enabled, a security module can use these hooks to
> + implement access controls on keys.
> + If you are unsure how to answer this question, answer N.
> +
> config SECURITY_SECLVL
> tristate "BSD Secure Levels"
> depends on SECURITY
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/compat.c linux-2.6.14-rc3-keys-lsm/security/keys/compat.c
> --- linux-2.6.14-rc3-keys/security/keys/compat.c 2005-08-30 13:56:44.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/compat.c 2005-10-05 16:30:38.000000000 +0100
> @@ -74,6 +74,14 @@ asmlinkage long compat_sys_keyctl(u32 op
> case KEYCTL_SET_REQKEY_KEYRING:
> return keyctl_set_reqkey_keyring(arg2);
>
> + case KEYCTL_SET_SECURITY:
> + return keyctl_set_security(arg2, compat_ptr(arg3),
> + compat_ptr(arg4), arg5);
> +
> + case KEYCTL_GET_SECURITY:
> + return keyctl_get_security(arg2, compat_ptr(arg3),
> + compat_ptr(arg4), arg5);
> +
> default:
> return -EOPNOTSUPP;
> }
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/internal.h linux-2.6.14-rc3-keys-lsm/security/keys/internal.h
> --- linux-2.6.14-rc3-keys/security/keys/internal.h 2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/internal.h 2005-10-05 16:29:39.000000000 +0100
> @@ -12,6 +12,7 @@
> #ifndef _INTERNAL_H
> #define _INTERNAL_H
>
> +#include <linux/security.h>

This looks unnecessary. I'd include it where it's used.

> #include <linux/key.h>
> #include <linux/key-ui.h>
>
> @@ -137,7 +138,10 @@ extern long keyctl_instantiate_key(key_s
> size_t, key_serial_t);
> extern long keyctl_negate_key(key_serial_t, unsigned, key_serial_t);
> extern long keyctl_set_reqkey_keyring(int);
> -
> +extern long keyctl_set_security(key_serial_t, const char __user *,
> + const void __user *, size_t);
> +extern long keyctl_get_security(key_serial_t, const char __user *,
> + void __user *, size_t);
>
> /*
> * debugging key validation
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/key.c linux-2.6.14-rc3-keys-lsm/security/keys/key.c
> --- linux-2.6.14-rc3-keys/security/keys/key.c 2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/key.c 2005-10-05 15:33:29.000000000 +0100
> @@ -1,6 +1,6 @@
> /* key.c: basic authentication token and access key management
> *
> - * Copyright (C) 2004-5 Red Hat, Inc. All Rights Reserved.
> + * Copyright (C) 2004 Red Hat, Inc. All Rights Reserved.
> * Written by David Howells ([email protected])
> *
> * This program is free software; you can redistribute it and/or
> @@ -253,6 +253,7 @@ struct key *key_alloc(struct key_type *t
> struct key_user *user = NULL;
> struct key *key;
> size_t desclen, quotalen;
> + int ret;
>
> key = ERR_PTR(-EINVAL);
> if (!desc || !*desc)
> @@ -305,6 +306,7 @@ struct key *key_alloc(struct key_type *t
> key->flags = 0;
> key->expiry = 0;
> key->payload.data = NULL;
> + key->security = NULL;
>
> if (!not_in_quota)
> key->flags |= 1 << KEY_FLAG_IN_QUOTA;
> @@ -315,16 +317,37 @@ struct key *key_alloc(struct key_type *t
> key->magic = KEY_DEBUG_MAGIC;
> #endif
>
> + /* do a final security check before publishing the key */
> + ret = security_key_alloc(key);

This may simply be allocating space for the label (and possibly labelling)
not necessarily a security check.

> + if (ret < 0)
> + goto security_error;
> +
> /* publish the key by giving it a serial number */
> atomic_inc(&user->nkeys);
> key_alloc_serial(key);
>
> - error:
> + /* let the security module know the key has been published */
> + security_key_post_alloc(key);

This is odd, esp since nothing could have failed between alloc and
publish. Only state change is serial number. Would you expect the
security module to update a label based on serial number?

> +
> +error:
> return key;
>
> - no_memory_3:
> +security_error:
> + kfree(key->description);
> + kmem_cache_free(key_jar, key);
> + if (!not_in_quota) {
> + spin_lock(&user->lock);
> + user->qnkeys--;
> + user->qnbytes -= quotalen;
> + spin_unlock(&user->lock);
> + }
> + key_user_put(user);
> + key = ERR_PTR(ret);
> + goto error;
> +
> +no_memory_3:
> kmem_cache_free(key_jar, key);
> - no_memory_2:
> +no_memory_2:
> if (!not_in_quota) {
> spin_lock(&user->lock);
> user->qnkeys--;
> @@ -332,11 +355,11 @@ struct key *key_alloc(struct key_type *t
> spin_unlock(&user->lock);
> }
> key_user_put(user);
> - no_memory_1:
> +no_memory_1:
> key = ERR_PTR(-ENOMEM);
> goto error;
>
> - no_quota:
> +no_quota:
> spin_unlock(&user->lock);
> key_user_put(user);
> key = ERR_PTR(-EDQUOT);
> @@ -406,6 +429,11 @@ static int __key_instantiate_and_link(st
>
> /* can't instantiate twice */
> if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
> + /* consult the security modules */
> + ret = security_key_set_data(key, instkey, data, datalen);
> + if (ret < 0)
> + goto error;
> +
> /* instantiate the key */
> ret = key->type->instantiate(key, data, datalen);

I assume you don't check key_permission here because it's in context
creating the key?

> @@ -427,6 +455,7 @@ static int __key_instantiate_and_link(st
> }
> }
>
> +error:
> up_write(&key_construction_sem);
>
> /* wake up anyone waiting for a key to be constructed */
> @@ -556,6 +585,8 @@ static void key_cleanup(void *data)
>
> key_check(key);
>
> + security_key_free(key);
> +
> /* deal with the user's key tracking and quota */
> if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> spin_lock(&key->user->lock);
> @@ -710,11 +741,14 @@ static inline key_ref_t __key_update(key
>
> down_write(&key->sem);
>
> - ret = key->type->update(key, payload, plen);
> + ret = security_key_set_data(key, NULL, payload, plen);
> + if (ret == 0) {
> + ret = key->type->update(key, payload, plen);
>
> - if (ret == 0)
> - /* updating a negative key instantiates it */
> - clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> + if (ret == 0)
> + /* updating a negative key instantiates it */
> + clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> + }
>
> up_write(&key->sem);
>
> @@ -848,11 +882,15 @@ int key_update(key_ref_t key_ref, const
> ret = -EOPNOTSUPP;
> if (key->type->update) {
> down_write(&key->sem);
> - ret = key->type->update(key, payload, plen);
>
> - if (ret == 0)
> - /* updating a negative key instantiates it */
> - clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> + ret = security_key_set_data(key, NULL, payload, plen);
> + if (ret == 0) {
> + ret = key->type->update(key, payload, plen);
> +
> + if (ret == 0)
> + /* updating a negative key instantiates it */
> + clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
> + }
>
> up_write(&key->sem);
> }
> diff -uNrp linux-2.6.14-rc3-keys/security/keys/keyctl.c linux-2.6.14-rc3-keys-lsm/security/keys/keyctl.c
> --- linux-2.6.14-rc3-keys/security/keys/keyctl.c 2005-10-03 10:48:43.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/keyctl.c 2005-10-05 16:25:58.000000000 +0100
> @@ -964,6 +964,82 @@ long keyctl_set_reqkey_keyring(int reqke
>
> /*****************************************************************************/
> /*
> + * apply security data to a key
> + */
> +long keyctl_set_security(key_serial_t id,
> + const char __user *name,
> + const void __user *data,
> + size_t dlen)
> +{
> + struct key *key;
> + key_ref_t key_ref;
> + long ret;
> +
> + ret = -EINVAL;
> + if (!name)
> + goto error;
> +
> + key_ref = lookup_user_key(NULL, id, 1, 1, 0);
> + if (IS_ERR(key_ref)) {
> + ret = PTR_ERR(key_ref);
> + goto error;
> + }
> +
> + key = key_ref_to_ptr(key_ref);
> +
> + /* make the changes with the locks held to prevent races */
> + ret = -EACCES;
> + down_write(&key->sem);
> +
> + /* if we're not the sysadmin, we can only change a key that we own */
> + if (capable(CAP_SYS_ADMIN) || key->uid == current->fsuid)
> + ret = security_key_set_security(key, name, data, dlen);

Are you sure this is right? Normally I'd expect users can _not_ set the
security labels of their own keys. But perhaps I've missed the point
of this one, could you give a use case?

> + up_write(&key->sem);
> + key_put(key);
> +error:
> + return ret;
> +
> +} /* end keyctl_set_security() */
> +
> +/*****************************************************************************/
> +/*
> + * retrieve security data from a key
> + */
> +long keyctl_get_security(key_serial_t id,
> + const char __user *name,
> + void __user *buffer,
> + size_t blen)
> +{
> + struct key *key;
> + key_ref_t key_ref;
> + long ret;
> +
> + ret = -EINVAL;
> + if (!name)
> + goto error;
> +
> + key_ref = lookup_user_key(NULL, id, 1, 1, 0);
> + if (IS_ERR(key_ref)) {
> + ret = PTR_ERR(key_ref);
> + goto error;
> + }
> +
> + key = key_ref_to_ptr(key_ref);
> +
> + /* make the changes with the locks held to prevent races */
> + down_read(&key->sem);
> + ret = security_key_get_security(key, name, buffer, blen);

This would be a whole lot easier if keys were available in keyfs ;-)
/me ducks and runs
Again, maybe I've lost you on the keyctl interface, but what's this for?
How will users benefit from read/write of the security label on a key?

> + up_read(&key->sem);
> +
> + key_put(key);
> +error:
> + return ret;
> +
> +} /* end keyctl_get_security() */
> +
> +/*****************************************************************************/
> +/*
> * the key control system call
> */
> asmlinkage long sys_keyctl(int option, unsigned long arg2, unsigned long arg3,
> @@ -1035,6 +1111,18 @@ asmlinkage long sys_keyctl(int option, u
> case KEYCTL_SET_REQKEY_KEYRING:
> return keyctl_set_reqkey_keyring(arg2);
>
> + case KEYCTL_SET_SECURITY:
> + return keyctl_set_security((key_serial_t) arg2,
> + (const char __user *) arg3,
> + (const void __user *) arg4,
> + (size_t) arg5);
> +
> + case KEYCTL_GET_SECURITY:
> + return keyctl_get_security((key_serial_t) arg2,
> + (const char __user *) arg3,
> + (void __user *) arg4,
> + (size_t) arg5);
> +
> default:
> return -EOPNOTSUPP;
> }

> diff -uNrp linux-2.6.14-rc3-keys/security/keys/permission.c linux-2.6.14-rc3-keys-lsm/security/keys/permission.c
> --- linux-2.6.14-rc3-keys/security/keys/permission.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.14-rc3-keys-lsm/security/keys/permission.c 2005-10-05 15:36:22.000000000 +0100
> @@ -0,0 +1,81 @@
> +/* permission.c: key permission determination
> + *
> + * Copyright (C) 2005 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells ([email protected])
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include "internal.h"
> +
> +/*****************************************************************************/
> +/*
> + * check to see whether permission is granted to use a key in the desired way,
> + * but permit the security modules to override
> + */
> +int key_task_permission(const key_ref_t key_ref,
> + struct task_struct *context,
> + key_perm_t perm)
> +{
> + struct key *key;
> + key_perm_t kperm;
> + int ret;
> +
> + /* let the security module have first say
> + * - it should return:
> + * +ve to grant access
> + * 0 to deny access
> + * -ve to fall back to normal permission checking
> + */
> + ret = security_key_permission(key_ref, context, perm);
> + if (ret >= 0)
> + return ret;

This is not right. Do normal permissions check first. Iff they pass,
then allow security module to check labels. And simply return 0 on
success and -ERR on error.

> + /* normal permissions checking */
> + key = key_ref_to_ptr(key_ref);
> +
> + /* use the top 8-bits of permissions for keys the caller possesses */
> + if (is_key_possessed(key_ref)) {
> + kperm = key->perm >> 24;
> + goto use_these_perms;
> + }
> +
> + /* use the second 8-bits of permissions for keys the caller owns */
> + if (key->uid == context->fsuid) {
> + kperm = key->perm >> 16;
> + goto use_these_perms;
> + }
> +
> + /* use the third 8-bits of permissions for keys the caller has a group
> + * membership in common with */
> + if (key->gid != -1 && key->perm & KEY_GRP_ALL) {
> + if (key->gid == context->fsgid) {
> + kperm = key->perm >> 8;
> + goto use_these_perms;
> + }
> +
> + task_lock(context);
> + ret = groups_search(context->group_info, key->gid);
> + task_unlock(context);
> +
> + if (ret) {
> + kperm = key->perm >> 8;
> + goto use_these_perms;
> + }
> + }
> +
> + /* otherwise use the least-significant 8-bits */
> + kperm = key->perm;
> +
> +use_these_perms:
> + kperm = kperm & perm & KEY_ALL;
> +
> + return kperm == perm;
> +
> +} /* end key_task_permission() */
> +
> +EXPORT_SYMBOL(key_task_permission);

EXPORT_SYMBOL_GPL looks more appropriate here.

thanks,
-chris

2005-10-06 08:04:04

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Wed, 5 Oct 2005, Chris Wright wrote:

> What case causes context != current?

Indeed, this is critical: we always need to know which task initiated the
current action. If it's not current, then we need the calling task struct
passed into the security hook.

> > + /* do a final security check before publishing the key */
> > + ret = security_key_alloc(key);
>
> This may simply be allocating space for the label (and possibly labelling)
> not necessarily a security check.

Agree, in fact, I think we should always aim to keep housekeeping hooks
separate from access control hooks.

Access checks seem to be usually done before this point via
lookup_user_key(), which is ideal.

> > - error:
> > + /* let the security module know the key has been published */
> > + security_key_post_alloc(key);
>
> This is odd, esp since nothing could have failed between alloc and
> publish. Only state change is serial number. Would you expect the
> security module to update a label based on serial number?

I don't think SELinux would care about this yet. If so, the hook can be
added later.

> > + /* if we're not the sysadmin, we can only change a key that we own */
> > + if (capable(CAP_SYS_ADMIN) || key->uid == current->fsuid)
> > + ret = security_key_set_security(key, name, data, dlen);
>
> Are you sure this is right? Normally I'd expect users can _not_ set the
> security labels of their own keys. But perhaps I've missed the point
> of this one, could you give a use case?

I think this is like xattrs on files, where the user can set and view
security attributes.

In any case, I don't see why you'd use a DAC check here at all, as this is
a complete passthrough to the security module.

key_get_security() has no DAC check.

> This would be a whole lot easier if keys were available in keyfs ;-)

Yes, then standard setxattr() getxattr() syscalls could be used, and we
can avoid two new multiplexed syscalls.

David, admit it, this key stuff is all really a filesystem :-)

- James
--
James Morris
<[email protected]>

2005-10-06 08:38:49

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

I think this looks ok from an SELinux point of view if keys are treated as
opaque objects, i.e. like files.

We could do something like create a new object class (kernkey or
something) and implement SELinux permissions for the class such as read,
write, search, link, setattr and getattr. Your KEY_VIEW perm could be
translated to SELinux getattr.

More thought needs to go into whether we need to implement an SELinux
create permission (and add hooks into the code), for control over whether
a process can create an anonymous keyring.

I'm not sure if we need user-level labeling of keys via the set & get
security ops, although LSPP may require some form of get_security. If we
don't need to manually set security attributes but still view them, they
could be displayed via /proc/keys rather than implementing a separate
multiplexed syscall.

It seems that there are no LSM checks for the following:

keyctl_chown_key()
keyctl_setperm_key()
keyctl_set_reqkey_keyring()

keyctl_join_session_keyring() [only if we add a 'create' perm]

All users of key_permission() need to propagate the error code from the
LSM back to the user.

- James
--
James Morris
<[email protected]>

2005-10-06 10:30:39

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

Chris Wright <[email protected]> wrote:

> So the hooks you added control search/read/write via permission

Actually, that's search/read/write/view/link permissions.

> ->instantiate

Either must be creating the key or must have a request-key-authorisation key
for the key being instantiated and Write permission on that key.

> ->duplicate

Need to be able to create a new key. Don't need to be able to Read the
original key because you're not actually trying to access it.

> ->update

Need Write permission on the key.

> ->match

Need Search permission on the key.

> ->describe

Need View permission on the key. I should probably make /proc/keys consult the
LSM too.

> ->read

Need Read permission on the key, or it must be Searchable from the accessing
process's keyrings.

> and then special case instantiate and update. Does this mean you expect the
> security module to inspect the key data and set a label based on data?

It seemed like a good idea to make it available, since I have it to hand.
Whether the security module should use it to impose a lable, probably not, I
suppose.

> Does duplicate need to update label to a new context, or should it get
> identical lable, and does it need a hook for that?

I think it should start off with identical labels, but the set_security keyctl
op can then be called to change that.

It might be better to implement what Kyle Moffett wants and make clonable key
handles that contain the security information, but still refer to the same
key.

> This move of key_ref_t handling is either unrelated or simple prep work,
> yes? Just clutters the patch.

The problem is that key_ref_t isn't available if CONFIG_KEYS is not defined,
but it's still referenced in security.h. Would it be reasonable to make all
the security_key_*() functions contingent on CONFIG_KEYS since they're only
called from the key management code? That would mean I wouldn't need to do
this.

> Again patch clutter, since this is just removing unused code AFAICT
> (which should definitely be removed). I mention only because it makes it
> harder to review since I don't know the key infrastructure as well as you.

I can split the permissions function out into a .c file in a preliminary
patch.

> Just curious, from quick look, appears that key_task_permission is
> basically always called with current. I found one (which appears to be
> a recursive call) that uses rka->context in search_process_keyrings.
> What case causes context != current?

What happens is this:

(1) Process A calls request_key().

(2) request_key() sees that A doesn't have the desired key yet, so it
creates:

(a) An uninstantiated key U of appropriate type and description.

(b) An authorisation key V that refers to U and notes that process A is
the context in which V should be instantiated, and from which key
requests may be satisfied.

(3) request_key() then forks and executes /sbin/request-key with a session
keyring containing auth key V.

(4) /sbin/request-key execs an appropriate program to instantiate key U.

(5) The program may want to access another key from A's context (say a
Kerberos TGT key). It just requests the appropriate key, and the keyring
search notes that the session keyring has in its bottom level auth key V.

This will permit it to then search the keyrings of process A with the
UID, GID, groups and security info of process A as if it was process A,
coming up with key W.

(6) The program then does what it must to get the security data for key U,
using key W as a reference (perhaps it contacts a Kerberos server using
the TGT) and then instantiates key U.

(7) Upon instantiating key U, auth key V is automatically revoked so that it
may not be used again.

(8) The program then exits 0 and request_key() deletes key V and returns key
U to the caller.

This also extends further. If key W (step 5 above) didn't exist, another auth
key (X) will be created and another copy of /sbin/request-key spawned; but the
context specified by key X will still be process A.

The problem I've tried to solve is that I can't just attach process A's
keyrings to /sbin/request-key at the appropriate places because (a) execve
will discard two of them, and (b) it requires the same UID/GID/Groups all the
way through.

At some point, I will have to make it so that I don't have to use
/sbin/request-key, but can instead request an already running daemon assume
the context from an auth key specified to it, say by passing the key serial
number over a socket.

> > +#include <linux/security.h>
>
> This looks unnecessary. I'd include it where it's used.

I can do that.

> > + /* do a final security check before publishing the key */
> > + ret = security_key_alloc(key);
>
> This may simply be allocating space for the label (and possibly labelling)
> not necessarily a security check.

I don't know that. The LSM can always refuse permission to allocate a key if
it wants to.

> > + security_key_post_alloc(key);
>
> This is odd, esp since nothing could have failed between alloc and
> publish. Only state change is serial number. Would you expect the
> security module to update a label based on serial number?

I was thinking that it may want to be able to log it for auditing purposes.

> > ret = key->type->instantiate(key, data, datalen);
>
> I assume you don't check key_permission here because it's in context
> creating the key?

The caller has to do that because it's done differently depending on how it's
invoked. It may be called either by direct key creation (no permissions
required, only quota and keyring Write), or by later instantiation (requires
auth key and Write permission).

> > + /* if we're not the sysadmin, we can only change a key that we own */
> > + if (capable(CAP_SYS_ADMIN) || key->uid == current->fsuid)
> > + ret = security_key_set_security(key, name, data, dlen);
>
> Are you sure this is right? Normally I'd expect users can _not_ set the
> security labels of their own keys. But perhaps I've missed the point
> of this one, could you give a use case?

I haven't said that they can; I've said that they must own the key to be able
to request setting security data, or that they must be the superuser. I can
drop that check if you'd prefer and just leave it to the LSM.

> > + ret = security_key_get_security(key, name, buffer, blen);
>
> This would be a whole lot easier if keys were available in keyfs ;-)
> /me ducks and runs

It might be, but keyfs makes things so much bigger and more complicated.

> Again, maybe I've lost you on the keyctl interface, but what's this for?
> How will users benefit from read/write of the security label on a key?

I was thinking of xattr equivalent. I can drop one or both functions if you'd
prefer.

> > + ret = security_key_permission(key_ref, context, perm);
> > + if (ret >= 0)
> > + return ret;
>
> This is not right. Do normal permissions check first. Iff they pass,
> then allow security module to check labels. And simply return 0 on
> success and -ERR on error.

Don't you want to be able to override that completely? I'd've thought you
would have wanted to.

> > +EXPORT_SYMBOL(key_task_permission);
>
> EXPORT_SYMBOL_GPL looks more appropriate here.

Actually, it doesn't. The functions was publicly available in a header file
before.

David

2005-10-06 10:54:26

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> > What case causes context != current?
>
> Indeed, this is critical: we always need to know which task initiated the
> current action. If it's not current, then we need the calling task struct
> passed into the security hook.

Surely you know the calling task struct: it's current, but I can pass it in
anyway if you wish.

As I outlined in a previous email, this has to do with the way request_key()
works, and the need for the process actually instantiating the key to gain
access to the keyrings, ownership, group membership, etc. of the process that
created the key.

> > > + /* do a final security check before publishing the key */
> > > + ret = security_key_alloc(key);
> >
> > This may simply be allocating space for the label (and possibly labelling)
> > not necessarily a security check.
>
> Agree, in fact, I think we should always aim to keep housekeeping hooks
> separate from access control hooks.

What do you mean by separate? And this provides a chance for the LSM to deny
the creation of a key before it's published.

> Access checks seem to be usually done before this point via
> lookup_user_key(), which is ideal.

Eh? lookup_user_key()? That's not necessarily called before, not if you're
creating a key.

> > This is odd, esp since nothing could have failed between alloc and
> > publish. Only state change is serial number. Would you expect the
> > security module to update a label based on serial number?
>
> I don't think SELinux would care about this yet. If so, the hook can be
> added later.

Auditing?

> > Are you sure this is right? Normally I'd expect users can _not_ set the
> > security labels of their own keys. But perhaps I've missed the point
> > of this one, could you give a use case?
>
> I think this is like xattrs on files, where the user can set and view
> security attributes.

That's what I was thinking of.

> David, admit it, this key stuff is all really a filesystem :-)

Grrrr. Next time I see you I might have to toss you in the nearest river:-)

No, it isn't, except in that all filesystems are databases anyway, and the VFS
should be ripped out and replaced with Reiser4.

David

2005-10-06 11:06:21

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> I think this looks ok from an SELinux point of view if keys are treated as
> opaque objects, i.e. like files.

I'll make some changes based on the suggestions I've received. Those who
request the return of keyfs can go boil their heads.

> We could do something like create a new object class (kernkey or
> something) and implement SELinux permissions for the class such as read,
> write, search, link, setattr and getattr. Your KEY_VIEW perm could be
> translated to SELinux getattr.

Should I expand the permissions mask to include a setattr?

> More thought needs to go into whether we need to implement an SELinux
> create permission (and add hooks into the code), for control over whether
> a process can create an anonymous keyring.

That's not really a per-key type of thing.

> I'm not sure if we need user-level labeling of keys via the set & get
> security ops, although LSPP may require some form of get_security. If we
> don't need to manually set security attributes but still view them, they
> could be displayed via /proc/keys rather than implementing a separate
> multiplexed syscall.

Would it be worth me adding a key type op by which a security module can ask
the type its opinion (or by which key_alloc() can ask the type to give the
security module an earful)?

> keyctl_chown_key()
> keyctl_setperm_key()

Okay.

> keyctl_set_reqkey_keyring()

Should this really be securified? It merely controls the default destination
for a key created by request_key(), and is limited to the keyrings the process
is subscribed to in any case.

> keyctl_join_session_keyring() [only if we add a 'create' perm]

This does need a security hook, at least for joining an existing session.

I wonder if I should treat named sessions on a per-user basis and whether I
should separate them from keyrings, so that session names refer to keyrings
and have their own permissions and security, but aren't those keyrings. This
latter bit is the big stumbling block that I had with the clone-handle
functionality that Kyle Moffett woulkd like.

> All users of key_permission() need to propagate the error code from the
> LSM back to the user.

Really? Why?

Note that the fact that key_permission() fails for a key is sometimes ignored,
such as when I'm doing a search and one potentially matching key fails, but a
subsequent matching key passes.

David

2005-10-06 14:25:50

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Thu, 6 Oct 2005, David Howells wrote:

> James Morris <[email protected]> wrote:
>
> > I think this looks ok from an SELinux point of view if keys are treated as
> > opaque objects, i.e. like files.
>
> I'll make some changes based on the suggestions I've received. Those who
> request the return of keyfs can go boil their heads.

You know, I was thinking, ext3 could be much more compact if it was just a
set of custom syscalls and had no VFS representation.

What about a per process /proc/pid/keys, which contains keyrings and keys,
which can be opened, closed, use xattrs for any special access control
etc. ?

> > We could do something like create a new object class (kernkey or
> > something) and implement SELinux permissions for the class such as read,
> > write, search, link, setattr and getattr. Your KEY_VIEW perm could be
> > translated to SELinux getattr.
>
> Should I expand the permissions mask to include a setattr?

Possibly for setperm and chown.

> > I'm not sure if we need user-level labeling of keys via the set & get
> > security ops, although LSPP may require some form of get_security. If we
> > don't need to manually set security attributes but still view them, they
> > could be displayed via /proc/keys rather than implementing a separate
> > multiplexed syscall.
>
> Would it be worth me adding a key type op by which a security module can ask
> the type its opinion (or by which key_alloc() can ask the type to give the
> security module an earful)?

Well, SELinux is the only significant LSM in the tree and I don't think it
needs to set the labels. So, no.

> > keyctl_chown_key()
> > keyctl_setperm_key()
>
> Okay.
>
> > keyctl_set_reqkey_keyring()
>
> Should this really be securified? It merely controls the default destination
> for a key created by request_key(), and is limited to the keyrings the process
> is subscribed to in any case.

Ok, if needed, it can be added later.

> > All users of key_permission() need to propagate the error code from the
> > LSM back to the user.
>
> Really? Why?

Because the LSM has final say on the error code returned to the caller.
If the LSM runs out of memory, for example, it's silly to return -EACCES.

> Note that the fact that key_permission() fails for a key is sometimes ignored,
> such as when I'm doing a search and one potentially matching key fails, but a
> subsequent matching key passes.

Ok, that sounds like an internal issue to be resolved, ensuring that if
you are returning to the caller, the LSM's error code is returned.

- James
--
James Morris
<[email protected]>

2005-10-06 15:04:53

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Thu, 6 Oct 2005, David Howells wrote:

> > Agree, in fact, I think we should always aim to keep housekeeping hooks
> > separate from access control hooks.
>
> What do you mean by separate? And this provides a chance for the LSM to deny
> the creation of a key before it's published.

Separate in terms of providing clear semantics in the API, so that you
know a hook is either used for housekeeping (allocation, deallocation etc)
or for access control. But this is only an aim, an if it makes sense to
combine housekeeping and access control functions in some specific
instance, then so be it.

> > Access checks seem to be usually done before this point via
> > lookup_user_key(), which is ideal.
>
> Eh? lookup_user_key()? That's not necessarily called before, not if you're
> creating a key.

I thought this was generally called before key operations.

For example, sys_add_key() calls it with KEY_WRITE against the destination
keyring.

> > > This is odd, esp since nothing could have failed between alloc and
> > > publish. Only state change is serial number. Would you expect the
> > > security module to update a label based on serial number?
> >
> > I don't think SELinux would care about this yet. If so, the hook can be
> > added later.
>
> Auditing?

SELinux does not audit object creation, it will sometimes use a _post hook
to update its internal state or perform the access control check for
creating the object.

- James
--
James Morris
<[email protected]>

2005-10-06 15:11:51

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> What about a per process /proc/pid/keys, which contains keyrings and keys,
> which can be opened, closed, use xattrs for any special access control
> etc. ?

Not a good idea; especially not in /proc. Why are people fixated on using the
VFS interface when it doesn't fit? Making things into filesystems isn't
necessarily a good thing.

However, /proc/pid/xxx files for the ID of each keyring would be a good idea;
I did have a patch to do that once, but it seems have got lost.

> > Should I expand the permissions mask to include a setattr?
>
> Possibly for setperm and chown.

For setperm?

> > > All users of key_permission() need to propagate the error code from the
> > > LSM back to the user.
> >
> > Really? Why?
>
> Because the LSM has final say on the error code returned to the caller.
> If the LSM runs out of memory, for example, it's silly to return -EACCES.
>
> > Note that the fact that key_permission() fails for a key is sometimes
> > ignored, such as when I'm doing a search and one potentially matching key
> > fails, but a subsequent matching key passes.
>
> Ok, that sounds like an internal issue to be resolved, ensuring that if
> you are returning to the caller, the LSM's error code is returned.

But which LSM error code?

Let me explain what I mean:

(1) When the key management code searches for a key (keyring_search_aux) it
firstly calls key_permission(SEARCH) on the keyring it's starting with,
if this denies permission, it doesn't search further.

(2) It considers all the non-keyring keys within that keyring and, if any key
matches the criteria specified, calls key_permission(SEARCH) on it to see
if the key is allowed to be found. If it is, that key is returned; if
not, the search continues, and the error code is retained if of higher
priority than the one currently set.

(3) It then considers all the keyring-type keys in the keyring it's currently
searching. It calls key_permission(SEARCH) on each keyring, and if this
grants permission, it recurses, executing steps (2) and (3) on that
keyring.

The process stops immediately a valid key is found with permission granted to
use it. Any error from a previous match attempt is discarded and the key is
returned.

When search_process_keyrings() is invoked, it performs the following searches
until one succeeds:

(1) If extant, the process's thread keyring is searched.

(2) If extant, the process's process keyring is searched.

(3) The process's session keyring is searched.

(4) If the process has a request_key() authorisation key in its session
keyring then:

(a) If extant, the calling process's thread keyring is searched.

(b) If extant, the calling process's process keyring is searched.

(c) The calling process's session keyring is searched.

The moment one succeeds, all pending errors are discarded and the found key is
returned.

Only if all these fail does the whole thing fail with the highest priority
error. Note that several errors may have come from LSM.

The error priority should be:

EKEYREVOKED > EKEYEXPIRED > ENOKEY

EACCES/EPERM should only really be returned on a direct keyring search where
the basal keyring doesn't have Search permission.

The fact that I couldn't find a key because I didn't have permission to find
it probably shouldn't cause an immediate abort of the search with an error, or
any error other than ENOKEY/EKEYREVOKED/EKEYEXPIRED, unless I'm not permitted
to request a new key.

David

2005-10-06 15:18:23

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> > > Access checks seem to be usually done before this point via
> > > lookup_user_key(), which is ideal.
> >
> > Eh? lookup_user_key()? That's not necessarily called before, not if you're
> > creating a key.
>
> I thought this was generally called before key operations.
>
> For example, sys_add_key() calls it with KEY_WRITE against the destination
> keyring.

Yes, but not in regard to the new key, which is what I thought you were
implying.

Besides, it's logically two operations: create key and link key to
keyring. The reason they have to be combined is that the key would be
immediately destroyed if it wasn't attached to a keyring.

The permissions check done on the keyring merely assures that the keyring can
be modified, not that a new key may or may not actually be created.

Maybe we're talking at cross-purposes here.

> > > I don't think SELinux would care about this yet. If so, the hook can be
> > > added later.
> >
> > Auditing?
>
> SELinux does not audit object creation, it will sometimes use a _post hook
> to update its internal state or perform the access control check for
> creating the object.

I meant the auditing service. Doesn't that use the security module hooks?

David

2005-10-06 16:03:00

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Thu, 6 Oct 2005, David Howells wrote:

> > For example, sys_add_key() calls it with KEY_WRITE against the destination
> > keyring.
>
> Yes, but not in regard to the new key, which is what I thought you were
> implying.
>
> Besides, it's logically two operations: create key and link key to
> keyring. The reason they have to be combined is that the key would be
> immediately destroyed if it wasn't attached to a keyring.

I had assumed that you didn't want a permission check just for creating a
key (which is a fairly abstract and inert thing if you don't do anything
with it), and were only wanting to peform a check when linking.

> The permissions check done on the keyring merely assures that the keyring can
> be modified, not that a new key may or may not actually be created.

Ok, time to add KEY_CREATE?

> > > > I don't think SELinux would care about this yet. If so, the hook can be
> > > > added later.
> > >
> > > Auditing?
> >
> > SELinux does not audit object creation, it will sometimes use a _post hook
> > to update its internal state or perform the access control check for
> > creating the object.
>
> I meant the auditing service. Doesn't that use the security module hooks?

LSM is supposed to be about access control only, although SELinux and
Audit are becoming more intimate as time passes.

[added Steve Grubb to the cc list, who is looking at LSPP audit
requirements]

- James
--
James Morris
<[email protected]>

2005-10-06 16:14:17

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Thu, 6 Oct 2005, David Howells wrote:

> > > Should I expand the permissions mask to include a setattr?
> >
> > Possibly for setperm and chown.
>
> For setperm?

It changes an attribute of a key, for which you have DAC checks, therefore
you could assume that we'd also want MAC checks.

> > > > All users of key_permission() need to propagate the error code from the
> > > > LSM back to the user.
> > >
> > > Really? Why?
> >
> > Because the LSM has final say on the error code returned to the caller.
> > If the LSM runs out of memory, for example, it's silly to return -EACCES.
> >
> > > Note that the fact that key_permission() fails for a key is sometimes
> > > ignored, such as when I'm doing a search and one potentially matching key
> > > fails, but a subsequent matching key passes.
> >
> > Ok, that sounds like an internal issue to be resolved, ensuring that if
> > you are returning to the caller, the LSM's error code is returned.
>
> But which LSM error code?
>
> Let me explain what I mean:

[snipped]

Thanks for the detailed explanation.

Not sure yet how we'll avoid generating spurious SELinux denial messages.

> Only if all these fail does the whole thing fail with the highest priority
> error. Note that several errors may have come from LSM.
>
> The error priority should be:
>
> EKEYREVOKED > EKEYEXPIRED > ENOKEY
>
> EACCES/EPERM should only really be returned on a direct keyring search where
> the basal keyring doesn't have Search permission.

Ok, if a failure of this nature is generated by an LSM, we need to return
the LSM's error code (e.g. ask the LSM for search permission and it
returns -ENOMEM).

> The fact that I couldn't find a key because I didn't have permission to find
> it probably shouldn't cause an immediate abort of the search with an error, or
> any error other than ENOKEY/EKEYREVOKED/EKEYEXPIRED, unless I'm not permitted
> to request a new key.

Ok.

- James
--
James Morris
<[email protected]>

2005-10-06 18:01:43

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

* David Howells ([email protected]) wrote:
> James Morris <[email protected]> wrote:
> > > What case causes context != current?
> >
> > Indeed, this is critical: we always need to know which task initiated the
> > current action. If it's not current, then we need the calling task struct
> > passed into the security hook.
>
> Surely you know the calling task struct: it's current, but I can pass it in
> anyway if you wish.
>
> As I outlined in a previous email, this has to do with the way request_key()
> works, and the need for the process actually instantiating the key to gain
> access to the keyrings, ownership, group membership, etc. of the process that
> created the key.

The security check is comparing key label to task label. If it's not
done 100% in current context, then task must be passed to get access
to proper label. So, for example, request-key is done by the special
privileged /sbin/request-key via usermodehelper on behalf of someone else.

> > > > + /* do a final security check before publishing the key */
> > > > + ret = security_key_alloc(key);
> > >
> > > This may simply be allocating space for the label (and possibly labelling)
> > > not necessarily a security check.
> >
> > Agree, in fact, I think we should always aim to keep housekeeping hooks
> > separate from access control hooks.
>
> What do you mean by separate? And this provides a chance for the LSM to deny
> the creation of a key before it's published.

Just remove the comment, and we'll all agree ;-)

> > Access checks seem to be usually done before this point via
> > lookup_user_key(), which is ideal.
>
> Eh? lookup_user_key()? That's not necessarily called before, not if you're
> creating a key.
>
> > > This is odd, esp since nothing could have failed between alloc and
> > > publish. Only state change is serial number. Would you expect the
> > > security module to update a label based on serial number?
> >
> > I don't think SELinux would care about this yet. If so, the hook can be
> > added later.
>
> Auditing?

Hmm, suppose, but auditing is not the charter of LSM. So in this case,
the previous hook can audit key creation if needed. Just looking to
avoid hook proliferation if possible.

> > > Are you sure this is right? Normally I'd expect users can _not_ set the
> > > security labels of their own keys. But perhaps I've missed the point
> > > of this one, could you give a use case?
> >
> > I think this is like xattrs on files, where the user can set and view
> > security attributes.
>
> That's what I was thinking of.

I see, what would they used for?

thanks,
-chris

2005-10-06 23:10:31

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

* David Howells ([email protected]) wrote:
> Chris Wright <[email protected]> wrote:
>
> > So the hooks you added control search/read/write via permission
>
> Actually, that's search/read/write/view/link permissions.

Good point, thanks.

> > ->instantiate
>
> Either must be creating the key or must have a request-key-authorisation key
> for the key being instantiated and Write permission on that key.
>
> > ->duplicate
>
> Need to be able to create a new key. Don't need to be able to Read the
> original key because you're not actually trying to access it.
>
> > ->update
>
> Need Write permission on the key.
>
> > ->match
>
> Need Search permission on the key.
>
> > ->describe
>
> Need View permission on the key. I should probably make /proc/keys consult the
> LSM too.

BTW, /proc/keys should move, esp since it's a debugging interface.

> > ->read
>
> Need Read permission on the key, or it must be Searchable from the accessing
> process's keyrings.
>
> > and then special case instantiate and update. Does this mean you expect the
> > security module to inspect the key data and set a label based on data?
>
> It seemed like a good idea to make it available, since I have it to hand.
> Whether the security module should use it to impose a lable, probably not, I
> suppose.

It means the security modules have to be able to parse the data. I
think that'd be the rough analog to updating file label based on file
contents, right? And we definitely don't want that.

> > Does duplicate need to update label to a new context, or should it get
> > identical lable, and does it need a hook for that?
>
> I think it should start off with identical labels, but the set_security keyctl
> op can then be called to change that.
>
> It might be better to implement what Kyle Moffett wants and make clonable key
> handles that contain the security information, but still refer to the same
> key.

So this security information is COW?

> > This move of key_ref_t handling is either unrelated or simple prep work,
> > yes? Just clutters the patch.
>
> The problem is that key_ref_t isn't available if CONFIG_KEYS is not defined,
> but it's still referenced in security.h. Would it be reasonable to make all
> the security_key_*() functions contingent on CONFIG_KEYS since they're only
> called from the key management code? That would mean I wouldn't need to do
> this.

I see. I thought they already were conditional on CONFIG_KEYS.

> > Again patch clutter, since this is just removing unused code AFAICT
> > (which should definitely be removed). I mention only because it makes it
> > harder to review since I don't know the key infrastructure as well as you.
>
> I can split the permissions function out into a .c file in a preliminary
> patch.

Prolly be nice. I think I've deciphered the patch, so it was just a nitpick.

> > Just curious, from quick look, appears that key_task_permission is
> > basically always called with current. I found one (which appears to be
> > a recursive call) that uses rka->context in search_process_keyrings.
> > What case causes context != current?
>
> What happens is this:
>
> (1) Process A calls request_key().
>
> (2) request_key() sees that A doesn't have the desired key yet, so it
> creates:
>
> (a) An uninstantiated key U of appropriate type and description.
>
> (b) An authorisation key V that refers to U and notes that process A is
> the context in which V should be instantiated, and from which key
> requests may be satisfied.

So this is where 'rka->context = current' is established. And since
call_usermodehelper is called with wait flag set, you're sure current
won't go away...OK scratch that worry of mine.

> (3) request_key() then forks and executes /sbin/request-key with a session
> keyring containing auth key V.
>
> (4) /sbin/request-key execs an appropriate program to instantiate key U.
>
> (5) The program may want to access another key from A's context (say a
> Kerberos TGT key). It just requests the appropriate key, and the keyring
> search notes that the session keyring has in its bottom level auth key V.
>
> This will permit it to then search the keyrings of process A with the
> UID, GID, groups and security info of process A as if it was process A,
> coming up with key W.
>
> (6) The program then does what it must to get the security data for key U,
> using key W as a reference (perhaps it contacts a Kerberos server using
> the TGT) and then instantiates key U.
>
> (7) Upon instantiating key U, auth key V is automatically revoked so that it
> may not be used again.
>
> (8) The program then exits 0 and request_key() deletes key V and returns key
> U to the caller.
>
> This also extends further. If key W (step 5 above) didn't exist, another auth
> key (X) will be created and another copy of /sbin/request-key spawned; but the
> context specified by key X will still be process A.

Ah, I saw that code and didn't grok why that bit was needed, thanks.

> The problem I've tried to solve is that I can't just attach process A's
> keyrings to /sbin/request-key at the appropriate places because (a) execve
> will discard two of them, and (b) it requires the same UID/GID/Groups all the
> way through.
>
> At some point, I will have to make it so that I don't have to use
> /sbin/request-key, but can instead request an already running daemon assume
> the context from an auth key specified to it, say by passing the key serial
> number over a socket.

I can see the appeal, but actually current architecture makes it easier
to do checks against the caller that initiated the request.

> > > +#include <linux/security.h>
> >
> > This looks unnecessary. I'd include it where it's used.
>
> I can do that.
>
> > > + /* do a final security check before publishing the key */
> > > + ret = security_key_alloc(key);
> >
> > This may simply be allocating space for the label (and possibly labelling)
> > not necessarily a security check.
>
> I don't know that. The LSM can always refuse permission to allocate a key if
> it wants to.

Typically this type of hook is used for reserving label space. Removing
the comment is sufficient AFAIC.

> > > + security_key_post_alloc(key);
> >
> > This is odd, esp since nothing could have failed between alloc and
> > publish. Only state change is serial number. Would you expect the
> > security module to update a label based on serial number?
>
> I was thinking that it may want to be able to log it for auditing purposes.

Perhaps we could just consider that later, and focus on the access
control for now.

> > > ret = key->type->instantiate(key, data, datalen);
> >
> > I assume you don't check key_permission here because it's in context
> > creating the key?
>
> The caller has to do that because it's done differently depending on how it's
> invoked. It may be called either by direct key creation (no permissions
> required, only quota and keyring Write), or by later instantiation (requires
> auth key and Write permission).

Yes, thanks, I missed the rka instantiation at first.

> > > + /* if we're not the sysadmin, we can only change a key that we own */
> > > + if (capable(CAP_SYS_ADMIN) || key->uid == current->fsuid)
> > > + ret = security_key_set_security(key, name, data, dlen);
> >
> > Are you sure this is right? Normally I'd expect users can _not_ set the
> > security labels of their own keys. But perhaps I've missed the point
> > of this one, could you give a use case?
>
> I haven't said that they can; I've said that they must own the key to be able
> to request setting security data, or that they must be the superuser. I can
> drop that check if you'd prefer and just leave it to the LSM.

I simply don't see the point. I'd expect policy to mandate key labels,
not users.

> > > + ret = security_key_get_security(key, name, buffer, blen);
> >
> > This would be a whole lot easier if keys were available in keyfs ;-)
> > /me ducks and runs
>
> It might be, but keyfs makes things so much bigger and more complicated.
>
> > Again, maybe I've lost you on the keyctl interface, but what's this for?
> > How will users benefit from read/write of the security label on a key?
>
> I was thinking of xattr equivalent. I can drop one or both functions if you'd
> prefer.

It's more that I don't understand the use.

> > > + ret = security_key_permission(key_ref, context, perm);
> > > + if (ret >= 0)
> > > + return ret;
> >
> > This is not right. Do normal permissions check first. Iff they pass,
> > then allow security module to check labels. And simply return 0 on
> > success and -ERR on error.
>
> Don't you want to be able to override that completely? I'd've thought you
> would have wanted to.

Heh, yes seems logical, but we actually want the basic DAC permission
check to be done first, and only consult the security module if that
passes. This keeps the interface restrictive as opposed to authoritative,
which is required at present.

> > > +EXPORT_SYMBOL(key_task_permission);
> >
> > EXPORT_SYMBOL_GPL looks more appropriate here.
>
> Actually, it doesn't. The functions was publicly available in a header file
> before.

You're right, somehow I thought it was newly introduced.

thanks,
-chris

2005-10-07 08:51:00

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> > The permissions check done on the keyring merely assures that the keyring
> > can be modified, not that a new key may or may not actually be created.
>
> Ok, time to add KEY_CREATE?

But to what? It is possible to request or create a key without linking it to
anything, at least for kernel services.

David

2005-10-07 09:03:56

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

James Morris <[email protected]> wrote:

> > > > Should I expand the permissions mask to include a setattr?
> > >
> > > Possibly for setperm and chown.
> >
> > For setperm?
>
> It changes an attribute of a key, for which you have DAC checks, therefore
> you could assume that we'd also want MAC checks.

Does it matter that you can take away your own permission to change the
permissions?

David

2005-10-07 09:10:15

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

Chris Wright <[email protected]> wrote:

> The security check is comparing key label to task label. If it's not
> done 100% in current context, then task must be passed to get access
> to proper label. So, for example, request-key is done by the special
> privileged /sbin/request-key via usermodehelper on behalf of someone else.

Which task(s)? Both the one doing the check, and the one on whose behalf the
check is done?

> > Auditing?
>
> Hmm, suppose, but auditing is not the charter of LSM. So in this case,
> the previous hook can audit key creation if needed. Just looking to
> avoid hook proliferation if possible.

But you don't know the key serial number at that point, hence why I added the
second hook. I'll drop the second. I can always bring it back later.

> > That's what I was thinking of.
>
> I see, what would they used for?

I don't know. As far as I know, setxattr and co can be used to set and
retrieve security data on files. I thought it would be desirable to have
similar for keys. If not, I can remove both calls/hooks for the time being.

David

2005-10-07 09:57:18

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

Chris Wright <[email protected]> wrote:

> BTW, /proc/keys should move, esp since it's a debugging interface.

Move where? Actually, it shouldn't exist, except that I need it for debugging.

> It means the security modules have to be able to parse the data. I
> think that'd be the rough analog to updating file label based on file
> contents, right? And we definitely don't want that.

Okay, I'll not do that then.

> So this security information is COW?

That's a good point. I need to add a duplicate hook so that the LSM can copy
or whatever the security information. Or maybe I should get rid of key
duplication entirely since it's not available to userspace.

> > The problem is that key_ref_t isn't available if CONFIG_KEYS is not defined,
> > but it's still referenced in security.h. Would it be reasonable to make all
> > the security_key_*() functions contingent on CONFIG_KEYS since they're only
> > called from the key management code? That would mean I wouldn't need to do
> > this.
>
> I see. I thought they already were conditional on CONFIG_KEYS.

No... You get either one set which works or another set which is a bunch of
dummy functions, not neither. I should change that. I could ditch the
security_*() stubs entirely; they're just magic fluff to appease those who
feel queasy at the sight of #ifdefs in .c files.

> So this is where 'rka->context = current' is established. And since
> call_usermodehelper is called with wait flag set, you're sure current
> won't go away...OK scratch that worry of mine.

Even if that context could go away (say we made it request_key()
interruptible), the authorisation key would be revoked _first_ with the key
semaphore held, just to make sure there wouldn't be a race.

> Ah, I saw that code and didn't grok why that bit was needed, thanks.

I should wrap this outline up and stick it in a document somewhere.

> > At some point, I will have to make it so that I don't have to use
> > /sbin/request-key, but can instead request an already running daemon assume
> > the context from an auth key specified to it, say by passing the key serial
> > number over a socket.
>
> I can see the appeal, but actually current architecture makes it easier
> to do checks against the caller that initiated the request.

It's going to be necessary. I've had requests for this from Trond (NFSv4)
amongst others. We discussed it at OLS; it really slows things down to be
forking off new processes regularly, so it needs to be done. I thought I
should probably do the LSM patch first so I could then work out how to fit in
with that - so there may be more key security hooks coming.

> Typically this type of hook is used for reserving label space. Removing
> the comment is sufficient AFAIC.

Okay.

> Perhaps we could just consider that later, and focus on the access
> control for now.

Okay.

> > I haven't said that they can; I've said that they must own the key to be
> > able to request setting security data, or that they must be the
> > superuser. I can drop that check if you'd prefer and just leave it to the
> > LSM.
>
> I simply don't see the point. I'd expect policy to mandate key labels,
> not users.

Okay.

> > I was thinking of xattr equivalent. I can drop one or both functions if
> > you'd prefer.
>
> It's more that I don't understand the use.

I'll get rid of them then. I don't really know how the security thing is done
with SE Linux and suchlike, I suppose. They can always be added back later.

> > Don't you want to be able to override that completely? I'd've thought you
> > would have wanted to.
>
> Heh, yes seems logical, but we actually want the basic DAC permission
> check to be done first, and only consult the security module if that
> passes. This keeps the interface restrictive as opposed to authoritative,
> which is required at present.

Okay.

> You're right, somehow I thought it was newly introduced.

Well, you (or someone) did comment on the bit of the patch where I removed it
from the header file...

David

2005-10-07 13:03:33

by Stephen Smalley

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Fri, 2005-10-07 at 10:10 +0100, David Howells wrote:
> I don't know. As far as I know, setxattr and co can be used to set and
> retrieve security data on files. I thought it would be desirable to have
> similar for keys. If not, I can remove both calls/hooks for the time being.

I agree that enabling security-aware applications to separately label
specific keys is desirable, so I'd suggest retaining that support, not
dropping it. We ultimately need the same support for all kernel objects
(we lost it for sockets and System V IPC when the old SELinux API had to
be dropped).

--
Stephen Smalley
National Security Agency

2005-10-07 14:05:47

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

On Fri, 7 Oct 2005, David Howells wrote:

> James Morris <[email protected]> wrote:
>
> > > > > Should I expand the permissions mask to include a setattr?
> > > >
> > > > Possibly for setperm and chown.
> > >
> > > For setperm?
> >
> > It changes an attribute of a key, for which you have DAC checks, therefore
> > you could assume that we'd also want MAC checks.
>
> Does it matter that you can take away your own permission to change the
> permissions?

Not that I'm aware of.

- James
--
James Morris
<[email protected]>

2005-10-07 18:36:24

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

* David Howells ([email protected]) wrote:
> James Morris <[email protected]> wrote:
>
> > Ok, time to add KEY_CREATE?
>
> But to what? It is possible to request or create a key without linking it to
> anything, at least for kernel services.

I don't see the need. Write permission to keyring, yes, link permission
to link a key to a keyring, yes.

2005-10-07 18:51:32

[permalink] [raw]

Subject: Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management

* David Howells ([email protected]) wrote:
> Chris Wright <[email protected]> wrote:
>
> > The security check is comparing key label to task label. If it's not
> > done 100% in current context, then task must be passed to get access
> > to proper label. So, for example, request-key is done by the special
> > privileged /sbin/request-key via usermodehelper on behalf of someone else.
>
> Which task(s)? Both the one doing the check, and the one on whose behalf the
> check is done?

Sorry, the one who initiated the request, so rka->context in this case
(the one on whose behalf the check is done).

> > > Auditing?
> >
> > Hmm, suppose, but auditing is not the charter of LSM. So in this case,
> > the previous hook can audit key creation if needed. Just looking to
> > avoid hook proliferation if possible.
>
> But you don't know the key serial number at that point, hence why I added the
> second hook. I'll drop the second. I can always bring it back later.

You'll know the serial number any time an action is taken on the key,
and that's auditable. I agree, if we find a need it can certainly be
resurrected.

> > > That's what I was thinking of.
> >
> > I see, what would they used for?
>
> I don't know. As far as I know, setxattr and co can be used to set and
> retrieve security data on files. I thought it would be desirable to have
> similar for keys. If not, I can remove both calls/hooks for the time being.

Right, that makes sense considering that data is stored on disk and
likely needs to be initialized at some point by an admin or install.
Keys are transient and I'd expect policy engine to label them when
created. Looks like Stephen sees a use, so perhaps just dropping
surrounding conditional logic and letting module handle it same as
setxattr case.

thanks,
-chris

2005-10-07 19:36:53