[PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Anvik Loitongbam]

construct_alloc_key() pre-links a newly allocated key into the destination
keyring with __key_link(), bypassing __key_link_check_restriction(). If
the request-key helper instantiates the key with ringid 0 (or any keyring
other than the restricted destination), the key becomes positive while the
provisional link in a restricted keyring remains, so the restriction never
sees the payload, potentially resulting in a policy violation, where a
user with write access can inject keys which are restricted on the restricted
keyring.

Restricted destinations lose the provisional-link deduplication while the
key is being constructed. Callers that pass
KEY_ALLOC_BYPASS_RESTRICTION keep the existing provisional-link behavior.

Fixes: 5ac7eace2d00 ("KEYS: Add a check to restrict new links into a keyring")
Cc: stable@vger.kernel.org
Signed-off-by: Anvik Loitongbam <permissiontogallop@gmail.com>
---
 security/keys/request_key.c | 97 ++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 12 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e6ba2d054399..3de6c9160651 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -370,18 +370,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 			       struct key *dest_keyring,
 			       unsigned long flags,
 			       struct key_user *user,
-			       struct key **_key)
+			       struct key **_key,
+			       bool *defer_link)
 {
 	struct assoc_array_edit *edit = NULL;
 	struct key *key;
 	key_perm_t perm;
 	key_ref_t key_ref;
+	bool link_locked = false;
 	int ret;
 
 	kenter("%s,%s,,,",
 	       ctx->index_key.type->name, ctx->index_key.description);
 
 	*_key = NULL;
+	*defer_link = false;
 	mutex_lock(&user->cons_lock);
 
 	perm = KEY_POS_VIEW | KEY_POS_SEARCH | KEY_POS_LINK | KEY_POS_SETATTR;
@@ -404,6 +407,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 		ret = __key_link_lock(dest_keyring, &key->index_key);
 		if (ret < 0)
 			goto link_lock_failed;
+
+		/*
+		 * A restricted keyring should not take an uninstantiated key,
+		 * the restriction has to be evaluated with the payload,
+		 * which is only available after construction. Skip the
+		 * provisional link and add the key with key_link() later,
+		 * unless the caller explicitly asked to bypass restrictions.
+		 */
+		if (dest_keyring->restrict_link &&
+		    !(flags & KEY_ALLOC_BYPASS_RESTRICTION)) {
+			__key_link_end(dest_keyring, &key->index_key, NULL);
+			*defer_link = true;
+		} else {
+			link_locked = true;
+		}
 	}
 
 	/*
@@ -424,7 +442,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	if (!IS_ERR(key_ref))
 		goto key_already_present;
 
-	if (dest_keyring) {
+	if (dest_keyring && link_locked) {
 		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
 		if (ret < 0)
 			goto link_alloc_failed;
@@ -432,7 +450,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	}
 
 	mutex_unlock(&key_construction_mutex);
-	if (dest_keyring)
+	if (link_locked)
 		__key_link_end(dest_keyring, &key->index_key, edit);
 	mutex_unlock(&user->cons_lock);
 	*_key = key;
@@ -446,15 +464,44 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	mutex_unlock(&key_construction_mutex);
 	key = key_ref_to_ptr(key_ref);
 	if (dest_keyring) {
-		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
-		if (ret < 0)
-			goto link_alloc_failed_unlocked;
-		ret = __key_link_check_live_key(dest_keyring, key);
-		if (ret == 0)
-			__key_link(dest_keyring, key, &edit);
-		__key_link_end(dest_keyring, &key->index_key, edit);
-		if (ret < 0)
+		/*
+		 * The outer search checks Link permission on a key it finds.
+		 * The second search must do the same; otherwise a key that
+		 * appeared in the race window could be linked without the
+		 * caller holding Link permission on it.
+		 */
+		ret = key_task_permission(key_ref, ctx->cred, KEY_NEED_LINK);
+		if (ret < 0) {
+			if (link_locked)
+				__key_link_end(dest_keyring, &key->index_key,
+					       NULL);
 			goto link_check_failed;
+		}
+		if (*defer_link) {
+			/*
+			 * The key may still be under construction, in which
+			 * case its payload cannot be checked yet.  Do not add
+			 * a provisional link to a restricted keyring; only an
+			 * already instantiated key can be checked here.
+			 */
+			if (!key_is_positive(key)) {
+				ret = -EPERM;
+				goto link_check_failed;
+			}
+			ret = key_link(dest_keyring, key);
+			if (ret < 0)
+				goto link_check_failed;
+		} else {
+			ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
+			if (ret < 0)
+				goto link_alloc_failed_unlocked;
+			ret = __key_link_check_live_key(dest_keyring, key);
+			if (ret == 0)
+				__key_link(dest_keyring, key, &edit);
+			__key_link_end(dest_keyring, &key->index_key, edit);
+			if (ret < 0)
+				goto link_check_failed;
+		}
 	}
 	mutex_unlock(&user->cons_lock);
 	*_key = key;
@@ -495,6 +542,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 {
 	struct key_user *user;
 	struct key *key;
+	bool defer_link;
 	int ret;
 
 	kenter("");
@@ -512,7 +560,8 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 		goto error_put_dest_keyring;
 	}
 
-	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key);
+	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key,
+				  &defer_link);
 	key_user_put(user);
 
 	if (ret == 0) {
@@ -522,6 +571,30 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 			kdebug("cons failed");
 			goto construction_failed;
 		}
+
+		/*
+		 * The key was not provisionally linked because the
+		 * destination keyring is restricted. Link it now that the
+		 * payload is available so the restriction can be checked.
+		 */
+		if (defer_link) {
+			/*
+			 * A helper may have negatively instantiated the key
+			 * (e.g. KEYCTL_REJECT) and returned success.  A
+			 * restricted keyring must not cache a negative key;
+			 * key_reject_and_link() rejects those too.
+			 */
+			if (!key_is_positive(key)) {
+				ret = -EPERM;
+				key_put(key);
+				goto error_put_dest_keyring;
+			}
+			ret = key_link(dest_keyring, key);
+			if (ret < 0) {
+				key_put(key);
+				goto error_put_dest_keyring;
+			}
+		}
 	} else if (ret == -EINPROGRESS) {
 		ret = 0;
 	} else {
-- 
2.56.0

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KEYS: request_key_and_link() links uninstantiated key to restricted keyring
--

commit 97697a25e3b571e267998f93eacf1015231cd118
Author: Anvik Loitongbam <permissiontogallop@gmail.com>

KEYS: request_key: enforce keyring restrictions on constructed keys

This commit defers linking a newly allocated key into a restricted destination
keyring during key construction until the key has been instantiated and its
payload can be checked against the keyring's restriction.

> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index fa2bb9f2f5381..a47e6a72ded89 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c

[ ... ]

> @@ -446,15 +464,44 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	mutex_unlock(&key_construction_mutex);
>  	key = key_ref_to_ptr(key_ref);
>  	if (dest_keyring) {
> -		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> -		if (ret < 0)
> -			goto link_alloc_failed_unlocked;
> -		ret = __key_link_check_live_key(dest_keyring, key);
> -		if (ret == 0)
> -			__key_link(dest_keyring, key, &edit);
> -		__key_link_end(dest_keyring, &key->index_key, edit);
> -		if (ret < 0)
> +		/*
> +		 * The outer search checks Link permission on a key it finds.
> +		 * The second search must do the same; otherwise a key that
> +		 * appeared in the race window could be linked without the
> +		 * caller holding Link permission on it.
> +		 */
> +		ret = key_task_permission(key_ref, ctx->cred, KEY_NEED_LINK);
> +		if (ret < 0) {
> +			if (link_locked)
> +				__key_link_end(dest_keyring, &key->index_key,
> +					       NULL);
>  			goto link_check_failed;
> +		}
> +		if (*defer_link) {
> +			/*
> +			 * The key may still be under construction, in which
> +			 * case its payload cannot be checked yet.  Do not add
> +			 * a provisional link to a restricted keyring; only an
> +			 * already instantiated key can be checked here.
> +			 */
> +			if (!key_is_positive(key)) {
> +				ret = -EPERM;
> +				goto link_check_failed;
> +			}
> +			ret = key_link(dest_keyring, key);
> +			if (ret < 0)
> +				goto link_check_failed;

[Severity: High]
Can an uninstantiated key still be linked into a restricted keyring through
the outer search in request_key_and_link()?

In keyring_search_iterator(), only negative keys (state < 0) are skipped when
KEYRING_SEARCH_DO_STATE_CHECK is set, so search_process_keyrings_rcu() returns
keys that are still in KEY_IS_UNINSTANTIATED (0) state:

security/keys/keyring.c:keyring_search_iterator() {
    ...
	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
		/* we set a different error code if we pass a negative key */
		if (state < 0) {
			ctx->result = ERR_PTR(state);
			kleave(" = %d [neg]", ctx->skipped_ret);
			goto skipped;
		}
	}

	/* Found */
	ctx->result = make_key_ref(key, ctx->possessed);
	kleave(" = 1 [found]");
	return 1;
}

If a task initiates a request_key() call targeting an unrestricted process
keyring (such as the session, process, or thread keyring),
construct_alloc_key() provisionally links the uninstantiated key into that
keyring while the /sbin/request-key upcall is running.

If another thread or helper then calls request_key() for the same key with a
restricted dest_keyring before instantiation completes, the outer
search_process_keyrings_rcu() in request_key_and_link() finds that
uninstantiated key and passes it straight to key_link() before
wait_for_key_construction() is reached:

security/keys/request_key.c:request_key_and_link() {
    ...
	rcu_read_lock();
	key_ref = search_process_keyrings_rcu(&ctx);
	rcu_read_unlock();

	if (!IS_ERR(key_ref)) {
		...
		key = key_ref_to_ptr(key_ref);
		if (dest_keyring) {
			ret = key_link(dest_keyring, key);
    ...
}

Because __key_link_check_restriction() in security/keys/keyring.c also does
not check key_is_positive(key) before passing &key->payload to
keyring->restrict_link->check(), doesn't this cause a few issues?

First, if dest_keyring uses a restriction callback that only validates
key->type and relies on key->type->instantiate() to verify the payload (such
as restrict_link_for_blacklist() in certs/blacklist.c), or if
restrict_link->check is NULL, key_link() will link the uninstantiated key into
the restricted keyring, and the helper can subsequently instantiate or
negatively instantiate the key with ringid 0, leaving an unverified or
negative key linked in the restricted keyring.

Second, if the helper is concurrently instantiating the key with ringid 0 in
__key_instantiate_and_link() (which holds key_construction_mutex, not
dest_keyring->sem), key->type->instantiate() populates key->payload before
mark_key_instantiated() executes smp_store_release(&key->state,
KEY_IS_POSITIVE). Calling key_link() without first checking
key_is_positive(key) (which provides the pairing smp_load_acquire(&key->state))
allows restrict_link->check() to read key->payload concurrently with
instantiate().

Third, in construct_alloc_key() itself, because the first thread drops
user->cons_lock before calling construct_key(), a second thread of the same
user racing on the same key with a restricted dest_keyring can find the
uninstantiated key via the inner search and enter key_already_present while
!key_is_positive(key) is still true, causing it to fail immediately with
-EPERM instead of waiting for construction and then linking.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030173930.40948-1-permissiontogallop@gmail.com?part=1

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

On Fri, Oct 30, 2026 at 11:09:30PM +0530, Anvik Loitongbam wrote:
> construct_alloc_key() pre-links a newly allocated key into the destination
> keyring with __key_link(), bypassing __key_link_check_restriction(). If
> the request-key helper instantiates the key with ringid 0 (or any keyring
> other than the restricted destination), the key becomes positive while the
> provisional link in a restricted keyring remains, so the restriction never
> sees the payload, potentially resulting in a policy violation, where a
> user with write access can inject keys which are restricted on the restricted
> keyring.

What does "key becoming positive" mean here broken down?

Also I have really hard time on reading the second paragrah as it is,
let me count, over five lines long. I honestly cannot say that I
understood what I read. I tried it three times.

If you understand your fix, you should be able to nail it.

> 
> Restricted destinations lose the provisional-link deduplication while the
> key is being constructed. Callers that pass
> KEY_ALLOC_BYPASS_RESTRICTION keep the existing provisional-link behavior.

The second paragraph should be reformatted and there is no rationale
or descriptio for the code change.

Br, Jarkko