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

From d3600bcf9d64d88dc1d189a754dcfab960ce751f Mon Sep 17 00:00:00 2001

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 <[email protected]> know about it.

From d3600bcf9d64d88dc1d189a754dcfab960ce751f Mon Sep 17 00:00:00 2001
From: Mimi Zohar <[email protected]>
Date: Tue, 10 Nov 2015 08:34:46 -0500
Subject: KEYS: prevent keys from being removed from specified keyrings

From: Mimi Zohar <[email protected]>

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 <[email protected]>

Cc David Howells <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 include/linux/key.h    |    1 
 security/keys/key.c    |    6 ++++-
 security/keys/keyctl.c |   56 ++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 52 insertions(+), 11 deletions(-)

include/linux/key.h +1 -0

@@ -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

security/keys/key.c +5 -1

@@ -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)

security/keys/keyctl.c +46 -10

@@ -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 [email protected] are

queue-3.10/keys-refcount-bug-fix.patch queue-3.10/keys-prevent-keys-from-being-removed-from-specified-keyrings.patch

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.

Quoted 7 lines · Mimi Zohar

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.

Ah, ok, remove this for all stable kernel trees, right? For some reason I thought this was resolving a different key security "issue" that was recently reported...