Skip to content

Commit 4dca6ea

Browse files
ebiggersdhowells
authored andcommitted
KEYS: add missing permission check for request_key() destination
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 8bbf497 ("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: 3e30148 ("[PATCH] Keys: Make request-key create an authorisation key") Cc: <stable@vger.kernel.org> # v2.6.13+ Signed-off-by: Eric Biggers <ebiggers@google.com> Signed-off-by: David Howells <dhowells@redhat.com>
1 parent a2d8737 commit 4dca6ea

File tree

1 file changed

+37
-9
lines changed

1 file changed

+37
-9
lines changed

Diff for: security/keys/request_key.c

+37-9
Original file line numberDiff line numberDiff line change
@@ -251,11 +251,12 @@ static int construct_key(struct key *key, const void *callout_info,
251251
* The keyring selected is returned with an extra reference upon it which the
252252
* caller must release.
253253
*/
254-
static void construct_get_dest_keyring(struct key **_dest_keyring)
254+
static int construct_get_dest_keyring(struct key **_dest_keyring)
255255
{
256256
struct request_key_auth *rka;
257257
const struct cred *cred = current_cred();
258258
struct key *dest_keyring = *_dest_keyring, *authkey;
259+
int ret;
259260

260261
kenter("%p", dest_keyring);
261262

@@ -264,6 +265,8 @@ static void construct_get_dest_keyring(struct key **_dest_keyring)
264265
/* the caller supplied one */
265266
key_get(dest_keyring);
266267
} else {
268+
bool do_perm_check = true;
269+
267270
/* use a default keyring; falling through the cases until we
268271
* find one that we actually have */
269272
switch (cred->jit_keyring) {
@@ -278,8 +281,10 @@ static void construct_get_dest_keyring(struct key **_dest_keyring)
278281
dest_keyring =
279282
key_get(rka->dest_keyring);
280283
up_read(&authkey->sem);
281-
if (dest_keyring)
284+
if (dest_keyring) {
285+
do_perm_check = false;
282286
break;
287+
}
283288
}
284289

285290
case KEY_REQKEY_DEFL_THREAD_KEYRING:
@@ -314,11 +319,29 @@ static void construct_get_dest_keyring(struct key **_dest_keyring)
314319
default:
315320
BUG();
316321
}
322+
323+
/*
324+
* Require Write permission on the keyring. This is essential
325+
* because the default keyring may be the session keyring, and
326+
* joining a keyring only requires Search permission.
327+
*
328+
* However, this check is skipped for the "requestor keyring" so
329+
* that /sbin/request-key can itself use request_key() to add
330+
* keys to the original requestor's destination keyring.
331+
*/
332+
if (dest_keyring && do_perm_check) {
333+
ret = key_permission(make_key_ref(dest_keyring, 1),
334+
KEY_NEED_WRITE);
335+
if (ret) {
336+
key_put(dest_keyring);
337+
return ret;
338+
}
339+
}
317340
}
318341

319342
*_dest_keyring = dest_keyring;
320343
kleave(" [dk %d]", key_serial(dest_keyring));
321-
return;
344+
return 0;
322345
}
323346

324347
/*
@@ -444,11 +467,15 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
444467
if (ctx->index_key.type == &key_type_keyring)
445468
return ERR_PTR(-EPERM);
446469

447-
user = key_user_lookup(current_fsuid());
448-
if (!user)
449-
return ERR_PTR(-ENOMEM);
470+
ret = construct_get_dest_keyring(&dest_keyring);
471+
if (ret)
472+
goto error;
450473

451-
construct_get_dest_keyring(&dest_keyring);
474+
user = key_user_lookup(current_fsuid());
475+
if (!user) {
476+
ret = -ENOMEM;
477+
goto error_put_dest_keyring;
478+
}
452479

453480
ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key);
454481
key_user_put(user);
@@ -463,7 +490,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
463490
} else if (ret == -EINPROGRESS) {
464491
ret = 0;
465492
} else {
466-
goto couldnt_alloc_key;
493+
goto error_put_dest_keyring;
467494
}
468495

469496
key_put(dest_keyring);
@@ -473,8 +500,9 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
473500
construction_failed:
474501
key_negate_and_link(key, key_negative_timeout, NULL, NULL);
475502
key_put(key);
476-
couldnt_alloc_key:
503+
error_put_dest_keyring:
477504
key_put(dest_keyring);
505+
error:
478506
kleave(" = %d", ret);
479507
return ERR_PTR(ret);
480508
}

0 commit comments

Comments
 (0)