| From b4a1b4f5047e4f54e194681125c74c0aa64d637d Mon Sep 17 00:00:00 2001 |
| From: David Howells <dhowells@redhat.com> |
| Date: Fri, 18 Dec 2015 01:34:26 +0000 |
| Subject: KEYS: Fix race between read and revoke |
| |
| commit b4a1b4f5047e4f54e194681125c74c0aa64d637d upstream. |
| |
| 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 <sys/types.h> |
| #include <keyutils.h> |
| #include <pthread.h> |
| 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: [<ffffffff81279b08>] user_read+0x56/0xa3 |
| ... |
| Call Trace: |
| [<ffffffff81276aa9>] keyctl_read_key+0xb6/0xd7 |
| [<ffffffff81277815>] SyS_keyctl+0x83/0xe0 |
| [<ffffffff815dbb97>] entry_SYSCALL_64_fastpath+0x12/0x6f |
| |
| Reported-by: Dmitry Vyukov <dvyukov@google.com> |
| Signed-off-by: David Howells <dhowells@redhat.com> |
| Tested-by: Dmitry Vyukov <dvyukov@google.com> |
| Signed-off-by: James Morris <james.l.morris@oracle.com> |
| Signed-off-by: Zefan Li <lizefan@huawei.com> |
| --- |
| security/keys/keyctl.c | 18 +++++++++--------- |
| 1 file changed, 9 insertions(+), 9 deletions(-) |
| |
| --- a/security/keys/keyctl.c |
| +++ b/security/keys/keyctl.c |
| @@ -702,16 +702,16 @@ long keyctl_read_key(key_serial_t keyid, |
| |
| /* 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: |