| Message ID | 20180724141515.7478-2-kleber.souza@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix for CVE-2017-18270 | expand |
On 24.07.2018 16:15, Kleber Sacilotto de Souza wrote: > From: Eric Biggers <ebiggers@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@vger.kernel.org> [v2.6.26+] > Signed-off-by: Eric Biggers <ebiggers@google.com> > Signed-off-by: David Howells <dhowells@redhat.com> > > CVE-2017-18270 > (backported from commit 237bbd29f7a049d310d907f4b2716a7feef9abf3) > [ klebers: adjusted context. ] > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-by: Stefan Bader <stefan.bader@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; >
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;