KEYS: Don't permit request_key() to construct a new keyring

PATCH In review Eric Biggers 3 participants 4 messages Dec 15, 2017 – Dec 18, 2017 Stable branch

lore

Share & tools

lore

Eric Biggers

author

2017-12-15 20:46 UTC #0 lore raw

PATCH 1/2 KEYS: Don't permit request_key() to construct a new keyring

From: David Howells <[email protected]>

commit 911b79cde95c7da0ec02f48105358a36636b7a71 upstream. [Please apply to 3.18-stable and 4.1-stable.]

If request_key() is used to find a keyring, only do the search part - don't do the construction part if the keyring was not found by the search. We don't really want keyrings in the negative instantiated state since the rejected/negative instantiation error value in the payload is unioned with keyring metadata.

Now the kernel gives an error:

request_key("keyring", "#selinux,bdekeyring", "keyring", KEY_SPEC_USER_SESSION_KEYRING) = -1 EPERM (Operation not permitted)

Signed-off-by David Howells <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 security/keys/request_key.c | 3 +++
 1 file changed, 3 insertions(+)

security/keys/request_key.c +3 -0

index 486ef6fa393b..0d6253124278 100644
@@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 
 	kenter("");
 
+	if (ctx->index_key.type == &key_type_keyring)
+		return ERR_PTR(-EPERM);
+	
 	user = key_user_lookup(current_fsuid());
 	if (!user)
 		return ERR_PTR(-ENOMEM);

Signature · 1 line

2.15.1.504.g5279b80103-goog

Eric Biggers

author

2017-12-15 20:46 UTC #1 lore raw

PATCH 2/2 KEYS: add missing permission check for request_key() destination

From: Eric Biggers <[email protected]>

commit 4dca6ea1d9432052afb06baf2e3ae78188a4410b upstream. [Please apply to 3.18-stable and 4.1-stable.]

When the request_key() syscall is not passed a destination keyring, it links the requested key (if constructed) into the "default" request-key keyring. This should require Write permission to the keyring. However, there is actually no permission check.

This can be abused to add keys to any keyring to which only Search permission is granted. This is because Search permission allows joining the keyring. keyctl_set_reqkey_keyring(KEY_REQKEY_DEFL_SESSION_KEYRING) then will set the default request-key keyring to the session keyring. Then, request_key() can be used to add keys to the keyring.

Both negatively and positively instantiated keys can be added using this method. Adding negative keys is trivial. Adding a positive key is a bit trickier. It requires that either /sbin/request-key positively instantiates the key, or that another thread adds the key to the process keyring at just the right time, such that request_key() misses it initially but then finds it in construct_alloc_key().

Fix this bug by checking for Write permission to the keyring in construct_get_dest_keyring() when the default keyring is being used.

We don't do the permission check for non-default keyrings because that was already done by the earlier call to lookup_user_key(). Also, request_key_and_link() is currently passed a 'struct key *' rather than a key_ref_t, so the "possessed" bit is unavailable.

We also don't do the permission check for the "requestor keyring", to continue to support the use case described by commit 8bbf4976b59f ("KEYS: Alter use of key instantiation link-to-keyring argument") where /sbin/request-key recursively calls request_key() to add keys to the original requestor's destination keyring. (I don't know of any users who actually do that, though...)

Fixes 3e30148c3d52 ("[PATCH] Keys: Make request-key create an authorisation key")

Signed-off-by Eric Biggers <[email protected]>

Signed-off-by David Howells <[email protected]>

---
 security/keys/request_key.c | 48 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

security/keys/request_key.c +38 ~~-10~~

index 0d6253124278..edc367ef0bd6 100644
@@ -250,11 +250,12 @@ static int construct_key(struct key *key, const void *callout_info,
  * The keyring selected is returned with an extra reference upon it which the
  * caller must release.
  */
-static void construct_get_dest_keyring(struct key **_dest_keyring)
+static int construct_get_dest_keyring(struct key **_dest_keyring)
 {
 	struct request_key_auth *rka;
 	const struct cred *cred = current_cred();
 	struct key *dest_keyring = *_dest_keyring, *authkey;
+	int ret;
 
 	kenter("%p", dest_keyring);

@@ -263,6 +264,8 @@ static void construct_get_dest_keyring(struct key **_dest_keyring)
 		/* the caller supplied one */
 		key_get(dest_keyring);
 	} else {
+		bool do_perm_check = true;
+
 		/* use a default keyring; falling through the cases until we
 		 * find one that we actually have */
 		switch (cred->jit_keyring) {

@@ -277,8 +280,10 @@ static void construct_get_dest_keyring(struct key **_dest_keyring)
 					dest_keyring =
 						key_get(rka->dest_keyring);
 				up_read(&authkey->sem);
-				if (dest_keyring)
+				if (dest_keyring) {
+					do_perm_check = false;
 					break;
+				}
 			}
 
 		case KEY_REQKEY_DEFL_THREAD_KEYRING:

@@ -313,11 +318,29 @@ static void construct_get_dest_keyring(struct key **_dest_keyring)
 		default:
 			BUG();
 		}
+
+		/*
+		 * Require Write permission on the keyring.  This is essential
+		 * because the default keyring may be the session keyring, and
+		 * joining a keyring only requires Search permission.
+		 *
+		 * However, this check is skipped for the "requestor keyring" so
+		 * that /sbin/request-key can itself use request_key() to add
+		 * keys to the original requestor's destination keyring.
+		 */
+		if (dest_keyring && do_perm_check) {
+			ret = key_permission(make_key_ref(dest_keyring, 1),
+					     KEY_NEED_WRITE);
+			if (ret) {
+				key_put(dest_keyring);
+				return ret;
+			}
+		}
 	}
 
 	*_dest_keyring = dest_keyring;
 	kleave(" [dk %d]", key_serial(dest_keyring));
-	return;
+	return 0;
 }
 
 /*

@@ -442,12 +465,16 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 
 	if (ctx->index_key.type == &key_type_keyring)
 		return ERR_PTR(-EPERM);
-	
-	user = key_user_lookup(current_fsuid());
-	if (!user)
-		return ERR_PTR(-ENOMEM);
 
-	construct_get_dest_keyring(&dest_keyring);
+	ret = construct_get_dest_keyring(&dest_keyring);
+	if (ret)
+		goto error;
+
+	user = key_user_lookup(current_fsuid());
+	if (!user) {
+		ret = -ENOMEM;
+		goto error_put_dest_keyring;
+	}
 
 	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key);
 	key_user_put(user);

@@ -462,7 +489,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 	} else if (ret == -EINPROGRESS) {
 		ret = 0;
 	} else {
-		goto couldnt_alloc_key;
+		goto error_put_dest_keyring;
 	}
 
 	key_put(dest_keyring);

@@ -472,8 +499,9 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 construction_failed:
 	key_negate_and_link(key, key_negative_timeout, NULL, NULL);
 	key_put(key);
-couldnt_alloc_key:
+error_put_dest_keyring:
 	key_put(dest_keyring);
+error:
 	kleave(" = %d", ret);
 	return ERR_PTR(ret);
 }

Signature · 1 line

2.15.1.504.g5279b80103-goog

Greg KH

maintainer

2017-12-18 12:06 UTC #2 lore raw

Re: [PATCH 2/2 for 3.18/4.1] KEYS: add missing permission check for request_key() destination

Quoted 38 lines · Eric Biggers

From: Eric Biggers <[email protected]>
commit 4dca6ea1d9432052afb06baf2e3ae78188a4410b upstream. [Please apply to 3.18-stable and 4.1-stable.]
When the request_key() syscall is not passed a destination keyring, it links the requested key (if constructed) into the "default" request-key keyring. This should require Write permission to the keyring. However, there is actually no permission check.
This can be abused to add keys to any keyring to which only Search permission is granted. This is because Search permission allows joining the keyring. keyctl_set_reqkey_keyring(KEY_REQKEY_DEFL_SESSION_KEYRING) then will set the default request-key keyring to the session keyring. Then, request_key() can be used to add keys to the keyring.
Both negatively and positively instantiated keys can be added using this method. Adding negative keys is trivial. Adding a positive key is a bit trickier. It requires that either /sbin/request-key positively instantiates the key, or that another thread adds the key to the process keyring at just the right time, such that request_key() misses it initially but then finds it in construct_alloc_key().
Fix this bug by checking for Write permission to the keyring in construct_get_dest_keyring() when the default keyring is being used.
We don't do the permission check for non-default keyrings because that was already done by the earlier call to lookup_user_key(). Also, request_key_and_link() is currently passed a 'struct key *' rather than a key_ref_t, so the "possessed" bit is unavailable.
We also don't do the permission check for the "requestor keyring", to continue to support the use case described by commit 8bbf4976b59f ("KEYS: Alter use of key instantiation link-to-keyring argument") where /sbin/request-key recursively calls request_key() to add keys to the original requestor's destination keyring. (I don't know of any users who actually do that, though...)

Quoted 7 lines

Fixes 3e30148c3d52 ("[PATCH] Keys: Make request-key create an authorisation key")
Signed-off-by Eric Biggers <[email protected]>
Signed-off-by David Howells <[email protected]>
---
 security/keys/request_key.c | 48 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

Thanks for the patch, but this was already in the 3.18.88 release.

greg k-h

Greg KH

2017-12-18 12:07 UTC #3 lore raw

In reply to Eric Biggers

Quoted 13 lines · Eric Biggers

From: David Howells <[email protected]>
commit 911b79cde95c7da0ec02f48105358a36636b7a71 upstream. [Please apply to 3.18-stable and 4.1-stable.]
If request_key() is used to find a keyring, only do the search part - don't do the construction part if the keyring was not found by the search. We don't really want keyrings in the negative instantiated state since the rejected/negative instantiation error value in the payload is unioned with keyring metadata.
Now the kernel gives an error:

Quoted 7 lines

request_key("keyring", "#selinux,bdekeyring", "keyring", KEY_SPEC_USER_SESSION_KEYRING) = -1 EPERM (Operation not permitted)
Signed-off-by David Howells <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
---
 security/keys/request_key.c | 3 +++
 1 file changed, 3 insertions(+)

Now applied to 3.18, thanks.

greg k-h