Keyrings, user namespaces and the user_struct

Commit Message

Eric W. Biederman Oct. 26, 2016, 5:54 a.m. UTC

David Howells <[e-mail]> writes:

> Andy Lutomirski <[e-mail]> wrote:
>
>> ... Perhaps we could simply *remove* the concept of named keys and keyrings.
>
> See Linus's dictum about breaking userspace.
>
> The problem isn't named keys: keys have to be named - the description is how
> they're looked up typically.  Further, non-keyring keys can't be looked up
> directly by name - you have to search for them in a keyring.
>
> The issue here is named keyrings and keyctl_join_session_keyring().  It might
> well have been a bad idea - though I've seen some people arguing for a single
> session keyring shared across all a user's logins, in which case, we might
> want this after all (or use the user-default session).
>
> One thing we perhaps do want to do, though, is restrict the names of keyrings
> to the user_namespace in which the keyring was created.

Grr...

The first round of user namespace support had actually restricted the
description lookup to a single user namespace.  Then I missed a detail
and converted the code to it's current form.  Ooops!

I think the answer for all of the issues raised in this conversation is
to just make the keyring names and the keyring name lookup per user
namespace.

Maybe a few small additional tweaks to install_user_keyrings to notice
if we have the user keyring from the wrong user namespace.

Something like the untested patch below.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric W. Biederman Oct. 26, 2016, 6:48 a.m. UTC | #1

[e-mail] (Eric W. Biederman) writes:

> David Howells <[e-mail]> writes:
>
>> Andy Lutomirski <[e-mail]> wrote:
>>
>>> ... Perhaps we could simply *remove* the concept of named keys and keyrings.
>>
>> See Linus's dictum about breaking userspace.
>>
>> The problem isn't named keys: keys have to be named - the description is how
>> they're looked up typically.  Further, non-keyring keys can't be looked up
>> directly by name - you have to search for them in a keyring.
>>
>> The issue here is named keyrings and keyctl_join_session_keyring().  It might
>> well have been a bad idea - though I've seen some people arguing for a single
>> session keyring shared across all a user's logins, in which case, we might
>> want this after all (or use the user-default session).
>>
>> One thing we perhaps do want to do, though, is restrict the names of keyrings
>> to the user_namespace in which the keyring was created.
>
> Grr...
>
> The first round of user namespace support had actually restricted the
> description lookup to a single user namespace.  Then I missed a detail
> and converted the code to it's current form.  Ooops!
>
> I think the answer for all of the issues raised in this conversation is
> to just make the keyring names and the keyring name lookup per user
> namespace.
>
> Maybe a few small additional tweaks to install_user_keyrings to notice
> if we have the user keyring from the wrong user namespace.
>
> Something like the untested patch below.

Ugh but that approach runs into the fact that the uid_keyring lives
in user_struct.

So in the short term it is probably better to use the uid in the initial
user namespace in install_user_keyrings.  Then in
keyctl_join_session_keyring look for names that start with _uid. and
_uid_ses. and change the strings to have the numbers in the initial user
namespace instead of the current user namespace.

That would be the smallest change we could make, that would result
in correct code.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index eb209d4523f5..5ea474df16bd 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -52,6 +52,12 @@  struct user_namespace {
 	struct key		*persistent_keyring_register;
 	struct rw_semaphore	persistent_keyring_register_sem;
 #endif
+#ifdef CONFIG_KEYS
+# ifndef KEYRING_NAME_HASH_SIZE
+# define KEYRING_NAME_HASH_SIZE	(1 << 5)
+# endif
+	struct list_head keyring_name_hash[KEYRING_NAME_HASH_SIZE];
+#endif
 	struct work_struct	work;
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_set	set;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index a705a7d92ad7..e83e9e6129fc 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -18,6 +18,7 @@ 
 #include <linux/keyctl.h>
 
 struct iovec;
+struct user_namespace;
 
 #ifdef __KDEBUG
 #define kenter(FMT, ...) \
@@ -137,7 +138,7 @@  extern key_ref_t keyring_search_aux(key_ref_t keyring_ref,
 extern key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx);
 extern key_ref_t search_process_keyrings(struct keyring_search_context *ctx);
 
-extern struct key *find_keyring_by_name(const char *name, bool skip_perm_check);
+extern struct key *find_keyring_by_name(struct user_namespace *ns, const char *name, bool skip_perm_check);
 
 extern int install_user_keyrings(void);
 extern int install_thread_keyring_to_cred(struct cred *);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index c91e4e0cea08..be1bb5ca1c3a 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -20,6 +20,7 @@ 
 #include <keys/user-type.h>
 #include <linux/assoc_array_priv.h>
 #include <linux/uaccess.h>
+#include <linux/user_namespace.h>
 #include "internal.h"
 
 /*
@@ -55,7 +56,6 @@  static inline void *keyring_key_to_ptr(struct key *key)
 	return key;
 }
 
-static struct list_head	keyring_name_hash[KEYRING_NAME_HASH_SIZE];
 static DEFINE_RWLOCK(keyring_name_lock);
 
 static inline unsigned keyring_hash(const char *desc)
@@ -106,7 +106,7 @@  static DECLARE_RWSEM(keyring_serialise_link_sem);
  * Publish the name of a keyring so that it can be found by name (if it has
  * one).
  */
-static void keyring_publish_name(struct key *keyring)
+static void keyring_publish_name(struct user_namespace *ns, struct key *keyring)
 {
 	int bucket;
 
@@ -115,11 +115,11 @@  static void keyring_publish_name(struct key *keyring)
 
 		write_lock(&keyring_name_lock);
 
-		if (!keyring_name_hash[bucket].next)
-			INIT_LIST_HEAD(&keyring_name_hash[bucket]);
+		if (!ns->keyring_name_hash[bucket].next)
+			INIT_LIST_HEAD(&ns->keyring_name_hash[bucket]);
 
 		list_add_tail(&keyring->name_link,
-			      &keyring_name_hash[bucket]);
+			      &ns->keyring_name_hash[bucket]);
 
 		write_unlock(&keyring_name_lock);
 	}
@@ -148,9 +148,11 @@  static void keyring_free_preparse(struct key_preparsed_payload *prep)
 static int keyring_instantiate(struct key *keyring,
 			       struct key_preparsed_payload *prep)
 {
+	struct user_namespace *ns = (void *)prep->data;
+
 	assoc_array_init(&keyring->keys);
 	/* make the keyring available by name if it has one */
-	keyring_publish_name(keyring);
+	keyring_publish_name(ns, keyring);
 	return 0;
 }
 
@@ -497,13 +499,14 @@  struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,
 					       const union key_payload *),
 			  struct key *dest)
 {
+	void *data = cred->user_ns;
 	struct key *keyring;
 	int ret;
 
 	keyring = key_alloc(&key_type_keyring, description,
 			    uid, gid, cred, perm, flags, restrict_link);
 	if (!IS_ERR(keyring)) {
-		ret = key_instantiate_and_link(keyring, NULL, 0, dest, NULL);
+		ret = key_instantiate_and_link(keyring, data, 0, dest, NULL);
 		if (ret < 0) {
 			key_put(keyring);
 			keyring = ERR_PTR(ret);
@@ -997,7 +1000,8 @@  key_ref_t find_key_to_update(key_ref_t keyring_ref,
  * Returns a pointer to the keyring with the keyring's refcount having being
  * incremented on success.  -ENOKEY is returned if a key could not be found.
  */
-struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
+struct key *find_keyring_by_name(struct user_namespace *ns,
+				 const char *name, bool skip_perm_check)
 {
 	struct key *keyring;
 	int bucket;
@@ -1009,16 +1013,13 @@  struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
 
 	read_lock(&keyring_name_lock);
 
-	if (keyring_name_hash[bucket].next) {
+	if (ns->keyring_name_hash[bucket].next) {
 		/* search this hash bucket for a keyring with a matching name
 		 * that's readable and that hasn't been revoked */
 		list_for_each_entry(keyring,
-				    &keyring_name_hash[bucket],
+				    &ns->keyring_name_hash[bucket],
 				    name_link
 				    ) {
-			if (!kuid_has_mapping(current_user_ns(), keyring->user->uid))
-				continue;
-
 			if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
 				continue;
 
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 40a885239782..5f527dcb4e79 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -72,7 +72,7 @@  int install_user_keyrings(void)
 		 *   may have been destroyed by setuid */
 		sprintf(buf, "_uid.%u", uid);
 
-		uid_keyring = find_keyring_by_name(buf, true);
+		uid_keyring = find_keyring_by_name(cred->user_ns, buf, true);
 		if (IS_ERR(uid_keyring)) {
 			uid_keyring = keyring_alloc(buf, user->uid, INVALID_GID,
 						    cred, user_keyring_perm,
@@ -88,7 +88,7 @@  int install_user_keyrings(void)
 		 * already) */
 		sprintf(buf, "_uid_ses.%u", uid);
 
-		session_keyring = find_keyring_by_name(buf, true);
+		session_keyring = find_keyring_by_name(cred->user_ns, buf, true);
 		if (IS_ERR(session_keyring)) {
 			session_keyring =
 				keyring_alloc(buf, user->uid, INVALID_GID,
@@ -783,7 +783,7 @@  long join_session_keyring(const char *name)
 	mutex_lock(&key_session_mutex);
 
 	/* look for an existing keyring of this name */
-	keyring = find_keyring_by_name(name, false);
+	keyring = find_keyring_by_name(old->user_ns, name, false);
 	if (PTR_ERR(keyring) == -ENOKEY) {
 		/* not found - try and create a new one */
 		keyring = keyring_alloc(