| Message ID | 20170918183703.114134-1-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Biggers <ebiggers3@gmail.com> wrote: > 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. I wonder if it's better just to reject attempts to manually create/join keyrings of such names. PAM uses the implicit creation method of specifying the 'macro' key IDs for these keyrings. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 19, 2017 at 05:05:20PM +0100, David Howells wrote: > Eric Biggers <ebiggers3@gmail.com> wrote: > > > 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. > > I wonder if it's better just to reject attempts to manually create/join > keyrings of such names. PAM uses the implicit creation method of specifying > the 'macro' key IDs for these keyrings. > > David Well, maybe. Whitelists are hard to get right, and it would be a bit ugly having to check the name in both add_key() and join_session_keyring(). And hopefully that would be everything? I think there's also a more fundamental problem with how keyring names work. If you try to join a keyring with a certain name, how are you supposed to know which one you're joining? There can be many keyrings that have the same name; and any unprivileged user can create a keyring with the name, and they can grant everyone SEARCH permission so that their keyring can be joined. So it can be the case that a user is wanting to join a particular keyring, but they actually get a keyring that a malicious user has crafted for them... Also, if period ('.') is meant to be the reserved character in keyring names, why do most of the special names actually start with underscore ('_')? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Biggers <ebiggers3@gmail.com> wrote: > Well, maybe. Whitelists are hard to get right, and it would be a bit ugly > having to check the name in both add_key() and join_session_keyring(). And > hopefully that would be everything? Actually, having thought about it some more, I think your way is better. > I think there's also a more fundamental problem with how keyring names work. > If you try to join a keyring with a certain name, how are you supposed to > know which one you're joining? There can be many keyrings that have the > same name; and any unprivileged user can create a keyring with the name, and > they can grant everyone SEARCH permission so that their keyring can be > joined. So it can be the case that a user is wanting to join a particular > keyring, but they actually get a keyring that a malicious user has crafted > for them... Yeah. With hindsight, I think that firstly, joinable keyrings really need enablement and, secondly, thread, process, session, user and user-session need to have to be non-manually-creatable. However, I'm not sure they can be renamed, since they're searchable and joinable by name and fixing this might break something in userspace (though I should hope that this is unlikely). > Also, if period ('.') is meant to be the reserved character in keyring names, > why do most of the special names actually start with underscore ('_')? '.' wasn't a reserved char originally. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/key.h b/include/linux/key.h index 044114185120..e315e16b6ff8 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -187,6 +187,7 @@ struct key { #define KEY_FLAG_BUILTIN 8 /* set if key is built in to the kernel */ #define KEY_FLAG_ROOT_CAN_INVAL 9 /* set if key can be invalidated by root without permission */ #define KEY_FLAG_KEEP 10 /* set if key should not be removed */ +#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 @@ -243,6 +244,7 @@ extern struct key *key_alloc(struct key_type *type, #define KEY_ALLOC_NOT_IN_QUOTA 0x0002 /* not in quota */ #define KEY_ALLOC_BUILT_IN 0x0004 /* Key is built into kernel */ #define KEY_ALLOC_BYPASS_RESTRICTION 0x0008 /* Override the check on restricted keyrings */ +#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 1c02c6547038..503adbae7b0d 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -141,7 +141,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 83da68d98b40..e5c0896c3a8f 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -302,6 +302,8 @@ struct key *key_alloc(struct key_type *type, const char *desc, key->flags |= 1 << KEY_FLAG_IN_QUOTA; if (flags & KEY_ALLOC_BUILT_IN) key->flags |= 1 << KEY_FLAG_BUILTIN; + 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 de81793f9920..d8d08bd9c159 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -1101,15 +1101,15 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref, /* * 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; @@ -1137,10 +1137,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_NEED_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_NEED_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 86bced9fdbdf..293d3598153b 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -77,7 +77,8 @@ 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, + KEY_ALLOC_UID_KEYRING | + KEY_ALLOC_IN_QUOTA, NULL, NULL); if (IS_ERR(uid_keyring)) { ret = PTR_ERR(uid_keyring); @@ -94,7 +95,8 @@ int install_user_keyrings(void) session_keyring = keyring_alloc(buf, user->uid, INVALID_GID, cred, user_keyring_perm, - KEY_ALLOC_IN_QUOTA, + KEY_ALLOC_UID_KEYRING | + KEY_ALLOC_IN_QUOTA, NULL, NULL); if (IS_ERR(session_keyring)) { ret = PTR_ERR(session_keyring);