Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management
From: Chris Wright
Date: Wed Oct 05 2005 - 16:11:16 EST
* David Howells (dhowells@xxxxxxxxxx) 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 (dhowells@xxxxxxxxxx)
> *
> * 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 (dhowells@xxxxxxxxxx)
> + *
> + * 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/