linux/security/keys/request_key.c, branch v5.0

KEYS: always initialize keyring_index_key::desc_len

2019-02-22T18:11:34Z

syzbot hit the 'BUG_ON(index_key->desc_len == 0);' in __key_link_begin() called from construct_alloc_key() during sys_request_key(), because the length of the key description was never calculated. The problem is that we rely on ->desc_len being initialized by search_process_keyrings(), specifically by search_nested_keyrings(). But, if the process isn't subscribed to any keyrings that never happens. Fix it by always initializing keyring_index_key::desc_len as soon as the description is set, like we already do in some places. The following program reproduces the BUG_ON() when it's run as root and no session keyring has been installed. If it doesn't work, try removing pam_keyinit.so from /etc/pam.d/login and rebooting. #include #include #include int main(void) { int id = add_key("keyring", "syz", NULL, 0, KEY_SPEC_USER_KEYRING); keyctl_setperm(id, KEY_OTH_WRITE); setreuid(5000, 5000); request_key("user", "desc", "", id); } Reported-by: syzbot+ec24e95ea483de0a24da@syzkaller.appspotmail.com Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring") Signed-off-by: Eric Biggers Signed-off-by: David Howells Cc: stable@vger.kernel.org Signed-off-by: James Morris

keys: Fix dependency loop between construction record and auth key

2019-02-15T22:12:09Z

In the request_key() upcall mechanism there's a dependency loop by which if a key type driver overrides the ->request_key hook and the userspace side manages to lose the authorisation key, the auth key and the internal construction record (struct key_construction) can keep each other pinned. Fix this by the following changes: (1) Killing off the construction record and using the auth key instead. (2) Including the operation name in the auth key payload and making the payload available outside of security/keys/. (3) The ->request_key hook is given the authkey instead of the cons record and operation name. Changes (2) and (3) allow the auth key to naturally be cleaned up if the keyring it is in is destroyed or cleared or the auth key is unlinked. Fixes: 7ee02a316600 ("keys: Fix dependency loop between construction record and auth key") Signed-off-by: David Howells Signed-off-by: James Morris

security: audit and remove any unnecessary uses of module.h

2018-12-12T22:58:51Z

Historically a lot of these existed because we did not have a distinction between what was modular code and what was providing support to modules via EXPORT_SYMBOL and friends. That changed when we forked out support for the latter into the export.h file. This means we should be able to reduce the usage of module.h in code that is obj-y Makefile or bool Kconfig. The advantage in removing such instances is that module.h itself sources about 15 other headers; adding significantly to what we feed cpp, and it can obscure what headers we are effectively using. Since module.h might have been the implicit source for init.h (for __init) and for export.h (for EXPORT_SYMBOL) we consider each instance for the presence of either and replace as needed. Cc: James Morris Cc: "Serge E. Hallyn" Cc: John Johansen Cc: Mimi Zohar Cc: Dmitry Kasatkin Cc: David Howells Cc: linux-security-module@vger.kernel.org Cc: linux-integrity@vger.kernel.org Cc: keyrings@vger.kernel.org Signed-off-by: Paul Gortmaker Signed-off-by: James Morris

KEYS: add missing permission check for request_key() destination

2017-12-08T15:13:27Z

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

KEYS: remove unnecessary get/put of explicit dest_keyring

2017-12-08T15:13:27Z

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

KEYS: Fix race between updating and finding a negative key

2017-10-18T08:12:40Z

Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection error into one field such that: (1) The instantiation state can be modified/read atomically. (2) The error can be accessed atomically with the state. (3) The error isn't stored unioned with the payload pointers. This deals with the problem that the state is spread over three different objects (two bits and a separate variable) and reading or updating them atomically isn't practical, given that not only can uninstantiated keys change into instantiated or rejected keys, but rejected keys can also turn into instantiated keys - and someone accessing the key might not be using any locking. The main side effect of this problem is that what was held in the payload may change, depending on the state. For instance, you might observe the key to be in the rejected state. You then read the cached error, but if the key semaphore wasn't locked, the key might've become instantiated between the two reads - and you might now have something in hand that isn't actually an error code. The state is now KEY_IS_UNINSTANTIATED, KEY_IS_POSITIVE or a negative error code if the key is negatively instantiated. The key_is_instantiated() function is replaced with key_is_positive() to avoid confusion as negative keys are also 'instantiated'. Additionally, barriering is included: (1) Order payload-set before state-set during instantiation. (2) Order state-read before payload-read when using the key. Further separate barriering is necessary if RCU is being used to access the payload content after reading the payload pointers. Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data") Cc: stable@vger.kernel.org # v4.4+ Reported-by: Eric Biggers Signed-off-by: David Howells Reviewed-by: Eric Biggers

doc: ReSTify keys-request-key.txt

2017-05-18T16:33:51Z

Adjusts for ReST markup and moves under keys security devel index. Cc: David Howells Signed-off-by: Kees Cook Signed-off-by: Jonathan Corbet

Make static usermode helper binaries constant

2017-01-19T11:59:45Z

There are a number of usermode helper binaries that are "hard coded" in the kernel today, so mark them as "const" to make it harder for someone to change where the variables point to. Cc: Benjamin Herrenschmidt Cc: Thomas Sailer Cc: "Rafael J. Wysocki" Cc: Johan Hovold Cc: Alex Elder Cc: "J. Bruce Fields" Cc: Jeff Layton Cc: David Howells Signed-off-by: Greg Kroah-Hartman

KEYS: Strip trailing spaces

2016-06-14T09:29:44Z

Strip some trailing spaces. Signed-off-by: David Howells

KEYS: Add a facility to restrict new links into a keyring

2016-04-11T21:37:37Z

Add a facility whereby proposed new links to be added to a keyring can be vetted, permitting them to be rejected if necessary. This can be used to block public keys from which the signature cannot be verified or for which the signature verification fails. It could also be used to provide blacklisting. This affects operations like add_key(), KEYCTL_LINK and KEYCTL_INSTANTIATE. To this end: (1) A function pointer is added to the key struct that, if set, points to the vetting function. This is called as: int (*restrict_link)(struct key *keyring, const struct key_type *key_type, unsigned long key_flags, const union key_payload *key_payload), where 'keyring' will be the keyring being added to, key_type and key_payload will describe the key being added and key_flags[*] can be AND'ed with KEY_FLAG_TRUSTED. [*] This parameter will be removed in a later patch when KEY_FLAG_TRUSTED is removed. The function should return 0 to allow the link to take place or an error (typically -ENOKEY, -ENOPKG or -EKEYREJECTED) to reject the link. The pointer should not be set directly, but rather should be set through keyring_alloc(). Note that if called during add_key(), preparse is called before this method, but a key isn't actually allocated until after this function is called. (2) KEY_ALLOC_BYPASS_RESTRICTION is added. This can be passed to key_create_or_update() or key_instantiate_and_link() to bypass the restriction check. (3) KEY_FLAG_TRUSTED_ONLY is removed. The entire contents of a keyring with this restriction emplaced can be considered 'trustworthy' by virtue of being in the keyring when that keyring is consulted. (4) key_alloc() and keyring_alloc() take an extra argument that will be used to set restrict_link in the new key. This ensures that the pointer is set before the key is published, thus preventing a window of unrestrictedness. Normally this argument will be NULL. (5) As a temporary affair, keyring_restrict_trusted_only() is added. It should be passed to keyring_alloc() as the extra argument instead of setting KEY_FLAG_TRUSTED_ONLY on a keyring. This will be replaced in a later patch with functions that look in the appropriate places for authoritative keys. Signed-off-by: David Howells Reviewed-by: Mimi Zohar