Skip to contentSkip to conversation

authentication / encryption key retention

PATCH In review David Howells 2 participants 4 messages Sep 5, 2003 – Sep 17, 2003

Subsystems (6)
VFS MM core Scheduler Security (LSM) +2 more subsystems
lore
Share & tools
lore

David Howells

author maintainer
#0 lore raw

PATCH 1/1 authentication / encryption key retention

Hi Linus,

Here's a patch to the kernel that can provide authentication token and encryption key retention at various levels. Could you have a look at it and see if it's along the lines of what you want?

Note that it uses the prctl() syscall to propose keys to the kernel.

One thing I think it needs is some way of limiting the number of extant keys a user is permitted to have.

I can now use it to retain AFS keys obtained from Kerberos.

David

fs/exec.c +7 -1
diff -uNr linux-2.6.0-test4/fs/exec.c linux-2.6.0-test4-keys/fs/exec.c
@@ -45,6 +45,7 @@
 #include <linux/mount.h>
 #include <linux/security.h>
 #include <linux/rmap-locking.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgalloc.h>

@@ -889,10 +890,15 @@
 
 void compute_creds(struct linux_binprm *bprm) 
 {
+	if (bprm->e_uid != current->uid)
+		suid_keys(current);
+	exec_keys(current);
+
 	task_lock(current);
 	if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) {
                 current->mm->dumpable = 0;
-		
+		suid_keys(current);
+
 		if (must_not_trace_exec(current)
 		    || atomic_read(&current->fs->count) > 1
 		    || atomic_read(&current->files->count) > 1
include/linux/key.h +144 -0

144 lines not on this page · raw message

include/linux/prctl.h +11 -0
diff -uNr linux-2.6.0-test4/include/linux/prctl.h linux-2.6.0-test4-keys/include/linux/prctl.h
@@ -43,5 +43,16 @@
 # define PR_TIMING_TIMESTAMP    1       /* Accurate timestamp based
                                                    process timing */
 
+/* Manage a process's keyrings */
+#define PR_SPEC_THREAD_KEYRING		0	/* - specifier for thread-specific keyring */
+#define PR_SPEC_PROCESS_KEYRING		1	/* - specifier for process-specific keyring */
+#define PR_SPEC_SESSION_KEYRING		2	/* - specifier for session-specific keyring */
+#define PR_SPEC_USER_KEYRING		3	/* - specifier for UID-specific keyring */
+#define PR_SPEC_GROUP_KEYRING		4	/* - specifier for GID-specific keyring */
+
+#define PR_GET_KEYRING_ID		15	/* ask for specified keyring's ID */
+#define PR_CLEAR_KEYRING		16	/* clear contents of specified keyring */
+#define PR_NEW_SESSION_KEYRING		17	/* start a new session keyring */
+#define PR_ADD_NEW_KEY			18	/* add a key to specified keyring */
 
 #endif /* _LINUX_PRCTL_H */
include/linux/sched.h +9 -0
diff -uNr linux-2.6.0-test4/include/linux/sched.h linux-2.6.0-test4-keys/include/linux/sched.h
@@ -290,6 +290,10 @@
 	atomic_t processes;	/* How many processes does this user have? */
 	atomic_t files;		/* How many open files does this user have? */
 
+#ifdef CONFIG_KEYS
+	struct key *keyring;	/* UID specific keyring */
+#endif
+
 	/* Hash table maintenance information */
 	struct list_head uidhash_list;
 	uid_t uid;

@@ -402,6 +406,11 @@
 	kernel_cap_t   cap_effective, cap_inheritable, cap_permitted;
 	int keep_capabilities:1;
 	struct user_struct *user;
+#ifdef CONFIG_KEYS
+	struct key *session_keyring;	/* keyring inherited over fork */
+	struct key *process_keyring;	/* keyring private to this process (CLONE_THREAD) */
+	struct key *thread_keyring;	/* keyring private to this thread */
+#endif
 /* limits */
 	struct rlimit rlim[RLIM_NLIMITS];
 	unsigned short used_math;
kernel/exit.c +2 -0
diff -uNr linux-2.6.0-test4/kernel/exit.c linux-2.6.0-test4-keys/kernel/exit.c
@@ -22,6 +22,7 @@
 #include <linux/profile.h>
 #include <linux/mount.h>
 #include <linux/proc_fs.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>

@@ -711,6 +712,7 @@
 	exit_namespace(tsk);
 	exit_itimers(tsk);
 	exit_thread();
+	exit_keys(tsk);
 
 	if (tsk->leader)
 		disassociate_ctty(1);
kernel/fork.c +6 -1
diff -uNr linux-2.6.0-test4/kernel/fork.c linux-2.6.0-test4-keys/kernel/fork.c
@@ -30,6 +30,7 @@
 #include <linux/futex.h>
 #include <linux/ptrace.h>
 #include <linux/mount.h>
+#include <linux/key.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -884,8 +885,10 @@
 		goto bad_fork_cleanup_sighand;
 	if ((retval = copy_mm(clone_flags, p)))
 		goto bad_fork_cleanup_signal;
-	if ((retval = copy_namespace(clone_flags, p)))
+	if ((retval = copy_keys(clone_flags, p)))
 		goto bad_fork_cleanup_mm;
+	if ((retval = copy_namespace(clone_flags, p)))
+		goto bad_fork_cleanup_keys;
 	retval = copy_thread(0, clone_flags, stack_start, stack_size, p, regs);
 	if (retval)
 		goto bad_fork_cleanup_namespace;

@@ -1021,6 +1024,8 @@
 
 bad_fork_cleanup_namespace:
 	exit_namespace(p);
+bad_fork_cleanup_keys:
+	exit_keys(p);
 bad_fork_cleanup_mm:
 	exit_mm(p);
 bad_fork_cleanup_signal:
kernel/sys.c +16 -0
diff -uNr linux-2.6.0-test4/kernel/sys.c linux-2.6.0-test4-keys/kernel/sys.c
@@ -23,6 +23,7 @@
 #include <linux/security.h>
 #include <linux/dcookies.h>
 #include <linux/suspend.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>

@@ -1417,6 +1418,21 @@
 			}
 			current->keep_capabilities = arg2;
 			break;
+		case PR_GET_KEYRING_ID:
+			error = get_process_keyring_ID(arg2);
+			break;
+		case PR_CLEAR_KEYRING:
+			error = clear_process_keyring(arg2);
+			break;
+		case PR_NEW_SESSION_KEYRING:
+			error = new_session_keyring();
+			break;
+		case PR_ADD_NEW_KEY:
+			error = add_user_key(arg2,
+					     (char __user *) arg3,
+					     (char __user *) arg4,
+					     (void __user *) arg5);
+			break;
 		default:
 			error = -EINVAL;
 			break;
kernel/user.c +12 -1
diff -uNr linux-2.6.0-test4/kernel/user.c linux-2.6.0-test4-keys/kernel/user.c
@@ -12,6 +12,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/bitops.h>
+#include <linux/key.h>
 
 /*
  * UID task count cache, to get fast user lookup in "alloc_uid"

@@ -30,7 +31,10 @@
 struct user_struct root_user = {
 	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
-	.files		= ATOMIC_INIT(0)
+	.files		= ATOMIC_INIT(0),
+#ifdef CONFIG_KEYS
+	.keyring	= &root_user_keyring,
+#endif
 };
 
 /*

@@ -73,6 +77,7 @@
 {
 	if (up && atomic_dec_and_lock(&up->__count, &uidhash_lock)) {
 		uid_hash_remove(up);
+		key_put(up->keyring);
 		kmem_cache_free(uid_cachep, up);
 		spin_unlock(&uidhash_lock);
 	}

@@ -98,6 +103,11 @@
 		atomic_set(&new->processes, 0);
 		atomic_set(&new->files, 0);
 
+		if (alloc_uid_keyring(new) < 0) {
+			kmem_cache_free(uid_cachep, up);
+			return NULL;
+		}
+
 		/*
 		 * Before adding this, check whether we raced
 		 * on adding the same user already..

@@ -130,6 +140,7 @@
 	atomic_dec(&old_user->processes);
 	current->user = new_user;
 	free_uid(old_user);
+	suid_keys(current);
 }
security/Kconfig +12 -0
diff -uNr linux-2.6.0-test4/security/Kconfig linux-2.6.0-test4-keys/security/Kconfig
@@ -4,6 +4,18 @@
 
 menu "Security options"
 
+config KEYS
+	bool "Enable access key retention support"
+	help
+	  This option provides support for retaining authentication tokens and
+	  access keys in the kernel.
+
+	  It also includes provision of methods by which such keys might be
+	  associated with a process so that network filesystems, encryption
+	  support and the like can find them.
+
+	  If you are unsure how to answer this question, answer N.
+
 config SECURITY
 	bool "Enable different security models"
 	help
security/keys/internal.h +75 -0

75 lines not on this page · raw message

security/keys/key.c +595 -0

595 lines not on this page · raw message

security/keys/Makefile +9 -0
diff -uNr linux-2.6.0-test4/security/keys/Makefile linux-2.6.0-test4-keys/security/keys/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for key management
+#
+
+obj-y := \
+	key.o \
+	process_keys.o
+
+obj-$(CONFIG_PROC_FS) += proc.o
security/keys/proc.c +119 -0

119 lines not on this page · raw message

security/keys/process_keys.c +491 -0

491 lines not on this page · raw message

security/Makefile +1 -0
diff -uNr linux-2.6.0-test4/security/Makefile linux-2.6.0-test4-keys/security/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the kernel security code
 #
 
+obj-$(CONFIG_KEYS)			+= keys/
 subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
 
 # if we don't select a security model, use the default capabilities
#1 lore raw

Hi David,

some suggestions to improve your patch. First: It is very elegant.

kernel/user.c line 107

Quoted 6 lines
kernel/user.c +2 -0
@@ -98,6 +103,11 @@
 		atomic_set(&new->processes, 0);
 		atomic_set(&new->files, 0);
 
+		if (alloc_uid_keyring(new) < 0) {
+			kmem_cache_free(uid_cachep, up);

this should be 'new' instead of 'up'

Quoted 12 lines
kernel/user.c +4 -0
+			return NULL;
+		}
+
 		/*
 		 * Before adding this, check whether we raced
 		 * on adding the same user already..

@@ -130,6 +140,7 @@
 	atomic_dec(&old_user->processes);
 	current->user = new_user;
 	free_uid(old_user);
+	suid_keys(current);
 }

kernel/user.c

Quoted 10 lines
kernel/user.c +9 -0
 
+config KEYS
+	bool "Enable access key retention support"
+	help
+	  This option provides support for retaining authentication tokens and
+	  access keys in the kernel.
+
+	  It also includes provision of methods by which such keys might be
+	  associated with a process so that network filesystems, encryption
+	  support and the like can find them.

Maybe the keyword "keyring" should be put here somehow, since many security people know this concept by exactly this name.

Quoted 49 lines

linux-2.6.0-test4/security/keys/key.c 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.0-test4-keys/security/keys/key.c 2003-08-29 11:07:39.000000000 +0100

security/keys/key.c +47 -0
+	spin_lock(&key_serial_lock);
+
+	/* propose a serial number and try to insert it into the tree */
+	key->serial = key_serial_next;
+	if (key->serial < 2)
+		key->serial = 2;
+	key_serial_next = key->serial + 1;
+
+	parent = NULL;
+	p = &key_serial_tree.rb_node;
+
+	while (*p) {
+		parent = *p;
+		xkey = rb_entry(parent, struct key, serial_node);
+
+		if (key->serial < xkey->serial)
+			p = &(*p)->rb_left;
+		else if (key->serial > xkey->serial)
+			p = &(*p)->rb_right;
+		else
+			goto serial_exists;
+	}
+	goto insert_here;
+
+	/* we found a key with the proposed serial number - walk the tree from
+	 * that point looking for the next unused serial number */
+ serial_exists:
+	for (;;) {
+		key->serial = key_serial_next;
+		if (key->serial < 2)
+			key->serial = 2;
+		key_serial_next = key->serial + 1;
+
+		if (!parent->rb_parent)
+			p = &key_serial_tree.rb_node;
+		else if (parent->rb_parent->rb_left == parent)
+			p = &parent->rb_parent->rb_left;
+		else
+			p = &parent->rb_parent->rb_right;
+
+		parent = rb_next(parent);
+		if (!parent)
+			break;
+
+		xkey = rb_entry(parent, struct key, serial_node);
+		if (key->serial < xkey->serial)
+			goto insert_here;

security/keys/key.c line 139

Quoted 10 lines
security/keys/key.c +10 -0
+	}
+
+ insert_here:
+	rb_link_node(&key->serial_node, parent, p);
+	rb_insert_color(&key->serial_node, &key_serial_tree);
+	spin_unlock(&key_serial_lock);
+
+	*_key = key;
+	return 0;
+} /* end key_alloc() */

Maybe this allocator should be separated. It will be useful outside this function.

Quoted 10 lines
security/keys/key.c +8 -0
+void key_retire(struct key *key)
+{
 
 
+
+	/* duplicate the list of subscribed keys */
+	if (source->payload.subscriptions) {
+		/* prevent the source keyring from being altered */
+		down_read(&source->sem);
+

[...]

security/keys/key.c line 238

Quoted 3 lines
security/keys/key.c +3 -0
+
+		up_read(&source->sem);
+	}

You should take the semaphore always to prevent the tiny race, where you allocate the first subscription, while you check this.

Quoted 63 lines
security/keys/key.c +63 -0
+int keyring_search(struct key *keyring,
+		   const struct key_type *type,
+		   const char *description,
+		   struct key **_key)
+{
+	struct {
+		struct key *keyring;
+		struct keyring_list *keylist;
+		int kix;
+	} stack[KEYRING_SEARCH_MAX_DEPTH];
+
+	struct keyring_list *keylist;
+	struct key *key;
+	int sp, psp, kix;
+
+	key_validate(keyring);
+
+	*_key = NULL;
+
+	if (keyring->type != &key_type_keyring)
+		return -EINVAL;
+
+	sp = 0;
+
+ descend:
+	read_lock(&keyring->lock);
+	if (keyring->flags & KEY_FLAG_RETIRED)
+		goto retired;
+
+	keylist = keyring->payload.subscriptions;
+	kix = 0;
+
+ ascend:
+	while (kix < keylist->nkeys) {
+		key = keylist->keys[kix];
+
+		if (key->type == type && key->type->match(key, description)) {
+			if (key->flags & KEY_FLAG_RETIRED)
+				goto next;
+			atomic_inc(&key->usage);
+			goto found;
+		}
+
+		if (key->type == &key_type_keyring) {
+			if (sp >= KEYRING_SEARCH_MAX_DEPTH)
+				goto next;
+
+			/* evade loops in the keyring tree */
+			for (psp = 0; psp < sp; psp++)
+				if (stack[psp].keyring == keyring)
+					goto next;
+
+			stack[sp].keyring = keyring;
+			stack[sp].keylist = keylist;
+			stack[sp].kix = kix;
+			sp++;
+			goto descend;
+		}
+
+	next:
+		kix++;
+	}
+

security/keys/key.c line 328

Quoted 10 lines
security/keys/key.c +10 -0
+ retired:
+	read_unlock(&keyring->lock);
+
+	if (sp > 0) {
+		sp--;
+		keyring = stack[sp].keyring;
+		keylist = stack[sp].keylist;
+		kix = stack[sp].kix + 1;
+		goto ascend;
+	}

Why do you do the iteration without taking the spinlock here? That whole function looks a little hackish and is difficult to follow. I would at least like to see

for (; kix < keyring->payload.subscriptions->nkeys; kix++) {
	key = keyring->payload.subscriptions->keys[kix];
}

Now the goto next becomes continue.

You also never need to store keylist on the stack, because of the identity stack[sp].keyring->payload.subscriptions == stack[sp].keylist

This should reduce stack usage or allow deeper nesting.

Loop detection should better be done on insert, not on search. With the above code you allow loops, but ignore them, right?

Quoted 4 lines
security/keys/key.c +4 -0
+int keyring_set_name(struct key *keyring, const char *fmt, ...)
+{
+	struct keyring_name *kname;
+	va_list va;

security/keys/key.c line 440

Quoted 10 lines
security/keys/key.c +10 -0
+	size_t len;
+	char buf[32];
+
+	key_validate(keyring);
+
+	if (keyring->type != &key_type_keyring)
+		return -EINVAL;
+
+	va_start(va, fmt);
+	vsnprintf(buf, 32, fmt, va);

you should check the return value of vsnprintf here end fail for ret >= sizeof(buf)

security/keys/key.c line 442

Quoted 2 lines
security/keys/key.c +2 -0
+	va_end(va);
+	len = strlen(buf);

Then this becomes unnecessary.

security/keys/key.c line 519

Quoted 12 lines

+/************************************************************************* ****/ +/*
+ * describe the keyring
+ */
+static void keyring_describe(const struct key *keyring, struct seq_file *m) +{

security/keys/key.c +6 -0
+	struct keyring_name *kname;
+	struct keyring_list *klist;
+
+	kname = keyring->description.ringname;
+	if (kname) {
+		seq_printf(m, "%s", kname->name);

What about seq_puts(kname->name) here?

Quoted 7 lines
security/keys/process_keys.c +7 -0
+static int install_session_keyring(struct task_struct *tsk)
+{
+	struct key *keyring;
+	int ret;
+
+	ret = keyring_alloc(NULL, &keyring);
+	if (ret < 0)

security/keys/process_keys.c line 103

Quoted 10 lines
security/keys/process_keys.c +10 -0
+		return ret;
+
+	ret = keyring_set_name(keyring, "_ses.%u", keyring->serial);
+	if (ret < 0)
+		return ret;
+
+	key_put(xchg(&tsk->session_keyring, keyring));
+
+	return 0;
+} /* end install_session_keyring() */

Why is this one implemented differently from the others above?

security/keys/process_keys.c line 205

Quoted 7 lines
security/keys/process_keys.c +7 -0
+long clear_process_keyring(int specifier)
+{
+
+	case PR_SPEC_GROUP_KEYRING:
+	default:
+		return -EINVAL;
+	}

Not implemented, yet? ;-)

Quoted 6 lines

+/************************************************************************* ****/ +/*
+ * install a new session keyring, discarding the old one
+ * - implements prctl(PR_NEW_SESSION_KEYRING)
+ */

security/keys/process_keys.c +1 -0
+long new_session_keyring(void)

security/keys/process_keys.c line 241

Quoted 10 lines
security/keys/process_keys.c +10 -0
+{
+	int ret;
+
+	ret = install_session_keyring(current);
+	if (ret < 0)
+		return ret;
+
+	return current->session_keyring ? current->session_keyring->serial : 0;
+
+} /* end new_session_keyring() */

You evaluate current three times here. Evaluating it is expensive. You might consider using a variable holding current instead.

Quoted 5 lines
security/keys/process_keys.c +5 -0
+long add_user_key(int specifier,
+		  char __user *_type,
+		  char __user *_description,
+		  void __user *_payload)
+{

[...]

security/keys/process_keys.c line 355

Quoted 1 line
security/keys/process_keys.c +1 -0
+	ret = get_user(plen, (uint16_t *) _payload);

Why this cast?

Quoted 15 lines
security/keys/process_keys.c +15 -0
+int copy_keys(unsigned long clone_flags, struct task_struct *tsk)
+{
+	int ret = 0;
+
+	key_validate(tsk->session_keyring);
+	key_validate(tsk->process_keyring);
+	key_validate(tsk->thread_keyring);
+
+	if (tsk->session_keyring)
+		atomic_inc(&tsk->session_keyring->usage);
+
+	if (tsk->process_keyring) {
+		if (clone_flags & CLONE_THREAD) {
+			atomic_inc(&tsk->process_keyring->usage);
+		}

security/keys/process_keys.c line 410

Quoted 10 lines
security/keys/process_keys.c +10 -0
+		else {
+			tsk->process_keyring = NULL;
+			ret = install_process_keyring(tsk);
+		}
+	}
+
+	tsk->thread_keyring = NULL;
+
+	return ret;
+} /* end copy_keys() */

What about the per thread keyring?
Is there an atomic_inc(tsk->thread_keyring) missing?

So all in all, it looks like a very nice and clean patch.

I'm looking forward to have this included.

Regards

Ingo Oeser

author maintainer
#2 lore raw

Hi Ingo,

Quoted 1 line

some suggestions to improve your patch. First: It is very elegant.

Thanks:-)

Quoted 2 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+			kmem_cache_free(uid_cachep, up);

this should be 'new' instead of 'up'

Fixed.

Quoted 2 lines

Maybe the keyword "keyring" should be put here somehow, since many security people know this concept by exactly this name.

Done:

+	  Furthermore, a special type of key is available that acts as keyring:
+	  a searchable sequence of keys. Each process is equipped with access
+	  to five standard keyrings: UID-specific, GID-specific, session,
+	  process and thread.
Quoted 2 lines

Maybe this allocator should be separated. It will be useful outside this function.

By "this allocator" do you mean the algorithm that generates a unique serial number?

Quoted 2 lines

You should take the semaphore always to prevent the tiny race, where you allocate the first subscription, while you check this.

I don't think there's a race there. We do a spot check, and if there's nothing there at that time, so be it. True, the subscriber may be inside the semaphore, preparatory to adding a ring, but they haven't done so yet, so it isn't much of a problem.

However, I'll make the change anyway... it's probably not going to be called much, so it shouldn't hurt.

Quoted 4 lines
Quoted 4 lines

+ retired:

(quoted diff) +3 -0
+	read_unlock(&keyring->lock);
+
+	if (sp > 0) {
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+		sp--;
+		keyring = stack[sp].keyring;
+		keylist = stack[sp].keylist;
+		kix = stack[sp].kix + 1;
+		goto ascend;
+	}

Why do you do the iteration without taking the spinlock here?

What do you mean? I read_lock() every node as I deal with it. All the nodes stored on the stack are still read_locked().

And do you mean iteration or recursion?

Quoted 7 lines

That whole function looks a little hackish and is difficult to follow. I would at least like to see

for (; kix < keyring->payload.subscriptions->nkeys; kix++) {
	key = keyring->payload.subscriptions->keys[kix];
}

Now the goto next becomes continue.

Erm... what about the recursion element? The set of keyrings is a graph not a one dimensional list (some of the keys may actually be keyrings themselves).

Quoted 2 lines

You also never need to store keylist on the stack, because of the identity stack[sp].keyring->payload.subscriptions == stack[sp].keylist

Good point. Fixed.

Quoted 1 line

This should reduce stack usage or allow deeper nesting.

True. But 6 levels is probably enough.

Quoted 2 lines

Loop detection should better be done on insert, not on search. With the above code you allow loops, but ignore them, right?

Yes, loop detection is a pain whenever it is done, especially as loops can cause recursive deadlocks in keyring_search().

I'm not sure what'd be the best way to avoid loop detection. What if the user sets up three keyrings and then subscribes A to B, B to C and C to A? On the other hand what if they subscribe A to B, B to C and A to C?

I think the only way to do it would be to add in the new subscription and then walk the new graph in its entirety, marking keyrings as you go down and then unmarking them as you rise back up, and giving an error if we try to walk over a node that's already marked.

The problem again is evading recursive deadlock - particularly against other algorithms also recursively walking the graph at the same time.

My initial thought was just to say that a user can only subscribe keyrings to one of the five process keyrings, and those can't be subscribed elsewhere.

Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	vsnprintf(buf, 32, fmt, va);

you should check the return value of vsnprintf here end fail for ret >= sizeof(buf)

But that can't happen. The kernel's vsnprintf() returns the number of chars written into the buffer (exclusive of the NUL char), and so if its return value is >= sizeof(buf) then your stack is stuffed:

	[lib/vsprintf.c]
	int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
	{
	...
		/* the trailing null byte doesn't count towards the total
		* ++str;
		*/
		return str-buf;
	}

However, I've changed things to eliminate the strlen() call and use the result of vsnprintf() instead.

Quoted 3 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+		seq_printf(m, "%s", kname->name);

What about seq_puts(kname->name) here?

Done. It used to say something else as well, but no longer...

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+static int install_session_keyring(struct task_struct *tsk)
+{

...
Why is this one implemented differently from the others above?

At one point the function did something else as well, but that has been taken out. I've normalised the process & thread versions to be the same, and I've made them release the newly allocated keyring if the naming function fails.

Quoted 3 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	case PR_SPEC_GROUP_KEYRING:

...
Not implemented, yet? ;-)

No. I don't want to start mucking around adding "struct group" to match "struct user" just yet. I'll consider that after Linus adds this key management stuff to the kernel.

I'd also like to add "login session" and "process" / "thread group" structs too.

Quoted 4 lines
Quoted 1 line

+long new_session_keyring(void)

...
You evaluate current three times here. Evaluating it is expensive. You might consider using a variable holding current instead.

It's not that expensive, at least not on the x86 arch. It's probably more expensive to save an extra value on the stack so as to have the value to hand. Besides, the get_current() function probably ought to be __attribute__((const)).

Quoted 3 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	ret = get_user(plen, (uint16_t *) _payload);

Why this cast?

get_user() looks at the pointer type to determine the fetch size. _payload is void *.

Quoted 4 lines
Quoted 1 line

+int copy_keys(unsigned long clone_flags, struct task_struct *tsk)

...
What about the per thread keyring?
Is there an atomic_inc(tsk->thread_keyring) missing?

No. Note the following:

	tsk->thread_keyring = NULL;
Quoted 1 line

So all in all, it looks like a very nice and clean patch.

I've attached a revised patch:-)

Quoted 1 line

I'm looking forward to have this included.

So am I.

Thanks for your comments,
David

fs/exec.c +7 -1
diff -uNr linux-2.6.0-test4/fs/exec.c linux-2.6.0-test4-keys/fs/exec.c
@@ -45,6 +45,7 @@
 #include <linux/mount.h>
 #include <linux/security.h>
 #include <linux/rmap-locking.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgalloc.h>

@@ -889,10 +890,15 @@
 
 void compute_creds(struct linux_binprm *bprm) 
 {
+	if (bprm->e_uid != current->uid)
+		suid_keys(current);
+	exec_keys(current);
+
 	task_lock(current);
 	if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) {
                 current->mm->dumpable = 0;
-		
+		suid_keys(current);
+
 		if (must_not_trace_exec(current)
 		    || atomic_read(&current->fs->count) > 1
 		    || atomic_read(&current->files->count) > 1
include/linux/key.h +144 -0

144 lines not on this page · raw message

include/linux/prctl.h +11 -0
diff -uNr linux-2.6.0-test4/include/linux/prctl.h linux-2.6.0-test4-keys/include/linux/prctl.h
@@ -43,5 +43,16 @@
 # define PR_TIMING_TIMESTAMP    1       /* Accurate timestamp based
                                                    process timing */
 
+/* Manage a process's keyrings */
+#define PR_SPEC_THREAD_KEYRING		0	/* - specifier for thread-specific keyring */
+#define PR_SPEC_PROCESS_KEYRING		1	/* - specifier for process-specific keyring */
+#define PR_SPEC_SESSION_KEYRING		2	/* - specifier for session-specific keyring */
+#define PR_SPEC_USER_KEYRING		3	/* - specifier for UID-specific keyring */
+#define PR_SPEC_GROUP_KEYRING		4	/* - specifier for GID-specific keyring */
+
+#define PR_GET_KEYRING_ID		15	/* ask for specified keyring's ID */
+#define PR_CLEAR_KEYRING		16	/* clear contents of specified keyring */
+#define PR_NEW_SESSION_KEYRING		17	/* start a new session keyring */
+#define PR_ADD_NEW_KEY			18	/* add a key to specified keyring */
 
 #endif /* _LINUX_PRCTL_H */
include/linux/sched.h +9 -0
diff -uNr linux-2.6.0-test4/include/linux/sched.h linux-2.6.0-test4-keys/include/linux/sched.h
@@ -290,6 +290,10 @@
 	atomic_t processes;	/* How many processes does this user have? */
 	atomic_t files;		/* How many open files does this user have? */
 
+#ifdef CONFIG_KEYS
+	struct key *keyring;	/* UID specific keyring */
+#endif
+
 	/* Hash table maintenance information */
 	struct list_head uidhash_list;
 	uid_t uid;

@@ -402,6 +406,11 @@
 	kernel_cap_t   cap_effective, cap_inheritable, cap_permitted;
 	int keep_capabilities:1;
 	struct user_struct *user;
+#ifdef CONFIG_KEYS
+	struct key *session_keyring;	/* keyring inherited over fork */
+	struct key *process_keyring;	/* keyring private to this process (CLONE_THREAD) */
+	struct key *thread_keyring;	/* keyring private to this thread */
+#endif
 /* limits */
 	struct rlimit rlim[RLIM_NLIMITS];
 	unsigned short used_math;
kernel/exit.c +2 -0
diff -uNr linux-2.6.0-test4/kernel/exit.c linux-2.6.0-test4-keys/kernel/exit.c
@@ -22,6 +22,7 @@
 #include <linux/profile.h>
 #include <linux/mount.h>
 #include <linux/proc_fs.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>

@@ -711,6 +712,7 @@
 	exit_namespace(tsk);
 	exit_itimers(tsk);
 	exit_thread();
+	exit_keys(tsk);
 
 	if (tsk->leader)
 		disassociate_ctty(1);
kernel/fork.c +6 -1
diff -uNr linux-2.6.0-test4/kernel/fork.c linux-2.6.0-test4-keys/kernel/fork.c
@@ -30,6 +30,7 @@
 #include <linux/futex.h>
 #include <linux/ptrace.h>
 #include <linux/mount.h>
+#include <linux/key.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -884,8 +885,10 @@
 		goto bad_fork_cleanup_sighand;
 	if ((retval = copy_mm(clone_flags, p)))
 		goto bad_fork_cleanup_signal;
-	if ((retval = copy_namespace(clone_flags, p)))
+	if ((retval = copy_keys(clone_flags, p)))
 		goto bad_fork_cleanup_mm;
+	if ((retval = copy_namespace(clone_flags, p)))
+		goto bad_fork_cleanup_keys;
 	retval = copy_thread(0, clone_flags, stack_start, stack_size, p, regs);
 	if (retval)
 		goto bad_fork_cleanup_namespace;

@@ -1021,6 +1024,8 @@
 
 bad_fork_cleanup_namespace:
 	exit_namespace(p);
+bad_fork_cleanup_keys:
+	exit_keys(p);
 bad_fork_cleanup_mm:
 	exit_mm(p);
 bad_fork_cleanup_signal:
kernel/sys.c +16 -0
diff -uNr linux-2.6.0-test4/kernel/sys.c linux-2.6.0-test4-keys/kernel/sys.c
@@ -23,6 +23,7 @@
 #include <linux/security.h>
 #include <linux/dcookies.h>
 #include <linux/suspend.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>

@@ -1417,6 +1418,21 @@
 			}
 			current->keep_capabilities = arg2;
 			break;
+		case PR_GET_KEYRING_ID:
+			error = get_process_keyring_ID(arg2);
+			break;
+		case PR_CLEAR_KEYRING:
+			error = clear_process_keyring(arg2);
+			break;
+		case PR_NEW_SESSION_KEYRING:
+			error = new_session_keyring();
+			break;
+		case PR_ADD_NEW_KEY:
+			error = add_user_key(arg2,
+					     (char __user *) arg3,
+					     (char __user *) arg4,
+					     (void __user *) arg5);
+			break;
 		default:
 			error = -EINVAL;
 			break;
kernel/user.c +12 -1
diff -uNr linux-2.6.0-test4/kernel/user.c linux-2.6.0-test4-keys/kernel/user.c
@@ -12,6 +12,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/bitops.h>
+#include <linux/key.h>
 
 /*
  * UID task count cache, to get fast user lookup in "alloc_uid"

@@ -30,7 +31,10 @@
 struct user_struct root_user = {
 	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
-	.files		= ATOMIC_INIT(0)
+	.files		= ATOMIC_INIT(0),
+#ifdef CONFIG_KEYS
+	.keyring	= &root_user_keyring,
+#endif
 };
 
 /*

@@ -73,6 +77,7 @@
 {
 	if (up && atomic_dec_and_lock(&up->__count, &uidhash_lock)) {
 		uid_hash_remove(up);
+		key_put(up->keyring);
 		kmem_cache_free(uid_cachep, up);
 		spin_unlock(&uidhash_lock);
 	}

@@ -98,6 +103,11 @@
 		atomic_set(&new->processes, 0);
 		atomic_set(&new->files, 0);
 
+		if (alloc_uid_keyring(new) < 0) {
+			kmem_cache_free(uid_cachep, new);
+			return NULL;
+		}
+
 		/*
 		 * Before adding this, check whether we raced
 		 * on adding the same user already..

@@ -130,6 +140,7 @@
 	atomic_dec(&old_user->processes);
 	current->user = new_user;
 	free_uid(old_user);
+	suid_keys(current);
 }
security/Kconfig +17 -0
diff -uNr linux-2.6.0-test4/security/Kconfig linux-2.6.0-test4-keys/security/Kconfig
@@ -4,6 +4,23 @@
 
 menu "Security options"
 
+config KEYS
+	bool "Enable access key retention support"
+	help
+	  This option provides support for retaining authentication tokens and
+	  access keys in the kernel.
+
+	  It also includes provision of methods by which such keys might be
+	  associated with a process so that network filesystems, encryption
+	  support and the like can find them.
+
+	  Furthermore, a special type of key is available that acts as keyring:
+	  a searchable sequence of keys. Each process is equipped with access
+	  to five standard keyrings: UID-specific, GID-specific, session,
+	  process and thread.
+
+	  If you are unsure as to whether this is required, answer N.
+
 config SECURITY
 	bool "Enable different security models"
 	help
security/keys/internal.h +75 -0

75 lines not on this page · raw message

security/keys/key.c +590 -0

590 lines not on this page · raw message

security/keys/Makefile +9 -0
diff -uNr linux-2.6.0-test4/security/keys/Makefile linux-2.6.0-test4-keys/security/keys/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for key management
+#
+
+obj-y := \
+	key.o \
+	process_keys.o
+
+obj-$(CONFIG_PROC_FS) += proc.o
security/keys/proc.c +119 -0

119 lines not on this page · raw message

security/keys/process_keys.c +506 -0

506 lines not on this page · raw message

security/Makefile +1 -0
diff -uNr linux-2.6.0-test4/security/Makefile linux-2.6.0-test4-keys/security/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the kernel security code
 #
 
+obj-$(CONFIG_KEYS)			+= keys/
 subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
 
 # if we don't select a security model, use the default capabilities
author maintainer
#3 lore raw

Hi Ingo,

Quoted 5 lines
Quoted 2 lines

Loop detection should better be done on insert, not on search. With the above code you allow loops, but ignore them, right?

Yes, loop detection is a pain whenever it is done, especially as loops can cause recursive deadlocks in keyring_search().

I've produced a new patch with loop detection added in (keyring_add_key() should now return -EDEADLK if you try to create a loop).

I've also got u-space tools to allow some keyring manipulation and also to submit AFS Kerberos keys for retention if you want a look.

David

fs/exec.c +7 -1
diff -uNr linux-2.6.0-test5/fs/exec.c linux-2.6.0-test5-keys/fs/exec.c
@@ -45,6 +45,7 @@
 #include <linux/mount.h>
 #include <linux/security.h>
 #include <linux/rmap-locking.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgalloc.h>

@@ -889,10 +890,15 @@
 
 void compute_creds(struct linux_binprm *bprm) 
 {
+	if (bprm->e_uid != current->uid)
+		suid_keys(current);
+	exec_keys(current);
+
 	task_lock(current);
 	if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) {
                 current->mm->dumpable = 0;
-		
+		suid_keys(current);
+
 		if (must_not_trace_exec(current)
 		    || atomic_read(&current->fs->count) > 1
 		    || atomic_read(&current->files->count) > 1
include/linux/key.h +144 -0

144 lines not on this page · raw message

include/linux/prctl.h +11 -0
diff -uNr linux-2.6.0-test5/include/linux/prctl.h linux-2.6.0-test5-keys/include/linux/prctl.h
@@ -43,5 +43,16 @@
 # define PR_TIMING_TIMESTAMP    1       /* Accurate timestamp based
                                                    process timing */
 
+/* Manage a process's keyrings */
+#define PR_SPEC_THREAD_KEYRING		0	/* - specifier for thread-specific keyring */
+#define PR_SPEC_PROCESS_KEYRING		1	/* - specifier for process-specific keyring */
+#define PR_SPEC_SESSION_KEYRING		2	/* - specifier for session-specific keyring */
+#define PR_SPEC_USER_KEYRING		3	/* - specifier for UID-specific keyring */
+#define PR_SPEC_GROUP_KEYRING		4	/* - specifier for GID-specific keyring */
+
+#define PR_GET_KEYRING_ID		15	/* ask for specified keyring's ID */
+#define PR_CLEAR_KEYRING		16	/* clear contents of specified keyring */
+#define PR_NEW_SESSION_KEYRING		17	/* start a new session keyring */
+#define PR_ADD_NEW_KEY			18	/* add a key to specified keyring */
 
 #endif /* _LINUX_PRCTL_H */
include/linux/sched.h +9 -0
diff -uNr linux-2.6.0-test5/include/linux/sched.h linux-2.6.0-test5-keys/include/linux/sched.h
@@ -290,6 +290,10 @@
 	atomic_t processes;	/* How many processes does this user have? */
 	atomic_t files;		/* How many open files does this user have? */
 
+#ifdef CONFIG_KEYS
+	struct key *keyring;	/* UID specific keyring */
+#endif
+
 	/* Hash table maintenance information */
 	struct list_head uidhash_list;
 	uid_t uid;

@@ -403,6 +407,11 @@
 	kernel_cap_t   cap_effective, cap_inheritable, cap_permitted;
 	int keep_capabilities:1;
 	struct user_struct *user;
+#ifdef CONFIG_KEYS
+	struct key *session_keyring;	/* keyring inherited over fork */
+	struct key *process_keyring;	/* keyring private to this process (CLONE_THREAD) */
+	struct key *thread_keyring;	/* keyring private to this thread */
+#endif
 /* limits */
 	struct rlimit rlim[RLIM_NLIMITS];
 	unsigned short used_math;
kernel/exit.c +2 -0
diff -uNr linux-2.6.0-test5/kernel/exit.c linux-2.6.0-test5-keys/kernel/exit.c
@@ -22,6 +22,7 @@
 #include <linux/profile.h>
 #include <linux/mount.h>
 #include <linux/proc_fs.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>

@@ -713,6 +714,7 @@
 	exit_namespace(tsk);
 	exit_itimers(tsk);
 	exit_thread();
+	exit_keys(tsk);
 
 	if (tsk->leader)
 		disassociate_ctty(1);
kernel/fork.c +6 -1
diff -uNr linux-2.6.0-test5/kernel/fork.c linux-2.6.0-test5-keys/kernel/fork.c
@@ -30,6 +30,7 @@
 #include <linux/futex.h>
 #include <linux/ptrace.h>
 #include <linux/mount.h>
+#include <linux/key.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -885,8 +886,10 @@
 		goto bad_fork_cleanup_sighand;
 	if ((retval = copy_mm(clone_flags, p)))
 		goto bad_fork_cleanup_signal;
-	if ((retval = copy_namespace(clone_flags, p)))
+	if ((retval = copy_keys(clone_flags, p)))
 		goto bad_fork_cleanup_mm;
+	if ((retval = copy_namespace(clone_flags, p)))
+		goto bad_fork_cleanup_keys;
 	retval = copy_thread(0, clone_flags, stack_start, stack_size, p, regs);
 	if (retval)
 		goto bad_fork_cleanup_namespace;

@@ -1022,6 +1025,8 @@
 
 bad_fork_cleanup_namespace:
 	exit_namespace(p);
+bad_fork_cleanup_keys:
+	exit_keys(p);
 bad_fork_cleanup_mm:
 	exit_mm(p);
 bad_fork_cleanup_signal:
kernel/sys.c +16 -0
diff -uNr linux-2.6.0-test5/kernel/sys.c linux-2.6.0-test5-keys/kernel/sys.c
@@ -23,6 +23,7 @@
 #include <linux/security.h>
 #include <linux/dcookies.h>
 #include <linux/suspend.h>
+#include <linux/key.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>

@@ -1423,6 +1424,21 @@
 			}
 			current->keep_capabilities = arg2;
 			break;
+		case PR_GET_KEYRING_ID:
+			error = get_process_keyring_ID(arg2);
+			break;
+		case PR_CLEAR_KEYRING:
+			error = clear_process_keyring(arg2);
+			break;
+		case PR_NEW_SESSION_KEYRING:
+			error = new_session_keyring();
+			break;
+		case PR_ADD_NEW_KEY:
+			error = add_user_key(arg2,
+					     (char __user *) arg3,
+					     (char __user *) arg4,
+					     (void __user *) arg5);
+			break;
 		default:
 			error = -EINVAL;
 			break;
kernel/user.c +12 -1
diff -uNr linux-2.6.0-test5/kernel/user.c linux-2.6.0-test5-keys/kernel/user.c
@@ -12,6 +12,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/bitops.h>
+#include <linux/key.h>
 
 /*
  * UID task count cache, to get fast user lookup in "alloc_uid"

@@ -30,7 +31,10 @@
 struct user_struct root_user = {
 	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
-	.files		= ATOMIC_INIT(0)
+	.files		= ATOMIC_INIT(0),
+#ifdef CONFIG_KEYS
+	.keyring	= &root_user_keyring,
+#endif
 };
 
 /*

@@ -73,6 +77,7 @@
 {
 	if (up && atomic_dec_and_lock(&up->__count, &uidhash_lock)) {
 		uid_hash_remove(up);
+		key_put(up->keyring);
 		kmem_cache_free(uid_cachep, up);
 		spin_unlock(&uidhash_lock);
 	}

@@ -98,6 +103,11 @@
 		atomic_set(&new->processes, 0);
 		atomic_set(&new->files, 0);
 
+		if (alloc_uid_keyring(new) < 0) {
+			kmem_cache_free(uid_cachep, new);
+			return NULL;
+		}
+
 		/*
 		 * Before adding this, check whether we raced
 		 * on adding the same user already..

@@ -130,6 +140,7 @@
 	atomic_dec(&old_user->processes);
 	current->user = new_user;
 	free_uid(old_user);
+	suid_keys(current);
 }
security/Kconfig +17 -0
diff -uNr linux-2.6.0-test5/security/Kconfig linux-2.6.0-test5-keys/security/Kconfig
@@ -4,6 +4,23 @@
 
 menu "Security options"
 
+config KEYS
+	bool "Enable access key retention support"
+	help
+	  This option provides support for retaining authentication tokens and
+	  access keys in the kernel.
+
+	  It also includes provision of methods by which such keys might be
+	  associated with a process so that network filesystems, encryption
+	  support and the like can find them.
+
+	  Furthermore, a special type of key is available that acts as keyring:
+	  a searchable sequence of keys. Each process is equipped with access
+	  to five standard keyrings: UID-specific, GID-specific, session,
+	  process and thread.
+
+	  If you are unsure as to whether this is required, answer N.
+
 config SECURITY
 	bool "Enable different security models"
 	help
security/keys/internal.h +75 -0

75 lines not on this page · raw message

security/keys/key.c +714 -0

714 lines not on this page · raw message

security/keys/Makefile +9 -0
diff -uNr linux-2.6.0-test5/security/keys/Makefile linux-2.6.0-test5-keys/security/keys/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for key management
+#
+
+obj-y := \
+	key.o \
+	process_keys.o
+
+obj-$(CONFIG_PROC_FS) += proc.o
security/keys/proc.c +120 -0

120 lines not on this page · raw message

security/keys/process_keys.c +510 -0

510 lines not on this page · raw message

security/Makefile +1 -0
diff -uNr linux-2.6.0-test5/security/Makefile linux-2.6.0-test5-keys/security/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the kernel security code
 #
 
+obj-$(CONFIG_KEYS)			+= keys/
 subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
 
 # if we don't select a security model, use the default capabilities