KEYS: potential uninitialized variable

Commit Message

David Howells June 16, 2016, 2:48 p.m. UTC

From: Dan Carpenter <[e-mail]>

If __key_link_begin() failed then "edit" would be uninitialized.  I've
added a check to fix that.

This allows a random user to crash the kernel, though it's quite difficult
to achieve.  There are three ways it can be done as the user would have to
cause an error to occur in __key_link():

 (1) Cause the kernel to run out of memory.  In practice, this is difficult
     to achieve without ENOMEM cropping up elsewhere and aborting the
     attempt.

 (2) Revoke the destination keyring between the keyring ID being looked up
     and it being tested for revocation.  In practice, this is difficult to
     time correctly because the KEYCTL_REJECT function can only be used
     from the request-key upcall process.  Further, users can only make use
     of what's in /sbin/request-key.conf, though this does including a
     rejection debugging test - which means that the destination keyring
     has to be the caller's session keyring in practice.

 (3) Have just enough key quota available to create a key, a new session
     keyring for the upcall and a link in the session keyring, but not then
     sufficient quota to create a link in the nominated destination keyring
     so that it fails with EDQUOT.

The bug can be triggered using option (3) above using something like the
following:

	echo 80 >/proc/sys/kernel/keys/root_maxbytes
	keyctl request2 user debug:fred negate @t

The above sets the quota to something much lower (80) to make the bug
easier to trigger, but this is dependent on the system.  Note also that the
name of the keyring created contains a random number that may be between 1
and 10 characters in size, so may throw the test off by changing the amount
of quota used.

Assuming the failure occurs, something like the following will be seen:

	kfree_debugcheck: out of range ptr 6b6b6b6b6b6b6b68h
	------------[ cut here ]------------
	kernel BUG at ../mm/slab.c:2821!
	...
	RIP: 0010:[<ffffffff811600f9>] kfree_debugcheck+0x20/0x25
	RSP: 0018:ffff8804014a7de8  EFLAGS: 00010092
	RAX: 0000000000000034 RBX: 6b6b6b6b6b6b6b68 RCX: 0000000000000000
	RDX: 0000000000040001 RSI: 00000000000000f6 RDI: 0000000000000300
	RBP: ffff8804014a7df0 R08: 0000000000000001 R09: 0000000000000000
	R10: ffff8804014a7e68 R11: 0000000000000054 R12: 0000000000000202
	R13: ffffffff81318a66 R14: 0000000000000000 R15: 0000000000000001
	...
	Call Trace:
	 [<ffffffff8116429f>] kfree+0xde/0x1bc
	 [<ffffffff81318a66>] assoc_array_cancel_edit+0x1f/0x36
	 [<ffffffff812922a2>] __key_link_end+0x55/0x63
	 [<ffffffff81290519>] key_reject_and_link+0x124/0x155
	 [<ffffffff81293728>] keyctl_reject_key+0xb6/0xe0
	 [<ffffffff81293762>] keyctl_negate_key+0x10/0x12
	 [<ffffffff81293dff>] SyS_keyctl+0x9f/0xe7
	 [<ffffffff81001ce1>] do_syscall_64+0x63/0x13a
	 [<ffffffff81601d1a>] entry_SYSCALL64_slow_path+0x25/0x25

Fixes: f70e2e06196a ('KEYS: Do preallocation for __key_link()')
Signed-off-by: Dan Carpenter <[e-mail]>
Signed-off-by: David Howells <[e-mail]>
cc: [e-mail]
---

 security/keys/key.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Howells June 16, 2016, 2:50 p.m. UTC | #1

David Howells <[e-mail]> wrote:

> From: Dan Carpenter <[e-mail]>
> 
> If __key_link_begin() failed then "edit" would be uninitialized.  I've
> added a check to fix that.
> 
> This allows a random user to crash the kernel, though it's quite difficult
> to achieve.  There are three ways it can be done as the user would have to
> cause an error to occur in __key_link():
>
> ...

I forgot to mention, this fixes CVE-2016-4470.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/security/keys/key.c b/security/keys/key.c
index bd5a272f28a6..346fbf201c22 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -597,7 +597,7 @@  int key_reject_and_link(struct key *key,
 
 	mutex_unlock(&key_construction_mutex);
 
-	if (keyring)
+	if (keyring && link_ret == 0)
 		__key_link_end(keyring, &key->index_key, edit);
 
 	/* wake up anyone waiting for a key to be constructed */