From d3600bcf9d64d88dc1d189a754dcfab960ce751f Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Tue, 10 Nov 2015 08:34:46 -0500 Subject: KEYS: prevent keys from being removed from specified keyrings 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 Cc: David Howells --- security/keys/keyctl.c | 56 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 10 deletions(-) (limited to 'security/keys/keyctl.c') diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index fb111eafcb89..e83ec6b9eb9d 100644 --- 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_NEED_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); @@ -420,8 +431,13 @@ long keyctl_invalidate_key(key_serial_t id) } invalidate: - 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; + } error_put: key_ref_put(key_ref); error: @@ -433,12 +449,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_NEED_WRITE); @@ -460,7 +477,11 @@ long keyctl_keyring_clear(key_serial_t ringid) } 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: @@ -511,11 +532,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_NEED_WRITE); @@ -530,7 +554,13 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid) 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: @@ -1289,6 +1319,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) @@ -1320,10 +1352,14 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout) 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; } -- cgit v1.2.3 From b4a1b4f5047e4f54e194681125c74c0aa64d637d Mon Sep 17 00:00:00 2001 From: David Howells Date: Fri, 18 Dec 2015 01:34:26 +0000 Subject: KEYS: Fix race between read and revoke This fixes CVE-2015-7550. There's a race between keyctl_read() and keyctl_revoke(). If the revoke happens between keyctl_read() checking the validity of a key and the key's semaphore being taken, then the key type read method will see a revoked key. This causes a problem for the user-defined key type because it assumes in its read method that there will always be a payload in a non-revoked key and doesn't check for a NULL pointer. Fix this by making keyctl_read() check the validity of a key after taking semaphore instead of before. I think the bug was introduced with the original keyrings code. This was discovered by a multithreaded test program generated by syzkaller (http://github.com/google/syzkaller). Here's a cleaned up version: #include #include #include void *thr0(void *arg) { key_serial_t key = (unsigned long)arg; keyctl_revoke(key); return 0; } void *thr1(void *arg) { key_serial_t key = (unsigned long)arg; char buffer[16]; keyctl_read(key, buffer, 16); return 0; } int main() { key_serial_t key = add_key("user", "%", "foo", 3, KEY_SPEC_USER_KEYRING); pthread_t th[5]; pthread_create(&th[0], 0, thr0, (void *)(unsigned long)key); pthread_create(&th[1], 0, thr1, (void *)(unsigned long)key); pthread_create(&th[2], 0, thr0, (void *)(unsigned long)key); pthread_create(&th[3], 0, thr1, (void *)(unsigned long)key); pthread_join(th[0], 0); pthread_join(th[1], 0); pthread_join(th[2], 0); pthread_join(th[3], 0); return 0; } Build as: cc -o keyctl-race keyctl-race.c -lkeyutils -lpthread Run as: while keyctl-race; do :; done as it may need several iterations to crash the kernel. The crash can be summarised as: BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 IP: [] user_read+0x56/0xa3 ... Call Trace: [] keyctl_read_key+0xb6/0xd7 [] SyS_keyctl+0x83/0xe0 [] entry_SYSCALL_64_fastpath+0x12/0x6f Reported-by: Dmitry Vyukov Signed-off-by: David Howells Tested-by: Dmitry Vyukov Cc: stable@vger.kernel.org Signed-off-by: James Morris --- security/keys/keyctl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'security/keys/keyctl.c') diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index fb111eafcb89..1c3872aeed14 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -751,16 +751,16 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) /* the key is probably readable - now try to read it */ can_read_key: - ret = key_validate(key); - if (ret == 0) { - ret = -EOPNOTSUPP; - if (key->type->read) { - /* read the data with the semaphore held (since we - * might sleep) */ - down_read(&key->sem); + ret = -EOPNOTSUPP; + if (key->type->read) { + /* Read the data with the semaphore held (since we might sleep) + * to protect against the key being updated or revoked. + */ + down_read(&key->sem); + ret = key_validate(key); + if (ret == 0) ret = key->type->read(key, buffer, buflen); - up_read(&key->sem); - } + up_read(&key->sem); } error2: -- cgit v1.2.3 From 1d6d167c2efcfe9539d9cffb1a1be9c92e39c2c0 Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Thu, 7 Jan 2016 07:46:36 -0500 Subject: KEYS: refcount bug fix This patch fixes the key_ref leak, removes the unnecessary KEY_FLAG_KEEP test before setting the flag, and cleans up the if/then brackets style introduced in commit: d3600bc KEYS: prevent keys from being removed from specified keyrings Reported-by: David Howells Signed-off-by: Mimi Zohar Acked-by: David Howells --- security/keys/key.c | 3 +-- security/keys/keyctl.c | 17 +++++++---------- 2 files changed, 8 insertions(+), 12 deletions(-) (limited to 'security/keys/keyctl.c') diff --git a/security/keys/key.c b/security/keys/key.c index 09ef276c4bdc..07a87311055c 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -430,8 +430,7 @@ static int __key_instantiate_and_link(struct key *key, /* and link it into the destination keyring */ if (keyring) { - if (test_bit(KEY_FLAG_KEEP, &keyring->flags)) - set_bit(KEY_FLAG_KEEP, &key->flags); + set_bit(KEY_FLAG_KEEP, &key->flags); __key_link(key, _edit); } diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index e83ec6b9eb9d..8f9f323f372b 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -381,12 +381,11 @@ long keyctl_revoke_key(key_serial_t id) } key = key_ref_to_ptr(key_ref); + ret = 0; if (test_bit(KEY_FLAG_KEEP, &key->flags)) - return -EPERM; - else { + ret = -EPERM; + else key_revoke(key); - ret = 0; - } key_ref_put(key_ref); error: @@ -432,12 +431,11 @@ long keyctl_invalidate_key(key_serial_t id) invalidate: key = key_ref_to_ptr(key_ref); + ret = 0; if (test_bit(KEY_FLAG_KEEP, &key->flags)) ret = -EPERM; - else { + else key_invalidate(key); - ret = 0; - } error_put: key_ref_put(key_ref); error: @@ -1352,12 +1350,11 @@ long keyctl_set_timeout(key_serial_t id, unsigned timeout) okay: key = key_ref_to_ptr(key_ref); + ret = 0; if (test_bit(KEY_FLAG_KEEP, &key->flags)) ret = -EPERM; - else { + else key_set_timeout(key, timeout); - ret = 0; - } key_put(key); error: -- cgit v1.2.3