ACK: [SRU][Trusty][PATCH 1/1] KEYS: prevent creating a different user's keyrings
Stefan Bader
stefan.bader at canonical.com
Wed Jul 25 15:20:32 UTC 2018
On 24.07.2018 16:15, Kleber Sacilotto de Souza wrote:
> From: Eric Biggers <ebiggers at google.com>
>
> It was possible for an unprivileged user to create the user and user
> session keyrings for another user. For example:
>
> sudo -u '#3000' sh -c 'keyctl add keyring _uid.4000 "" @u
> keyctl add keyring _uid_ses.4000 "" @u
> sleep 15' &
> sleep 1
> sudo -u '#4000' keyctl describe @u
> sudo -u '#4000' keyctl describe @us
>
> This is problematic because these "fake" keyrings won't have the right
> permissions. In particular, the user who created them first will own
> them and will have full access to them via the possessor permissions,
> which can be used to compromise the security of a user's keys:
>
> -4: alswrv-----v------------ 3000 0 keyring: _uid.4000
> -5: alswrv-----v------------ 3000 0 keyring: _uid_ses.4000
>
> Fix it by marking user and user session keyrings with a flag
> KEY_FLAG_UID_KEYRING. Then, when searching for a user or user session
> keyring by name, skip all keyrings that don't have the flag set.
>
> Fixes: 69664cf16af4 ("keys: don't generate user and user session keyrings unless they're accessed")
> Cc: <stable at vger.kernel.org> [v2.6.26+]
> Signed-off-by: Eric Biggers <ebiggers at google.com>
> Signed-off-by: David Howells <dhowells at redhat.com>
>
> CVE-2017-18270
> (backported from commit 237bbd29f7a049d310d907f4b2716a7feef9abf3)
> [ klebers: adjusted context. ]
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
> include/linux/key.h | 2 ++
> security/keys/internal.h | 2 +-
> security/keys/key.c | 2 ++
> security/keys/keyring.c | 23 ++++++++++++++---------
> security/keys/process_keys.c | 8 ++++++--
> 5 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 80d677483e31..76153d5c7fae 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -170,6 +170,7 @@ struct key {
> #define KEY_FLAG_INVALIDATED 7 /* set if key has been invalidated */
> #define KEY_FLAG_TRUSTED 8 /* set if key is trusted */
> #define KEY_FLAG_TRUSTED_ONLY 9 /* set if keyring only accepts links to trusted keys */
> +#define KEY_FLAG_UID_KEYRING 11 /* set if key is a user or user session keyring */
>
> /* the key type and key description string
> * - the desc is used to match a key against search criteria
> @@ -221,6 +222,7 @@ extern struct key *key_alloc(struct key_type *type,
> #define KEY_ALLOC_QUOTA_OVERRUN 0x0001 /* add to quota, permit even if overrun */
> #define KEY_ALLOC_NOT_IN_QUOTA 0x0002 /* not in quota */
> #define KEY_ALLOC_TRUSTED 0x0004 /* Key should be flagged as trusted */
> +#define KEY_ALLOC_UID_KEYRING 0x0010 /* allocating a user or user session keyring */
>
> extern void key_revoke(struct key *key);
> extern void key_invalidate(struct key *key);
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 6d5c72e6c717..8859fcff15bc 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -138,7 +138,7 @@ extern key_ref_t keyring_search_aux(key_ref_t keyring_ref,
> extern key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx);
> extern key_ref_t search_process_keyrings(struct keyring_search_context *ctx);
>
> -extern struct key *find_keyring_by_name(const char *name, bool skip_perm_check);
> +extern struct key *find_keyring_by_name(const char *name, bool uid_keyring);
>
> extern int install_user_keyrings(void);
> extern int install_thread_keyring_to_cred(struct cred *);
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 9478d668f874..7ee46477082b 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -298,6 +298,8 @@ struct key *key_alloc(struct key_type *type, const char *desc,
> key->flags |= 1 << KEY_FLAG_IN_QUOTA;
> if (flags & KEY_ALLOC_TRUSTED)
> key->flags |= 1 << KEY_FLAG_TRUSTED;
> + if (flags & KEY_ALLOC_UID_KEYRING)
> + key->flags |= 1 << KEY_FLAG_UID_KEYRING;
>
> #ifdef KEY_DEBUGGING
> key->magic = KEY_DEBUG_MAGIC;
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 13496c97c150..dd463826515f 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -936,15 +936,15 @@ found:
> /*
> * Find a keyring with the specified name.
> *
> - * All named keyrings in the current user namespace are searched, provided they
> - * grant Search permission directly to the caller (unless this check is
> - * skipped). Keyrings whose usage points have reached zero or who have been
> - * revoked are skipped.
> + * Only keyrings that have nonzero refcount, are not revoked, and are owned by a
> + * user in the current user namespace are considered. If @uid_keyring is %true,
> + * the keyring additionally must have been allocated as a user or user session
> + * keyring; otherwise, it must grant Search permission directly to the caller.
> *
> * Returns a pointer to the keyring with the keyring's refcount having being
> * incremented on success. -ENOKEY is returned if a key could not be found.
> */
> -struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
> +struct key *find_keyring_by_name(const char *name, bool uid_keyring)
> {
> struct key *keyring;
> int bucket;
> @@ -972,10 +972,15 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
> if (strcmp(keyring->description, name) != 0)
> continue;
>
> - if (!skip_perm_check &&
> - key_permission(make_key_ref(keyring, 0),
> - KEY_SEARCH) < 0)
> - continue;
> + if (uid_keyring) {
> + if (!test_bit(KEY_FLAG_UID_KEYRING,
> + &keyring->flags))
> + continue;
> + } else {
> + if (key_permission(make_key_ref(keyring, 0),
> + KEY_SEARCH) < 0)
> + continue;
> + }
>
> /* we've got a match but we might end up racing with
> * key_cleanup() if the keyring is currently 'dead'
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 18bad7caf602..f83e0ae7df16 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -76,7 +76,9 @@ int install_user_keyrings(void)
> if (IS_ERR(uid_keyring)) {
> uid_keyring = keyring_alloc(buf, user->uid, INVALID_GID,
> cred, user_keyring_perm,
> - KEY_ALLOC_IN_QUOTA, NULL);
> + KEY_ALLOC_UID_KEYRING |
> + KEY_ALLOC_IN_QUOTA,
> + NULL);
> if (IS_ERR(uid_keyring)) {
> ret = PTR_ERR(uid_keyring);
> goto error;
> @@ -92,7 +94,9 @@ int install_user_keyrings(void)
> session_keyring =
> keyring_alloc(buf, user->uid, INVALID_GID,
> cred, user_keyring_perm,
> - KEY_ALLOC_IN_QUOTA, NULL);
> + KEY_ALLOC_UID_KEYRING |
> + KEY_ALLOC_IN_QUOTA,
> + NULL);
> if (IS_ERR(session_keyring)) {
> ret = PTR_ERR(session_keyring);
> goto error_release;
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180725/8ddba415/attachment.sig>
More information about the kernel-team
mailing list