Skip to contentSkip to conversation

Keys: Split key permissions checking into a .c file

PATCH New David Howells 2 participants 13 messages Oct 7, 2005 – Nov 1, 2005

Subsystems (3)
lore
Share & tools
lore

David Howells

author maintainer
#0 lore raw

PATCH 1/1 Keys: Split key permissions checking into a .c file

The attached patch splits key permissions checking out of key-ui.h and moves it into a .c file. It's quite large and called quite a lot, and it's about to get bigger with the addition of LSM support for keys...

key_any_permission() is also discarded as it's no longer used.

---
warthog>diffstat -p1 keys-split-2614rc3.diff

 include/linux/key-ui.h     |   91 ++-------------------------------------------
 security/keys/Makefile     |    1 
 security/keys/permission.c |   70 ++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 86 deletions(-)
include/linux/key-ui.h +5 -86
diff -uNrp linux-2.6.14-rc3-keys/include/linux/key-ui.h linux-2.6.14-rc3-keys-split/include/linux/key-ui.h
@@ -38,97 +38,16 @@ struct keyring_list {
 	struct key	*keys[0];
 };
 
-
 /*
  * 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;
+extern int key_task_permission(const key_ref_t key_ref,
+			       struct task_struct *context,
+			       key_perm_t perm);
 
-	kperm = kperm & perm & KEY_ALL;
-
-	return kperm != 0;
-}
-
-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)
+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 == 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);
 }
 
 extern key_ref_t lookup_user_key(struct task_struct *context,
security/keys/Makefile +1 -0
diff -uNrp linux-2.6.14-rc3-keys/security/keys/Makefile linux-2.6.14-rc3-keys-split/security/keys/Makefile
@@ -6,6 +6,7 @@ obj-y := \
 	key.o \
 	keyring.o \
 	keyctl.o \
+	permission.o \
 	process_keys.o \
 	request_key.o \
 	request_key_auth.o \
security/keys/permission.c +70 -0

70 lines not on this page · raw message

author maintainer
#1 lore raw

PATCH 1/1 Keys: Possessor permissions should be additive

This patch makes the possessor permissions on a key additive with user/group/other permissions on the same key.

This permits extra rights to be granted to the possessor of a key without taking away any rights conferred by them owning the key or having common group membership.

This needs to be applied on top of the patch ensubjected:

[PATCH] Keys: Split key permissions checking into a .c file

---
warthog>diffstat -p1 keys-addperm-2614rc3.diff

 security/keys/permission.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
security/keys/permission.c +6 -6
diff -uNrp linux-2.6.14-rc3-keys-split/security/keys/permission.c linux-2.6.14-rc3-keys-addperm/security/keys/permission.c
@@ -27,12 +27,6 @@ int key_task_permission(const key_ref_t 
 
 	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;
@@ -61,6 +55,12 @@ int key_task_permission(const key_ref_t 
 	kperm = key->perm;
 
 use_these_perms:
+	/* use the top 8-bits of permissions for keys the caller possesses
+	 * - possessor permissions are additive with other permissions
+	 */
+	if (is_key_possessed(key_ref))
+		kperm |= key->perm >> 24;
+
 	kperm = kperm & perm & KEY_ALL;
 
 	return kperm == perm;
author maintainer
#2 lore raw

PATCH 1/1 Keys: Add LSM hooks for key management [try #2]

The attached patch adds LSM hooks for key management facilities. The notable changes are:

(1) The key struct now supports a security pointer for the use of security modules. This will permit key labelling and restrictions on which programs may access a key.

(2) Security modules get a chance to note (or abort) the allocation of a key.

(3) The key permission checking can now be enhanced by the security modules; the permissions check consults LSM if all other checks bear out.

(4) The key permissions checking functions now return an error code rather than a boolean value.

(5) An extra permission has been added to govern the modification of attributes (UID, GID, permissions).

Note that there isn't an LSM hook specifically for each keyctl() operation, but rather the permissions hook allows control of individual operations based on the permission request bits.

Key management access control through LSM is enabled by automatically if both CONFIG_KEYS and CONFIG_SECURITY are enabled.

This should be applied on top of the patch ensubjected:

[PATCH] Keys: Possessor permissions should be additive

---
warthog>diffstat -p1 keys-lsm-2614rc3-2.diff

 Documentation/keys.txt       |   22 +++++++-----
 include/linux/key-ui.h       |    3 +
 include/linux/key.h          |   13 +++++--
 include/linux/security.h     |   73 +++++++++++++++++++++++++++++++++++++++++++
 security/dummy.c             |   23 +++++++++++++
 security/keys/key.c          |   56 +++++++++++++++++++++++---------
 security/keys/keyctl.c       |   13 ++++---
 security/keys/keyring.c      |   21 +++++++-----
 security/keys/permission.c   |    7 +++-
 security/keys/process_keys.c |    9 ++---
 10 files changed, 191 insertions(+), 49 deletions(-)
Documentation/keys.txt +13 -9
diff -uNrp linux-2.6.14-rc3-keys-addperm/Documentation/keys.txt linux-2.6.14-rc3-keys-lsm/Documentation/keys.txt
@@ -196,7 +196,7 @@ KEY ACCESS PERMISSIONS
 
 Keys have an owner user ID, a group access ID, and a permissions mask. The mask
 has up to eight bits each for possessor, user, group and other access. Only
-five of each set of eight bits are defined. These permissions granted are:
+six of each set of eight bits are defined. These permissions granted are:
 
  (*) View
 
@@ -224,6 +224,10 @@ five of each set of eight bits are defin
      keyring to a key, a process must have Write permission on the keyring and
      Link permission on the key.
 
+ (*) Set Attribute
+
+     This permits a key's UID, GID and permissions mask to be changed.
+
 For changing the ownership, group ID or permissions mask, being the owner of
 the key or having the sysadmin capability is sufficient.
 
@@ -242,15 +246,15 @@ about the status of the key service:
      this way:
 
 	SERIAL   FLAGS  USAGE EXPY PERM     UID   GID   TYPE      DESCRIPTION: SUMMARY
-	00000001 I-----    39 perm 1f1f0000     0     0 keyring   _uid_ses.0: 1/4
-	00000002 I-----     2 perm 1f1f0000     0     0 keyring   _uid.0: empty
-	00000007 I-----     1 perm 1f1f0000     0     0 keyring   _pid.1: empty
-	0000018d I-----     1 perm 1f1f0000     0     0 keyring   _pid.412: empty
-	000004d2 I--Q--     1 perm 1f1f0000    32    -1 keyring   _uid.32: 1/4
-	000004d3 I--Q--     3 perm 1f1f0000    32    -1 keyring   _uid_ses.32: empty
+	00000001 I-----    39 perm 1f3f0000     0     0 keyring   _uid_ses.0: 1/4
+	00000002 I-----     2 perm 1f3f0000     0     0 keyring   _uid.0: empty
+	00000007 I-----     1 perm 1f3f0000     0     0 keyring   _pid.1: empty
+	0000018d I-----     1 perm 1f3f0000     0     0 keyring   _pid.412: empty
+	000004d2 I--Q--     1 perm 1f3f0000    32    -1 keyring   _uid.32: 1/4
+	000004d3 I--Q--     3 perm 1f3f0000    32    -1 keyring   _uid_ses.32: empty
 	00000892 I--QU-     1 perm 1f000000     0     0 user      metal:copper: 0
-	00000893 I--Q-N     1  35s 1f1f0000     0     0 user      metal:silver: 0
-	00000894 I--Q--     1  10h 001f0000     0     0 user      metal:gold: 0
+	00000893 I--Q-N     1  35s 1f3f0000     0     0 user      metal:silver: 0
+	00000894 I--Q--     1  10h 003f0000     0     0 user      metal:gold: 0
 
      The flags are:
 
include/linux/key.h +9 -4
diff -uNrp linux-2.6.14-rc3-keys-addperm/include/linux/key.h linux-2.6.14-rc3-keys-lsm/include/linux/key.h
@@ -40,28 +40,32 @@ struct key;
 #define KEY_POS_WRITE	0x04000000	/* possessor can update key payload / add link to keyring */
 #define KEY_POS_SEARCH	0x08000000	/* possessor can find a key in search / search a keyring */
 #define KEY_POS_LINK	0x10000000	/* possessor can create a link to a key/keyring */
-#define KEY_POS_ALL	0x1f000000
+#define KEY_POS_SETATTR	0x20000000	/* possessor can set key attributes */
+#define KEY_POS_ALL	0x3f000000
 
 #define KEY_USR_VIEW	0x00010000	/* user permissions... */
 #define KEY_USR_READ	0x00020000
 #define KEY_USR_WRITE	0x00040000
 #define KEY_USR_SEARCH	0x00080000
 #define KEY_USR_LINK	0x00100000
-#define KEY_USR_ALL	0x001f0000
+#define KEY_USR_SETATTR	0x00200000
+#define KEY_USR_ALL	0x003f0000
 
 #define KEY_GRP_VIEW	0x00000100	/* group permissions... */
 #define KEY_GRP_READ	0x00000200
 #define KEY_GRP_WRITE	0x00000400
 #define KEY_GRP_SEARCH	0x00000800
 #define KEY_GRP_LINK	0x00001000
-#define KEY_GRP_ALL	0x00001f00
+#define KEY_GRP_SETATTR	0x00002000
+#define KEY_GRP_ALL	0x00003f00
 
 #define KEY_OTH_VIEW	0x00000001	/* third party permissions... */
 #define KEY_OTH_READ	0x00000002
 #define KEY_OTH_WRITE	0x00000004
 #define KEY_OTH_SEARCH	0x00000008
 #define KEY_OTH_LINK	0x00000010
-#define KEY_OTH_ALL	0x0000001f
+#define KEY_OTH_SETATTR	0x00000020
+#define KEY_OTH_ALL	0x0000003f
 
 struct seq_file;
 struct user_struct;
@@ -119,6 +123,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;
include/linux/key-ui.h +2 -1
diff -uNrp linux-2.6.14-rc3-keys-addperm/include/linux/key-ui.h linux-2.6.14-rc3-keys-lsm/include/linux/key-ui.h
@@ -24,7 +24,8 @@ extern spinlock_t key_serial_lock;
 #define	KEY_WRITE	0x04	/* require permission to update / modify */
 #define	KEY_SEARCH	0x08	/* require permission to search (keyring) or find (key) */
 #define	KEY_LINK	0x10	/* require permission to link */
-#define	KEY_ALL		0x1f	/* all the above permissions */
+#define	KEY_SETATTR	0x20	/* require permission to change attributes */
+#define	KEY_ALL		0x3f	/* all the above permissions */
 
 /*
  * the keyring payload contains a list of the keys to which the keyring is
include/linux/security.h +73 -0

96 lines not on this page · raw message

security/dummy.c +23 -0
diff -uNrp linux-2.6.14-rc3-keys-addperm/security/dummy.c linux-2.6.14-rc3-keys-lsm/security/dummy.c
@@ -803,6 +803,23 @@ static int dummy_setprocattr(struct task
 	return -EINVAL;
 }
 
+#ifdef CONFIG_KEYS
+static inline int dummy_key_alloc(struct key *key)
+{
+	return 0;
+}
+
+static inline void dummy_key_free(struct key *key)
+{
+}
+
+static inline int dummy_key_permission(key_ref_t key_ref,
+				       struct task_struct *context,
+				       key_perm_t perm)
+{
+	return 0;
+}
+#endif /* CONFIG_KEYS */
 
 struct security_operations dummy_security_ops;
 
@@ -954,5 +971,11 @@ 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_KEYS
+	set_to_dummy_if_null(ops, key_alloc);
+	set_to_dummy_if_null(ops, key_free);
+	set_to_dummy_if_null(ops, key_permission);
+#endif	/* CONFIG_KEYS */
+
 }
 
security/keys/key.c +40 -16

145 lines not on this page · raw message

security/keys/keyctl.c +8 -5
diff -uNrp linux-2.6.14-rc3-keys-addperm/security/keys/keyctl.c linux-2.6.14-rc3-keys-lsm/security/keys/keyctl.c
@@ -624,8 +624,8 @@ long keyctl_keyring_search(key_serial_t 
 
 	/* link the resulting key to the destination keyring if we can */
 	if (dest_ref) {
-		ret = -EACCES;
-		if (!key_permission(key_ref, KEY_LINK))
+		ret = key_permission(key_ref, KEY_LINK);
+		if (ret < 0)
 			goto error6;
 
 		ret = key_link(key_ref_to_ptr(dest_ref), key_ref_to_ptr(key_ref));
@@ -676,8 +676,11 @@ long keyctl_read_key(key_serial_t keyid,
 	key = key_ref_to_ptr(key_ref);
 
 	/* see if we can read it directly */
-	if (key_permission(key_ref, KEY_READ))
+	ret = key_permission(key_ref, KEY_READ);
+	if (ret == 0)
 		goto can_read_key;
+	if (ret != -EACCES)
+		goto error;
 
 	/* we can't; see if it's searchable from this process's keyrings
 	 * - we automatically take account of the fact that it may be
@@ -726,7 +729,7 @@ long keyctl_chown_key(key_serial_t id, u
 	if (uid == (uid_t) -1 && gid == (gid_t) -1)
 		goto error;
 
-	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
+	key_ref = lookup_user_key(NULL, id, 1, 1, KEY_SETATTR);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
@@ -786,7 +789,7 @@ long keyctl_setperm_key(key_serial_t id,
 	if (perm & ~(KEY_POS_ALL | KEY_USR_ALL | KEY_GRP_ALL | KEY_OTH_ALL))
 		goto error;
 
-	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
+	key_ref = lookup_user_key(NULL, id, 1, 1, KEY_SETATTR);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
security/keys/keyring.c +13 -8
diff -uNrp linux-2.6.14-rc3-keys-addperm/security/keys/keyring.c linux-2.6.14-rc3-keys-lsm/security/keys/keyring.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/security.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
 #include <asm/uaccess.h>
@@ -309,7 +310,9 @@ struct key *keyring_alloc(const char *de
 	int ret;
 
 	keyring = key_alloc(&key_type_keyring, description,
-			    uid, gid, KEY_POS_ALL | KEY_USR_ALL, not_in_quota);
+			    uid, gid,
+			    (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL,
+			    not_in_quota);
 
 	if (!IS_ERR(keyring)) {
 		ret = key_instantiate_and_link(keyring, NULL, 0, dest, NULL);
@@ -359,9 +362,11 @@ key_ref_t keyring_search_aux(key_ref_t k
 	key_check(keyring);
 
 	/* top keyring must have search permission to begin the search */
-	key_ref = ERR_PTR(-EACCES);
-	if (!key_task_permission(keyring_ref, context, KEY_SEARCH))
+        err = key_task_permission(keyring_ref, context, KEY_SEARCH);
+	if (err < 0) {
+		key_ref = ERR_PTR(err);
 		goto error;
+	}
 
 	key_ref = ERR_PTR(-ENOTDIR);
 	if (keyring->type != &key_type_keyring)
@@ -402,8 +407,8 @@ descend:
 			continue;
 
 		/* key must have search permissions */
-		if (!key_task_permission(make_key_ref(key, possessed),
-					 context, KEY_SEARCH))
+		if (key_task_permission(make_key_ref(key, possessed),
+					context, KEY_SEARCH) < 0)
 			continue;
 
 		/* we set a different error code if we find a negative key */
@@ -430,7 +435,7 @@ ascend:
 			continue;
 
 		if (!key_task_permission(make_key_ref(key, possessed),
-					 context, KEY_SEARCH))
+					 context, KEY_SEARCH) < 0)
 			continue;
 
 		/* stack the current position */
@@ -521,7 +526,7 @@ key_ref_t __keyring_search_one(key_ref_t
 			    (!key->type->match ||
 			     key->type->match(key, description)) &&
 			    key_permission(make_key_ref(key, possessed),
-					   perm) &&
+					   perm) < 0 &&
 			    !test_bit(KEY_FLAG_REVOKED, &key->flags)
 			    )
 				goto found;
@@ -617,7 +622,7 @@ struct key *find_keyring_by_name(const c
 				continue;
 
 			if (!key_permission(make_key_ref(keyring, 0),
-					    KEY_SEARCH))
+					    KEY_SEARCH) < 0)
 				continue;
 
 			/* found a potential candidate, but we still need to
security/keys/permission.c +6 -1
diff -uNrp linux-2.6.14-rc3-keys-addperm/security/keys/permission.c linux-2.6.14-rc3-keys-lsm/security/keys/permission.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/security.h>
 #include "internal.h"
 
 /*****************************************************************************/
@@ -63,7 +64,11 @@ use_these_perms:
 
 	kperm = kperm & perm & KEY_ALL;
 
-	return kperm == perm;
+	if (kperm != perm)
+		return -EACCES;
+
+	/* let LSM be the final arbiter */
+	return security_key_permission(key_ref, context, perm);
 
 } /* end key_task_permission() */
 
security/keys/process_keys.c +4 -5
diff -uNrp linux-2.6.14-rc3-keys-addperm/security/keys/process_keys.c linux-2.6.14-rc3-keys-lsm/security/keys/process_keys.c
@@ -39,7 +39,7 @@ struct key root_user_keyring = {
 	.type		= &key_type_keyring,
 	.user		= &root_key_user,
 	.sem		= __RWSEM_INITIALIZER(root_user_keyring.sem),
-	.perm		= KEY_POS_ALL | KEY_USR_ALL,
+	.perm		= (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL,
 	.flags		= 1 << KEY_FLAG_INSTANTIATED,
 	.description	= "_uid.0",
 #ifdef KEY_DEBUGGING
@@ -54,7 +54,7 @@ struct key root_session_keyring = {
 	.type		= &key_type_keyring,
 	.user		= &root_key_user,
 	.sem		= __RWSEM_INITIALIZER(root_session_keyring.sem),
-	.perm		= KEY_POS_ALL | KEY_USR_ALL,
+	.perm		= (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL,
 	.flags		= 1 << KEY_FLAG_INSTANTIATED,
 	.description	= "_uid_ses.0",
 #ifdef KEY_DEBUGGING
@@ -666,9 +666,8 @@ key_ref_t lookup_user_key(struct task_st
 		goto invalid_key;
 
 	/* check the permissions */
-	ret = -EACCES;
-
-	if (!key_task_permission(key_ref, context, perm))
+	ret = key_task_permission(key_ref, context, perm);
+	if (ret < 0)
 		goto invalid_key;
 
 error:
author maintainer
#3 lore raw

PATCH 1/1 Keys: Remove key duplication

The attached patch removes the key duplication stuff since there's nothing that uses it, no way to get at it and it's awkward to deal with for LSM purposes.

This patch should be applied on top of the one ensubjected:

[PATCH] Keys: Add LSM hooks for key management [try #2]

---
warthog>diffstat -p1 keys-nodup-2614rc3.diff

 Documentation/keys.txt       |   18 ------------
 include/keys/user-type.h     |    1 
 include/linux/key.h          |    8 -----
 security/keys/key.c          |   56 ++-----------------------------------
 security/keys/keyring.c      |   64 -------------------------------------------
 security/keys/user_defined.c |   32 ---------------------
 6 files changed, 3 insertions(+), 176 deletions(-)
Documentation/keys.txt +0 -18
diff -uNrp linux-2.6.14-rc3-keys-lsm/Documentation/keys.txt linux-2.6.14-rc3-keys-nodup/Documentation/keys.txt
@@ -860,24 +860,6 @@ The structure has a number of fields, so
      It is safe to sleep in this method.
 
 
- (*) int (*duplicate)(struct key *key, const struct key *source);
-
-     If this type of key can be duplicated, then this method should be
-     provided. It is called to copy the payload attached to the source into the
-     new key. The data length on the new key will have been updated and the
-     quota adjusted already.
-
-     This method will be called with the source key's semaphore read-locked to
-     prevent its payload from being changed, thus RCU constraints need not be
-     applied to the source key.
-
-     This method does not have to lock the destination key in order to attach a
-     payload. The fact that KEY_FLAG_INSTANTIATED is not set in key->flags
-     prevents anything else from gaining access to the key.
-
-     It is safe to sleep in this method.
-
-
  (*) int (*update)(struct key *key, const void *data, size_t datalen);
 
      If this type of key can be updated, then this method should be provided.
include/keys/user-type.h +0 -1
diff -uNrp linux-2.6.14-rc3-keys-lsm/include/keys/user-type.h linux-2.6.14-rc3-keys-nodup/include/keys/user-type.h
@@ -35,7 +35,6 @@ struct user_key_payload {
 extern struct key_type key_type_user;
 
 extern int user_instantiate(struct key *key, const void *data, size_t datalen);
-extern int user_duplicate(struct key *key, const struct key *source);
 extern int user_update(struct key *key, const void *data, size_t datalen);
 extern int user_match(const struct key *key, const void *criterion);
 extern void user_destroy(struct key *key);
include/linux/key.h +0 -8
diff -uNrp linux-2.6.14-rc3-keys-lsm/include/linux/key.h linux-2.6.14-rc3-keys-nodup/include/linux/key.h
@@ -193,14 +193,6 @@ struct key_type {
 	 */
 	int (*instantiate)(struct key *key, const void *data, size_t datalen);
 
-	/* duplicate a key of this type (optional)
-	 * - the source key will be locked against change
-	 * - the new description will be attached
-	 * - the quota will have been adjusted automatically from
-	 *   source->quotalen
-	 */
-	int (*duplicate)(struct key *key, const struct key *source);
-
 	/* update a key of this type (optional)
 	 * - this method should call key_payload_reserve() to recalculate the
 	 *   quota consumption
security/keys/key.c +3 -53

68 lines not on this page · raw message

security/keys/keyring.c +0 -64

82 lines not on this page · raw message

security/keys/user_defined.c +0 -32
diff -uNrp linux-2.6.14-rc3-keys-lsm/security/keys/user_defined.c linux-2.6.14-rc3-keys-nodup/security/keys/user_defined.c
@@ -26,7 +26,6 @@
 struct key_type key_type_user = {
 	.name		= "user",
 	.instantiate	= user_instantiate,
-	.duplicate	= user_duplicate,
 	.update		= user_update,
 	.match		= user_match,
 	.destroy	= user_destroy,
@@ -73,37 +72,6 @@ EXPORT_SYMBOL(user_instantiate);
 
 /*****************************************************************************/
 /*
- * duplicate a user defined key
- * - both keys' semaphores are locked against further modification
- * - the new key cannot yet be accessed
- */
-int user_duplicate(struct key *key, const struct key *source)
-{
-	struct user_key_payload *upayload, *spayload;
-	int ret;
-
-	/* just copy the payload */
-	ret = -ENOMEM;
-	upayload = kmalloc(sizeof(*upayload) + source->datalen, GFP_KERNEL);
-	if (upayload) {
-		spayload = rcu_dereference(source->payload.data);
-		BUG_ON(source->datalen != spayload->datalen);
-
-		upayload->datalen = key->datalen = spayload->datalen;
-		memcpy(upayload->data, spayload->data, key->datalen);
-
-		key->payload.data = upayload;
-		ret = 0;
-	}
-
-	return ret;
-
-} /* end user_duplicate() */
-
-EXPORT_SYMBOL(user_duplicate);
-
-/*****************************************************************************/
-/*
  * dispose of the old data from an updated user defined key
  */
 static void user_update_rcu_disposal(struct rcu_head *rcu)
#4 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

In reply to David Howells

include/linux/security.h static inline void security_sk_free(stru · line 2834

Quoted 6 lines · * David Howells ([email protected]) wrote:
include/linux/security.h +6 -0
+static inline int security_key_permission(key_ref_t key_ref,
+					  struct task_struct *context,
+					  key_perm_t perm)
+{
+	return -1;
+}

No solid reason on that one, might as well be 0 for consistency.

Other than that, looks good. It's going to conflict with some pending changes I have, how shall we work that? I can put all of these in that queue?

thanks,
-chris

author maintainer
#5 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

Quoted 5 lines · Chris Wright

include/linux/security.h static inline void security_sk_free(stru · line 2834

Quoted 3 lines
include/linux/security.h +3 -0
+{
+	return -1;
+}

No solid reason on that one, might as well be 0 for consistency.

Grrr! That needs to be zero otherwise it'll deny everything by default. I'd fallen over that one and fixed it, but I must've forgotten to rediff the patch before submitting it.

David

#6 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

Quoted 11 lines · * David Howells ([email protected]) wrote:
Quoted 5 lines · Chris Wright

include/linux/security.h static inline void security_sk_free(stru · line 2834

Quoted 3 lines
include/linux/security.h +3 -0
+{
+	return -1;
+}

No solid reason on that one, might as well be 0 for consistency.

Grrr! That needs to be zero otherwise it'll deny everything by default. I'd fallen over that one and fixed it, but I must've forgotten to rediff the patch before submitting it.

I thought that too at first, which is why I flagged it at first. But I think it's actually not a real problem, because isn't that !CONFIG_KEYS? So, I think it's just cosmetic.

author maintainer
#7 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

Quoted 3 lines · Chris Wright

I thought that too at first, which is why I flagged it at first. But I think it's actually not a real problem, because isn't that !CONFIG_KEYS? So, I think it's just cosmetic.

It is a problem; at least I found it to be one. And no it isn't !CONFIG_KEYS.

David

author maintainer
#8 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

Hi Chris,

Quoted 5 lines · David Howells
Quoted 3 lines

I thought that too at first, which is why I flagged it at first. But I think it's actually not a real problem, because isn't that !CONFIG_KEYS? So, I think it's just cosmetic.

It is a problem; at least I found it to be one. And no it isn't !CONFIG_KEYS.

It's not one I'd fixed earlier. I fixed the same problem, but in the dummy routines. It's contingent on CONFIG_KEYS and !CONFIG_SECURITY. Would you be willing to whip up a patch to change it? I'd do it, but I'm currently logged in on my laptop over a 9600 baud modem connection.

Thanks!
David

#9 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

Quoted 9 lines · * David Howells ([email protected]) wrote:

Hi Chris,

Quoted 4 lines · David Howells
Quoted 3 lines

I thought that too at first, which is why I flagged it at first. But I think it's actually not a real problem, because isn't that !CONFIG_KEYS? So, I think it's just cosmetic.

Quoted 6 lines
Quoted 1 line

It is a problem; at least I found it to be one. And no it isn't !CONFIG_KEYS.

It's not one I'd fixed earlier. I fixed the same problem, but in the dummy routines. It's contingent on CONFIG_KEYS and !CONFIG_SECURITY. Would you be willing to whip up a patch to change it? I'd do it, but I'm currently logged in on my laptop over a 9600 baud modem connection.

No problem.

thanks,
-chris

#10 lore raw

Re: [Keyrings] [PATCH] Keys: Add LSM hooks for key management [try #2]

In reply to David Howells
Quoted 7 lines · * David Howells ([email protected]) wrote:
Quoted 3 lines · Chris Wright

I thought that too at first, which is why I flagged it at first. But I think it's actually not a real problem, because isn't that !CONFIG_KEYS? So, I think it's just cosmetic.

It is a problem; at least I found it to be one. And no it isn't !CONFIG_KEYS.

Ah, right, that's the other ifdef there.

#11 lore raw

PATCH 1/1 Keys: Add LSM hooks for key management [try #3]

In reply to David Howells

From: David Howells <[email protected]>

The attached patch adds LSM hooks for key management facilities. The notable changes are:

(1) The key struct now supports a security pointer for the use of security modules. This will permit key labelling and restrictions on which programs may access a key.

(2) Security modules get a chance to note (or abort) the allocation of a key.

(3) The key permission checking can now be enhanced by the security modules; the permissions check consults LSM if all other checks bear out.

(4) The key permissions checking functions now return an error code rather than a boolean value.

(5) An extra permission has been added to govern the modification of attributes (UID, GID, permissions).

Note that there isn't an LSM hook specifically for each keyctl() operation, but rather the permissions hook allows control of individual operations based on the permission request bits.

Key management access control through LSM is enabled by automatically if both CONFIG_KEYS and CONFIG_SECURITY are enabled.

This should be applied on top of the patch ensubjected:

[PATCH] Keys: Possessor permissions should be additive

---
 Documentation/keys.txt       |   22 +++++++-----
 include/linux/key-ui.h       |    3 +
 include/linux/key.h          |   13 +++++--
 include/linux/security.h     |   73 +++++++++++++++++++++++++++++++++++++++++++
 security/dummy.c             |   23 +++++++++++++
 security/keys/key.c          |   56 +++++++++++++++++++++++---------
 security/keys/keyctl.c       |   13 ++++---
 security/keys/keyring.c      |   21 +++++++-----
 security/keys/permission.c   |    7 +++-
 security/keys/process_keys.c |    9 ++---
 10 files changed, 191 insertions(+), 49 deletions(-)
Documentation/keys.txt +13 -9
Index: linus-2.6/Documentation/keys.txt
===================================================================
@@ -196,7 +196,7 @@ KEY ACCESS PERMISSIONS
 
 Keys have an owner user ID, a group access ID, and a permissions mask. The mask
 has up to eight bits each for possessor, user, group and other access. Only
-five of each set of eight bits are defined. These permissions granted are:
+six of each set of eight bits are defined. These permissions granted are:
 
  (*) View
 
@@ -224,6 +224,10 @@ five of each set of eight bits are defin
      keyring to a key, a process must have Write permission on the keyring and
      Link permission on the key.
 
+ (*) Set Attribute
+
+     This permits a key's UID, GID and permissions mask to be changed.
+
 For changing the ownership, group ID or permissions mask, being the owner of
 the key or having the sysadmin capability is sufficient.
 
@@ -242,15 +246,15 @@ about the status of the key service:
      this way:
 
 	SERIAL   FLAGS  USAGE EXPY PERM     UID   GID   TYPE      DESCRIPTION: SUMMARY
-	00000001 I-----    39 perm 1f1f0000     0     0 keyring   _uid_ses.0: 1/4
-	00000002 I-----     2 perm 1f1f0000     0     0 keyring   _uid.0: empty
-	00000007 I-----     1 perm 1f1f0000     0     0 keyring   _pid.1: empty
-	0000018d I-----     1 perm 1f1f0000     0     0 keyring   _pid.412: empty
-	000004d2 I--Q--     1 perm 1f1f0000    32    -1 keyring   _uid.32: 1/4
-	000004d3 I--Q--     3 perm 1f1f0000    32    -1 keyring   _uid_ses.32: empty
+	00000001 I-----    39 perm 1f3f0000     0     0 keyring   _uid_ses.0: 1/4
+	00000002 I-----     2 perm 1f3f0000     0     0 keyring   _uid.0: empty
+	00000007 I-----     1 perm 1f3f0000     0     0 keyring   _pid.1: empty
+	0000018d I-----     1 perm 1f3f0000     0     0 keyring   _pid.412: empty
+	000004d2 I--Q--     1 perm 1f3f0000    32    -1 keyring   _uid.32: 1/4
+	000004d3 I--Q--     3 perm 1f3f0000    32    -1 keyring   _uid_ses.32: empty
 	00000892 I--QU-     1 perm 1f000000     0     0 user      metal:copper: 0
-	00000893 I--Q-N     1  35s 1f1f0000     0     0 user      metal:silver: 0
-	00000894 I--Q--     1  10h 001f0000     0     0 user      metal:gold: 0
+	00000893 I--Q-N     1  35s 1f3f0000     0     0 user      metal:silver: 0
+	00000894 I--Q--     1  10h 003f0000     0     0 user      metal:gold: 0
 
      The flags are:
 
include/linux/key.h +9 -4
Index: linus-2.6/include/linux/key.h
===================================================================
@@ -40,28 +40,32 @@ struct key;
 #define KEY_POS_WRITE	0x04000000	/* possessor can update key payload / add link to keyring */
 #define KEY_POS_SEARCH	0x08000000	/* possessor can find a key in search / search a keyring */
 #define KEY_POS_LINK	0x10000000	/* possessor can create a link to a key/keyring */
-#define KEY_POS_ALL	0x1f000000
+#define KEY_POS_SETATTR	0x20000000	/* possessor can set key attributes */
+#define KEY_POS_ALL	0x3f000000
 
 #define KEY_USR_VIEW	0x00010000	/* user permissions... */
 #define KEY_USR_READ	0x00020000
 #define KEY_USR_WRITE	0x00040000
 #define KEY_USR_SEARCH	0x00080000
 #define KEY_USR_LINK	0x00100000
-#define KEY_USR_ALL	0x001f0000
+#define KEY_USR_SETATTR	0x00200000
+#define KEY_USR_ALL	0x003f0000
 
 #define KEY_GRP_VIEW	0x00000100	/* group permissions... */
 #define KEY_GRP_READ	0x00000200
 #define KEY_GRP_WRITE	0x00000400
 #define KEY_GRP_SEARCH	0x00000800
 #define KEY_GRP_LINK	0x00001000
-#define KEY_GRP_ALL	0x00001f00
+#define KEY_GRP_SETATTR	0x00002000
+#define KEY_GRP_ALL	0x00003f00
 
 #define KEY_OTH_VIEW	0x00000001	/* third party permissions... */
 #define KEY_OTH_READ	0x00000002
 #define KEY_OTH_WRITE	0x00000004
 #define KEY_OTH_SEARCH	0x00000008
 #define KEY_OTH_LINK	0x00000010
-#define KEY_OTH_ALL	0x0000001f
+#define KEY_OTH_SETATTR	0x00000020
+#define KEY_OTH_ALL	0x0000003f
 
 struct seq_file;
 struct user_struct;
@@ -119,6 +123,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;
include/linux/key-ui.h +2 -1
Index: linus-2.6/include/linux/key-ui.h
===================================================================
@@ -24,7 +24,8 @@ extern spinlock_t key_serial_lock;
 #define	KEY_WRITE	0x04	/* require permission to update / modify */
 #define	KEY_SEARCH	0x08	/* require permission to search (keyring) or find (key) */
 #define	KEY_LINK	0x10	/* require permission to link */
-#define	KEY_ALL		0x1f	/* all the above permissions */
+#define	KEY_SETATTR	0x20	/* require permission to change attributes */
+#define	KEY_ALL		0x3f	/* all the above permissions */
 
 /*
  * the keyring payload contains a list of the keys to which the keyring is
include/linux/security.h +73 -0

96 lines not on this page · raw message

security/dummy.c +23 -0
Index: linus-2.6/security/dummy.c
===================================================================
@@ -803,6 +803,23 @@ static int dummy_setprocattr(struct task
 	return -EINVAL;
 }
 
+#ifdef CONFIG_KEYS
+static inline int dummy_key_alloc(struct key *key)
+{
+	return 0;
+}
+
+static inline void dummy_key_free(struct key *key)
+{
+}
+
+static inline int dummy_key_permission(key_ref_t key_ref,
+				       struct task_struct *context,
+				       key_perm_t perm)
+{
+	return 0;
+}
+#endif /* CONFIG_KEYS */
 
 struct security_operations dummy_security_ops;
 
@@ -954,5 +971,11 @@ 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_KEYS
+	set_to_dummy_if_null(ops, key_alloc);
+	set_to_dummy_if_null(ops, key_free);
+	set_to_dummy_if_null(ops, key_permission);
+#endif	/* CONFIG_KEYS */
+
 }
 
security/keys/key.c +40 -16

145 lines not on this page · raw message

security/keys/keyctl.c +8 -5
Index: linus-2.6/security/keys/keyctl.c
===================================================================
@@ -624,8 +624,8 @@ long keyctl_keyring_search(key_serial_t 
 
 	/* link the resulting key to the destination keyring if we can */
 	if (dest_ref) {
-		ret = -EACCES;
-		if (!key_permission(key_ref, KEY_LINK))
+		ret = key_permission(key_ref, KEY_LINK);
+		if (ret < 0)
 			goto error6;
 
 		ret = key_link(key_ref_to_ptr(dest_ref), key_ref_to_ptr(key_ref));
@@ -676,8 +676,11 @@ long keyctl_read_key(key_serial_t keyid,
 	key = key_ref_to_ptr(key_ref);
 
 	/* see if we can read it directly */
-	if (key_permission(key_ref, KEY_READ))
+	ret = key_permission(key_ref, KEY_READ);
+	if (ret == 0)
 		goto can_read_key;
+	if (ret != -EACCES)
+		goto error;
 
 	/* we can't; see if it's searchable from this process's keyrings
 	 * - we automatically take account of the fact that it may be
@@ -726,7 +729,7 @@ long keyctl_chown_key(key_serial_t id, u
 	if (uid == (uid_t) -1 && gid == (gid_t) -1)
 		goto error;
 
-	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
+	key_ref = lookup_user_key(NULL, id, 1, 1, KEY_SETATTR);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
@@ -786,7 +789,7 @@ long keyctl_setperm_key(key_serial_t id,
 	if (perm & ~(KEY_POS_ALL | KEY_USR_ALL | KEY_GRP_ALL | KEY_OTH_ALL))
 		goto error;
 
-	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
+	key_ref = lookup_user_key(NULL, id, 1, 1, KEY_SETATTR);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
security/keys/keyring.c +13 -8
Index: linus-2.6/security/keys/keyring.c
===================================================================
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/security.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
 #include <asm/uaccess.h>
@@ -309,7 +310,9 @@ struct key *keyring_alloc(const char *de
 	int ret;
 
 	keyring = key_alloc(&key_type_keyring, description,
-			    uid, gid, KEY_POS_ALL | KEY_USR_ALL, not_in_quota);
+			    uid, gid,
+			    (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL,
+			    not_in_quota);
 
 	if (!IS_ERR(keyring)) {
 		ret = key_instantiate_and_link(keyring, NULL, 0, dest, NULL);
@@ -359,9 +362,11 @@ key_ref_t keyring_search_aux(key_ref_t k
 	key_check(keyring);
 
 	/* top keyring must have search permission to begin the search */
-	key_ref = ERR_PTR(-EACCES);
-	if (!key_task_permission(keyring_ref, context, KEY_SEARCH))
+        err = key_task_permission(keyring_ref, context, KEY_SEARCH);
+	if (err < 0) {
+		key_ref = ERR_PTR(err);
 		goto error;
+	}
 
 	key_ref = ERR_PTR(-ENOTDIR);
 	if (keyring->type != &key_type_keyring)
@@ -402,8 +407,8 @@ descend:
 			continue;
 
 		/* key must have search permissions */
-		if (!key_task_permission(make_key_ref(key, possessed),
-					 context, KEY_SEARCH))
+		if (key_task_permission(make_key_ref(key, possessed),
+					context, KEY_SEARCH) < 0)
 			continue;
 
 		/* we set a different error code if we find a negative key */
@@ -430,7 +435,7 @@ ascend:
 			continue;
 
 		if (!key_task_permission(make_key_ref(key, possessed),
-					 context, KEY_SEARCH))
+					 context, KEY_SEARCH) < 0)
 			continue;
 
 		/* stack the current position */
@@ -521,7 +526,7 @@ key_ref_t __keyring_search_one(key_ref_t
 			    (!key->type->match ||
 			     key->type->match(key, description)) &&
 			    key_permission(make_key_ref(key, possessed),
-					   perm) &&
+					   perm) < 0 &&
 			    !test_bit(KEY_FLAG_REVOKED, &key->flags)
 			    )
 				goto found;
@@ -617,7 +622,7 @@ struct key *find_keyring_by_name(const c
 				continue;
 
 			if (!key_permission(make_key_ref(keyring, 0),
-					    KEY_SEARCH))
+					    KEY_SEARCH) < 0)
 				continue;
 
 			/* found a potential candidate, but we still need to
security/keys/permission.c +6 -1
Index: linus-2.6/security/keys/permission.c
===================================================================
@@ -10,6 +10,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/security.h>
 #include "internal.h"
 
 /*****************************************************************************/
@@ -63,7 +64,11 @@ use_these_perms:
 
 	kperm = kperm & perm & KEY_ALL;
 
-	return kperm == perm;
+	if (kperm != perm)
+		return -EACCES;
+
+	/* let LSM be the final arbiter */
+	return security_key_permission(key_ref, context, perm);
 
 } /* end key_task_permission() */
 
security/keys/process_keys.c +4 -5
Index: linus-2.6/security/keys/process_keys.c
===================================================================
@@ -39,7 +39,7 @@ struct key root_user_keyring = {
 	.type		= &key_type_keyring,
 	.user		= &root_key_user,
 	.sem		= __RWSEM_INITIALIZER(root_user_keyring.sem),
-	.perm		= KEY_POS_ALL | KEY_USR_ALL,
+	.perm		= (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL,
 	.flags		= 1 << KEY_FLAG_INSTANTIATED,
 	.description	= "_uid.0",
 #ifdef KEY_DEBUGGING
@@ -54,7 +54,7 @@ struct key root_session_keyring = {
 	.type		= &key_type_keyring,
 	.user		= &root_key_user,
 	.sem		= __RWSEM_INITIALIZER(root_session_keyring.sem),
-	.perm		= KEY_POS_ALL | KEY_USR_ALL,
+	.perm		= (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL,
 	.flags		= 1 << KEY_FLAG_INSTANTIATED,
 	.description	= "_uid_ses.0",
 #ifdef KEY_DEBUGGING
@@ -666,9 +666,8 @@ key_ref_t lookup_user_key(struct task_st
 		goto invalid_key;
 
 	/* check the permissions */
-	ret = -EACCES;
-
-	if (!key_task_permission(key_ref, context, perm))
+	ret = key_task_permission(key_ref, context, perm);
+	if (ret < 0)
 		goto invalid_key;
 
 error:
author maintainer
#12 lore raw

PATCH 1/1 Keys: Remove incorrect and obsolete '!' operators

In reply to David Howells

The attached patch removes a couple of incorrect and obsolete '!' operators left over from the conversion of the key permission functions from true/false returns to zero/error returns.

---
warthog>diffstat -p1 keys-debang-2614rc5mm1.diff

 security/keys/keyring.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
security/keys/keyring.c +4 -4
diff -uNrp linux-2.6.14-rc5-mm1/security/keys/keyring.c linux-2.6.14-rc5-mm1-keys/security/keys/keyring.c
@@ -434,8 +434,8 @@ ascend:
 		if (sp >= KEYRING_SEARCH_MAX_DEPTH)
 			continue;
 
-		if (!key_task_permission(make_key_ref(key, possessed),
-					 context, KEY_SEARCH) < 0)
+		if (key_task_permission(make_key_ref(key, possessed),
+					context, KEY_SEARCH) < 0)
 			continue;
 
 		/* stack the current position */
@@ -621,8 +621,8 @@ struct key *find_keyring_by_name(const c
 			if (strcmp(keyring->description, name) != 0)
 				continue;
 
-			if (!key_permission(make_key_ref(keyring, 0),
-					    KEY_SEARCH) < 0)
+			if (key_permission(make_key_ref(keyring, 0),
+					   KEY_SEARCH) < 0)
 				continue;
 
 			/* found a potential candidate, but we still need to