From a2d8737d5c781d388b553dd085b56d7295fffc6c Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:27 +0000 Subject: KEYS: remove unnecessary get/put of explicit dest_keyring In request_key_and_link(), in the case where the dest_keyring was explicitly specified, there is no need to get another reference to dest_keyring before calling key_link(), then drop it afterwards. This is because by definition, we already have a reference to dest_keyring. This change is useful because we'll be making construct_get_dest_keyring() able to return an error code, and we don't want to have to handle that error here for no reason. Signed-off-by: Eric Biggers Signed-off-by: David Howells --- security/keys/request_key.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'security') diff --git a/security/keys/request_key.c b/security/keys/request_key.c index e8036cd0ad54..c6880af8b411 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -546,9 +546,7 @@ struct key *request_key_and_link(struct key_type *type, if (!IS_ERR(key_ref)) { key = key_ref_to_ptr(key_ref); if (dest_keyring) { - construct_get_dest_keyring(&dest_keyring); ret = key_link(dest_keyring, key); - key_put(dest_keyring); if (ret < 0) { key_put(key); key = ERR_PTR(ret); -- cgit v1.2.3-55-g7522 From 4dca6ea1d9432052afb06baf2e3ae78188a4410b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:27 +0000 Subject: 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 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") Cc: # v2.6.13+ Signed-off-by: Eric Biggers Signed-off-by: David Howells --- security/keys/request_key.c | 46 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) (limited to 'security') diff --git a/security/keys/request_key.c b/security/keys/request_key.c index c6880af8b411..114f7408feee 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -251,11 +251,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); @@ -264,6 +265,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) { @@ -278,8 +281,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: @@ -314,11 +319,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; } /* @@ -444,11 +467,15 @@ 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); + ret = construct_get_dest_keyring(&dest_keyring); + if (ret) + goto error; - construct_get_dest_keyring(&dest_keyring); + 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); @@ -463,7 +490,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); @@ -473,8 +500,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); } -- cgit v1.2.3-55-g7522 From 3d1f0255426af7eeb88bf7221ff683b7ca9e4493 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Fri, 8 Dec 2017 15:13:29 +0000 Subject: security: keys: remove redundant assignment to key_ref Variable key_ref is being assigned a value that is never read; key_ref is being re-assigned a few statements later. Hence this assignment is redundant and can be removed. Signed-off-by: Colin Ian King Signed-off-by: David Howells Reviewed-by: James Morris --- security/keys/key.c | 1 - 1 file changed, 1 deletion(-) (limited to 'security') diff --git a/security/keys/key.c b/security/keys/key.c index 66049183ad89..d97c9394b5dd 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -833,7 +833,6 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref, key_check(keyring); - key_ref = ERR_PTR(-EPERM); if (!(flags & KEY_ALLOC_BYPASS_RESTRICTION)) restrict_link = keyring->restrict_link; -- cgit v1.2.3-55-g7522 From 18026d866801d0c52e5550210563222bd6c7191d Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:29 +0000 Subject: KEYS: reject NULL restriction string when type is specified keyctl_restrict_keyring() allows through a NULL restriction when the "type" is non-NULL, which causes a NULL pointer dereference in asymmetric_lookup_restriction() when it calls strcmp() on the restriction string. But no key types actually use a "NULL restriction" to mean anything, so update keyctl_restrict_keyring() to reject it with EINVAL. Reported-by: syzbot Fixes: 97d3aa0f3134 ("KEYS: Add a lookup_restriction function for the asymmetric key type") Cc: # v4.12+ Signed-off-by: Eric Biggers Signed-off-by: David Howells --- security/keys/keyctl.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) (limited to 'security') diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 76d22f726ae4..1ffe60bb2845 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -1588,9 +1588,8 @@ error_keyring: * The caller must have Setattr permission to change keyring restrictions. * * The requested type name may be a NULL pointer to reject all attempts - * to link to the keyring. If _type is non-NULL, _restriction can be - * NULL or a pointer to a string describing the restriction. If _type is - * NULL, _restriction must also be NULL. + * to link to the keyring. In this case, _restriction must also be NULL. + * Otherwise, both _type and _restriction must be non-NULL. * * Returns 0 if successful. */ @@ -1598,7 +1597,6 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type, const char __user *_restriction) { key_ref_t key_ref; - bool link_reject = !_type; char type[32]; char *restriction = NULL; long ret; @@ -1607,31 +1605,29 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type, if (IS_ERR(key_ref)) return PTR_ERR(key_ref); + ret = -EINVAL; if (_type) { - ret = key_get_type_from_user(type, _type, sizeof(type)); - if (ret < 0) + if (!_restriction) goto error; - } - if (_restriction) { - if (!_type) { - ret = -EINVAL; + ret = key_get_type_from_user(type, _type, sizeof(type)); + if (ret < 0) goto error; - } restriction = strndup_user(_restriction, PAGE_SIZE); if (IS_ERR(restriction)) { ret = PTR_ERR(restriction); goto error; } + } else { + if (_restriction) + goto error; } - ret = keyring_restrict(key_ref, link_reject ? NULL : type, restriction); + ret = keyring_restrict(key_ref, _type ? type : NULL, restriction); kfree(restriction); - error: key_ref_put(key_ref); - return ret; } -- cgit v1.2.3-55-g7522