Re: Patch "KEYS: prevent keys from being removed from specified keyrings" has been added to the 3.10-stable tree

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

Hi Greg,

The concept of not being able to remove a key from a keyring was
introduced to prevent keys from being removed from the blacklist
keyring.  The blacklist keyring was just upstreamed in the current open
window.   I don't see a need to backport either this patch or the "KEYS:
refcount bug fix" patch.

Mimi

On Wed, 2016-01-20 at 08:58 -0800, gregkh@linuxfoundation.org wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     KEYS: prevent keys from being removed from specified keyrings
> 
> to the 3.10-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      keys-prevent-keys-from-being-removed-from-specified-keyrings.patch
> and it can be found in the queue-3.10 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
> 
> 
> From d3600bcf9d64d88dc1d189a754dcfab960ce751f Mon Sep 17 00:00:00 2001
> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Date: Tue, 10 Nov 2015 08:34:46 -0500
> Subject: KEYS: prevent keys from being removed from specified keyrings
> 
> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 
> commit d3600bcf9d64d88dc1d189a754dcfab960ce751f upstream.
> 
> Userspace should not be allowed to remove keys from certain keyrings
> (eg. blacklist), though the keys themselves can expire.
> 
> This patch defines a new key flag named KEY_FLAG_KEEP to prevent
> userspace from being able to unlink, revoke, invalidate or timed
> out a key on a keyring.  When this flag is set on the keyring, all
> keys subsequently added are flagged.
> 
> In addition, when this flag is set, the keyring itself can not be
> cleared.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
>  include/linux/key.h    |    1 
>  security/keys/key.c    |    6 ++++-
>  security/keys/keyctl.c |   56 ++++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 52 insertions(+), 11 deletions(-)
> 
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -162,6 +162,7 @@ struct key {
>  #define KEY_FLAG_NEGATIVE	5	/* set if key is negative */
>  #define KEY_FLAG_ROOT_CAN_CLEAR	6	/* set if key can be cleared by root without permission */
>  #define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
> +#define KEY_FLAG_KEEP		12	/* set if key should not be removed */
> 
>  	/* the description string
>  	 * - this is used to match a key against search criteria
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -434,8 +434,12 @@ static int __key_instantiate_and_link(st
>  				awaken = 1;
> 
>  			/* and link it into the destination keyring */
> -			if (keyring)
> +			if (keyring) {
> +				if (test_bit(KEY_FLAG_KEEP, &keyring->flags))
> +					set_bit(KEY_FLAG_KEEP, &key->flags);
> +
>  				__key_link(keyring, key, _prealloc);
> +			}
> 
>  			/* disable the authorisation key */
>  			if (authkey)
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -358,11 +358,14 @@ error:
>   * and any links to the key will be automatically garbage collected after a
>   * certain amount of time (/proc/sys/kernel/keys/gc_delay).
>   *
> + * Keys with KEY_FLAG_KEEP set should not be revoked.
> + *
>   * If successful, 0 is returned.
>   */
>  long keyctl_revoke_key(key_serial_t id)
>  {
>  	key_ref_t key_ref;
> +	struct key *key;
>  	long ret;
> 
>  	key_ref = lookup_user_key(id, 0, KEY_WRITE);
> @@ -377,8 +380,13 @@ long keyctl_revoke_key(key_serial_t id)
>  		}
>  	}
> 
> -	key_revoke(key_ref_to_ptr(key_ref));
> -	ret = 0;
> +	key = key_ref_to_ptr(key_ref);
> +	if (test_bit(KEY_FLAG_KEEP, &key->flags))
> +		return -EPERM;
> +	else {
> +		key_revoke(key);
> +		ret = 0;
> +	}
> 
>  	key_ref_put(key_ref);
>  error:
> @@ -392,11 +400,14 @@ error:
>   * The key and any links to the key will be automatically garbage collected
>   * immediately.
>   *
> + * Keys with KEY_FLAG_KEEP set should not be invalidated.
> + *
>   * If successful, 0 is returned.
>   */
>  long keyctl_invalidate_key(key_serial_t id)
>  {
>  	key_ref_t key_ref;
> +	struct key *key;
>  	long ret;
> 
>  	kenter("%d", id);
> @@ -407,8 +418,13 @@ long keyctl_invalidate_key(key_serial_t
>  		goto error;
>  	}
> 
> -	key_invalidate(key_ref_to_ptr(key_ref));
> -	ret = 0;
> +	key = key_ref_to_ptr(key_ref);
> +	if (test_bit(KEY_FLAG_KEEP, &key->flags))
> +		ret = -EPERM;
> +	else {
> +		key_invalidate(key);
> +		ret = 0;
> +	}
> 
>  	key_ref_put(key_ref);
>  error:
> @@ -420,12 +436,13 @@ error:
>   * Clear the specified keyring, creating an empty process keyring if one of the
>   * special keyring IDs is used.
>   *
> - * The keyring must grant the caller Write permission for this to work.  If
> - * successful, 0 will be returned.
> + * The keyring must grant the caller Write permission and not have
> + * KEY_FLAG_KEEP set for this to work.  If successful, 0 will be returned.
>   */
>  long keyctl_keyring_clear(key_serial_t ringid)
>  {
>  	key_ref_t keyring_ref;
> +	struct key *keyring;
>  	long ret;
> 
>  	keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_WRITE);
> @@ -447,7 +464,11 @@ long keyctl_keyring_clear(key_serial_t r
>  	}
> 
>  clear:
> -	ret = keyring_clear(key_ref_to_ptr(keyring_ref));
> +	keyring = key_ref_to_ptr(keyring_ref);
> +	if (test_bit(KEY_FLAG_KEEP, &keyring->flags))
> +		ret = -EPERM;
> +	else
> +		ret = keyring_clear(keyring);
>  error_put:
>  	key_ref_put(keyring_ref);
>  error:
> @@ -498,11 +519,14 @@ error:
>   * itself need not grant the caller anything.  If the last link to a key is
>   * removed then that key will be scheduled for destruction.
>   *
> + * Keys or keyrings with KEY_FLAG_KEEP set should not be unlinked.
> + *
>   * If successful, 0 will be returned.
>   */
>  long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
>  {
>  	key_ref_t keyring_ref, key_ref;
> +	struct key *keyring, *key;
>  	long ret;
> 
>  	keyring_ref = lookup_user_key(ringid, 0, KEY_WRITE);
> @@ -517,7 +541,13 @@ long keyctl_keyring_unlink(key_serial_t
>  		goto error2;
>  	}
> 
> -	ret = key_unlink(key_ref_to_ptr(keyring_ref), key_ref_to_ptr(key_ref));
> +	keyring = key_ref_to_ptr(keyring_ref);
> +	key = key_ref_to_ptr(key_ref);
> +	if (test_bit(KEY_FLAG_KEEP, &keyring->flags) &&
> +	    test_bit(KEY_FLAG_KEEP, &key->flags))
> +		ret = -EPERM;
> +	else
> +		ret = key_unlink(keyring, key);
> 
>  	key_ref_put(key_ref);
>  error2:
> @@ -1306,6 +1336,8 @@ error:
>   * the current time.  The key and any links to the key will be automatically
>   * garbage collected after the timeout expires.
>   *
> + * Keys with KEY_FLAG_KEEP set should not be timed out.
> + *
>   * If successful, 0 is returned.
>   */
>  long keyctl_set_timeout(key_serial_t id, unsigned timeout)
> @@ -1337,10 +1369,14 @@ long keyctl_set_timeout(key_serial_t id,
> 
>  okay:
>  	key = key_ref_to_ptr(key_ref);
> -	key_set_timeout(key, timeout);
> +	if (test_bit(KEY_FLAG_KEEP, &key->flags))
> +		ret = -EPERM;
> +	else {
> +		key_set_timeout(key, timeout);
> +		ret = 0;
> +	}
>  	key_put(key);
> 
> -	ret = 0;
>  error:
>  	return ret;
>  }
> 
> 
> Patches currently in stable-queue which might be from zohar@linux.vnet.ibm.com are
> 
> queue-3.10/keys-refcount-bug-fix.patch
> queue-3.10/keys-prevent-keys-from-being-removed-from-specified-keyrings.patch
>