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

[sashiko-bot@kernel.org]

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