New issue
Advanced search Search tips

Issue 783373 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Bring keys subsystem in chromeos-3.18 up to date

Project Member Reported by groeck@chromium.org, Nov 9 2017

Issue description

The KEYS subsystem in chromeos-3.18 is out of date and missing various security patches. Bring it up to date.

 
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 11 2017

Labels: merge-merged-chromeos-3.18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e74ee2f332af1c0595f34aba3155b5ec2b9443c4

commit e74ee2f332af1c0595f34aba3155b5ec2b9443c4
Author: Takashi Iwai <tiwai@suse.de>
Date: Sat Nov 11 00:27:05 2017

UPSTREAM: KEYS: Fix stale key registration at error path

commit b26bdde5bb27f3f900e25a95e33a0c476c8c2c48 upstream.

When loading encrypted-keys module, if the last check of
aes_get_sizes() in init_encrypted() fails, the driver just returns an
error without unregistering its key type.  This results in the stale
entry in the list.  In addition to memory leaks, this leads to a kernel
crash when registering a new key type later.

This patch fixes the problem by swapping the calls of aes_get_sizes()
and register_key_type(), and releasing resources properly at the error
paths.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I8f2959405df2d853ef2e7b6bc73c724115cd07d3
Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=908163
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit a481fd127721)
Reviewed-on: https://chromium-review.googlesource.com/761362

[modify] https://crrev.com/e74ee2f332af1c0595f34aba3155b5ec2b9443c4/security/keys/encrypted-keys/encrypted.c

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4bcb1ee6626105f12630663cb69dbb58614f2ce8

commit 4bcb1ee6626105f12630663cb69dbb58614f2ce8
Author: Colin Ian King <colin.king@canonical.com>
Date: Sat Nov 11 00:27:07 2017

UPSTREAM: KEYS: ensure we free the assoc array edit if edit is valid

[ Upstream commit HEAD ]

commit ca4da5dd1f99fe9c59f1709fb43e818b18ad20e0 upstream.

__key_link_end is not freeing the associated array edit structure
and this leads to a 512 byte memory leak each time an identical
existing key is added with add_key().

The reason the add_key() system call returns okay is that
key_create_or_update() calls __key_link_begin() before checking to see
whether it can update a key directly rather than adding/replacing - which
it turns out it can.  Thus __key_link() is not called through
__key_instantiate_and_link() and __key_link_end() must cancel the edit.

CVE-2015-1333

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

(cherry picked from commit c9cd9b18dac801040ada16562dc579d5ac366d75)
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

BUG= chromium:783373 
TEST=Build and run

Change-Id: I69f2474ec2a3dbf94ad67a5e49c509190679166b
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 66db51c9f7b2)
Reviewed-on: https://chromium-review.googlesource.com/761363

[modify] https://crrev.com/4bcb1ee6626105f12630663cb69dbb58614f2ce8/security/keys/keyring.c

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/80c1da4eb90838168cc497b6bd65e86db3870dc3

commit 80c1da4eb90838168cc497b6bd65e86db3870dc3
Author: David Howells <dhowells@redhat.com>
Date: Sat Nov 11 00:27:08 2017

UPSTREAM: KEYS: Fix race between read and revoke

[ Upstream commit b4a1b4f5047e4f54e194681125c74c0aa64d637d ]

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

BUG= chromium:783373 
TEST=Build and run

Change-Id: I55b4e69681ec7fe72e4a648fda8b21886a022a21
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>
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit e41946e47ec5)
Reviewed-on: https://chromium-review.googlesource.com/761364

[modify] https://crrev.com/80c1da4eb90838168cc497b6bd65e86db3870dc3/security/keys/keyctl.c

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/78b217b7fd53cf63638598f7ea4c2077feda20e7

commit 78b217b7fd53cf63638598f7ea4c2077feda20e7
Author: Mimi Zohar <zohar@linux.vnet.ibm.com>
Date: Sat Nov 11 00:27:09 2017

UPSTREAM: KEYS: fix "ca_keys=" partial key matching

[ Upstream commit f2b3dee484f9cee967a54ef05a66866282337519 ]

The call to asymmetric_key_hex_to_key_id() from ca_keys_setup()
silently fails with -ENOMEM.  Instead of dynamically allocating
memory from a __setup function, this patch defines a variable
and calls __asymmetric_key_hex_to_key_id(), a new helper function,
directly.

This bug was introduced by 'commit 46963b774d44 ("KEYS: Overhaul
key identification when searching for asymmetric keys")'.

Changelog:
- for clarification, rename hexlen to asciihexlen in
  asymmetric_key_hex_to_key_id()
- add size argument to __asymmetric_key_hex_to_key_id() - David Howells
- inline __asymmetric_key_hex_to_key_id() - David Howells
- remove duplicate strlen() calls

BUG= chromium:783373 
TEST=Build and run

Change-Id: I613b2b62a5fff41ec86c4c2fc7cf8eb9586482ae
Acked-by: David Howells <dhowells@redhat.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org # 3.18
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 07519e4ac1ca)
Reviewed-on: https://chromium-review.googlesource.com/761365

[modify] https://crrev.com/78b217b7fd53cf63638598f7ea4c2077feda20e7/crypto/asymmetric_keys/asymmetric_keys.h
[modify] https://crrev.com/78b217b7fd53cf63638598f7ea4c2077feda20e7/crypto/asymmetric_keys/asymmetric_type.c
[modify] https://crrev.com/78b217b7fd53cf63638598f7ea4c2077feda20e7/crypto/asymmetric_keys/x509_public_key.c

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/61cddf2003799f23e4d3df7b7336333ed65fbfe2

commit 61cddf2003799f23e4d3df7b7336333ed65fbfe2
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date: Sat Nov 11 00:27:12 2017

UPSTREAM: KEYS: potential uninitialized variable

[ Upstream commit 38327424b40bcebe2de92d07312c89360ac9229a ]

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:
	  kfree+0xde/0x1bc
	  assoc_array_cancel_edit+0x1f/0x36
	  __key_link_end+0x55/0x63
	  key_reject_and_link+0x124/0x155
	  keyctl_reject_key+0xb6/0xe0
	  keyctl_negate_key+0x10/0x12
	  SyS_keyctl+0x9f/0xe7
	  do_syscall_64+0x63/0x13a
	  entry_SYSCALL64_slow_path+0x25/0x25

BUG= chromium:783373 
TEST=Build and run

Change-Id: I7ad254b998596cc877568289d835b08ec15ff832
Fixes: f70e2e06196a ('KEYS: Do preallocation for __key_link()')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: stable@vger.kernel.org
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 4e7a91fe8332)
Reviewed-on: https://chromium-review.googlesource.com/761367

[modify] https://crrev.com/61cddf2003799f23e4d3df7b7336333ed65fbfe2/security/keys/key.c

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0f11d13341e3555a21fb5f863b9ea67f471fad6e

commit 0f11d13341e3555a21fb5f863b9ea67f471fad6e
Author: David Howells <dhowells@redhat.com>
Date: Sat Nov 11 00:27:13 2017

UPSTREAM: KEYS: Disallow keyrings beginning with '.' to be joined as session keyrings

commit ee8f844e3c5a73b999edf733df1c529d6503ec2f upstream.

This fixes CVE-2016-9604.

Keyrings whose name begin with a '.' are special internal keyrings and so
userspace isn't allowed to create keyrings by this name to prevent
shadowing.  However, the patch that added the guard didn't fix
KEYCTL_JOIN_SESSION_KEYRING.  Not only can that create dot-named keyrings,
it can also subscribe to them as a session keyring if they grant SEARCH
permission to the user.

This, for example, allows a root process to set .builtin_trusted_keys as
its session keyring, at which point it has full access because now the
possessor permissions are added.  This permits root to add extra public
keys, thereby bypassing module verification.

This also affects kexec and IMA.

This can be tested by (as root):

	keyctl session .builtin_trusted_keys
	keyctl add user a a @s
	keyctl list @s

which on my test box gives me:

	2 keys in keyring:
	180010936: ---lswrv     0     0 asymmetric: Build time autogenerated kernel key: ae3d4a31b82daa8e1a75b49dc2bba949fd992a05
	801382539: --alswrv     0     0 user: a

Fix this by rejecting names beginning with a '.' in the keyctl.

BUG= chromium:783373 
TEST=Build and run

Change-Id: Id9480385c5747867ffe78d489696bf22d3932f44
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
cc: linux-ima-devel@lists.sourceforge.net
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 44c037827f0a)
Reviewed-on: https://chromium-review.googlesource.com/761368

[modify] https://crrev.com/0f11d13341e3555a21fb5f863b9ea67f471fad6e/security/keys/keyctl.c

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ed739d786ca341887d40fbd376566a0cde83c796

commit ed739d786ca341887d40fbd376566a0cde83c796
Author: David Howells <dhowells@redhat.com>
Date: Sat Nov 11 00:27:14 2017

UPSTREAM: KEYS: Change the name of the dead type to ".dead" to prevent user access

commit c1644fe041ebaf6519f6809146a77c3ead9193af upstream.

This fixes CVE-2017-6951.

Userspace should not be able to do things with the "dead" key type as it
doesn't have some of the helper functions set upon it that the kernel
needs.  Attempting to use it may cause the kernel to crash.

Fix this by changing the name of the type to ".dead" so that it's rejected
up front on userspace syscalls by key_get_type_from_user().

Though this doesn't seem to affect recent kernels, it does affect older
ones, certainly those prior to:

	commit c06cfb08b88dfbe13be44a69ae2fdc3a7c902d81
	Author: David Howells <dhowells@redhat.com>
	Date:   Tue Sep 16 17:36:06 2014 +0100
	KEYS: Remove key_type::match in favour of overriding default by match_preparse

which went in before 3.18-rc1.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I50aaa7ba02776dd10a19a9f54c5b8ef47ed29740
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 44d6e10f7709)
Reviewed-on: https://chromium-review.googlesource.com/761369

[modify] https://crrev.com/ed739d786ca341887d40fbd376566a0cde83c796/security/keys/gc.c

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/30c4bd5f91fad592f609777c3d9f8f886543975d

commit 30c4bd5f91fad592f609777c3d9f8f886543975d
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:15 2017

UPSTREAM: KEYS: fix dereferencing NULL payload with nonzero length

commit 5649645d725c73df4302428ee4e02c869248b4c5 upstream.

sys_add_key() and the KEYCTL_UPDATE operation of sys_keyctl() allowed a
NULL payload with nonzero length to be passed to the key type's
->preparse(), ->instantiate(), and/or ->update() methods.  Various key
types including asymmetric, cifs.idmap, cifs.spnego, and pkcs7_test did
not handle this case, allowing an unprivileged user to trivially cause a
NULL pointer dereference (kernel oops) if one of these key types was
present.  Fix it by doing the copy_from_user() when 'plen' is nonzero
rather than when '_payload' is non-NULL, causing the syscall to fail
with EFAULT as expected when an invalid buffer is specified.

BUG= chromium:783373 
TEST=Build and run

Change-Id: Ib19f43d62507aaf95e465acad4bbc7067be9d8dc
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 8206e0a25785)
Reviewed-on: https://chromium-review.googlesource.com/761370

[modify] https://crrev.com/30c4bd5f91fad592f609777c3d9f8f886543975d/security/keys/keyctl.c

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4289a2c9462d509af7a203c4c09a848a4dc675f7

commit 4289a2c9462d509af7a203c4c09a848a4dc675f7
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:17 2017

UPSTREAM: KEYS: fix freeing uninitialized memory in key_update()

commit 63a0b0509e700717a59f049ec6e4e04e903c7fe2 upstream.

key_update() freed the key_preparsed_payload even if it was not
initialized first.  This would cause a crash if userspace called
keyctl_update() on a key with type like "asymmetric" that has a
->preparse() method but not an ->update() method.  Possibly it could
even be triggered for other key types by racing with keyctl_setperm() to
make the KEY_NEED_WRITE check fail (the permission was already checked,
so normally it wouldn't fail there).

Reproducer with key type "asymmetric", given a valid cert.der:

keyctl new_session
keyid=$(keyctl padd asymmetric desc @s < cert.der)
keyctl setperm $keyid 0x3f000000
keyctl update $keyid data

[  150.686666] BUG: unable to handle kernel NULL pointer dereference at 0000000000000001
[  150.687601] IP: asymmetric_key_free_kids+0x12/0x30
[  150.688139] PGD 38a3d067
[  150.688141] PUD 3b3de067
[  150.688447] PMD 0
[  150.688745]
[  150.689160] Oops: 0000 [#1] SMP
[  150.689455] Modules linked in:
[  150.689769] CPU: 1 PID: 2478 Comm: keyctl Not tainted 4.11.0-rc4-xfstests-00187-ga9f6b6b8cd2f #742
[  150.690916] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
[  150.692199] task: ffff88003b30c480 task.stack: ffffc90000350000
[  150.692952] RIP: 0010:asymmetric_key_free_kids+0x12/0x30
[  150.693556] RSP: 0018:ffffc90000353e58 EFLAGS: 00010202
[  150.694142] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000004
[  150.694845] RDX: ffffffff81ee3920 RSI: ffff88003d4b0700 RDI: 0000000000000001
[  150.697569] RBP: ffffc90000353e60 R08: ffff88003d5d2140 R09: 0000000000000000
[  150.702483] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
[  150.707393] R13: 0000000000000004 R14: ffff880038a4d2d8 R15: 000000000040411f
[  150.709720] FS:  00007fcbcee35700(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
[  150.711504] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  150.712733] CR2: 0000000000000001 CR3: 0000000039eab000 CR4: 00000000003406e0
[  150.714487] Call Trace:
[  150.714975]  asymmetric_key_free_preparse+0x2f/0x40
[  150.715907]  key_update+0xf7/0x140
[  150.716560]  ? key_default_cmp+0x20/0x20
[  150.717319]  keyctl_update_key+0xb0/0xe0
[  150.718066]  SyS_keyctl+0x109/0x130
[  150.718663]  entry_SYSCALL_64_fastpath+0x1f/0xc2
[  150.719440] RIP: 0033:0x7fcbce75ff19
[  150.719926] RSP: 002b:00007ffd5d167088 EFLAGS: 00000206 ORIG_RAX: 00000000000000fa
[  150.720918] RAX: ffffffffffffffda RBX: 0000000000404d80 RCX: 00007fcbce75ff19
[  150.721874] RDX: 00007ffd5d16785e RSI: 000000002866cd36 RDI: 0000000000000002
[  150.722827] RBP: 0000000000000006 R08: 000000002866cd36 R09: 00007ffd5d16785e
[  150.723781] R10: 0000000000000004 R11: 0000000000000206 R12: 0000000000404d80
[  150.724650] R13: 00007ffd5d16784d R14: 00007ffd5d167238 R15: 000000000040411f
[  150.725447] Code: 83 c4 08 31 c0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 85 ff 74 23 55 48 89 e5 53 48 89 fb <48> 8b 3f e8 06 21 c5 ff 48 8b 7b 08 e8 fd 20 c5 ff 48 89 df e8
[  150.727489] RIP: asymmetric_key_free_kids+0x12/0x30 RSP: ffffc90000353e58
[  150.728117] CR2: 0000000000000001
[  150.728430] ---[ end trace f7f8fe1da2d5ae8d ]---

BUG= chromium:783373 
TEST=Build and run

Change-Id: I3656c94cc3266df96b2106c867405992d147e260
Fixes: 4d8c0250b841 ("KEYS: Call ->free_preparse() even after ->preparse() returns an error")
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 45771cd795ec)
Reviewed-on: https://chromium-review.googlesource.com/761371

[modify] https://crrev.com/4289a2c9462d509af7a203c4c09a848a4dc675f7/security/keys/key.c

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/23b10b5829c2f2d61d7feba94169abd9e458b4a0

commit 23b10b5829c2f2d61d7feba94169abd9e458b4a0
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date: Sat Nov 11 00:27:18 2017

UPSTREAM: KEYS: Fix an error code in request_master_key()

commit 57cb17e764ba0aaa169d07796acce54ccfbc6cae upstream.

This function has two callers and neither are able to handle a NULL
return.  Really, -EINVAL is the correct thing return here anyway.  This
fixes some static checker warnings like:

	security/keys/encrypted-keys/encrypted.c:709 encrypted_key_decrypt()
	error: uninitialized symbol 'master_key'.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I1dcb9e1fa9854fbbba6659a97e4aa784a9db230c
Fixes: 7e70cb497850 ("keys: add new key-type encrypted")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 9c2586ce3255)
Reviewed-on: https://chromium-review.googlesource.com/761372

[modify] https://crrev.com/23b10b5829c2f2d61d7feba94169abd9e458b4a0/security/keys/encrypted-keys/encrypted.c

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5583421cbbf0df34474dcf91971d1fe84411c29e

commit 5583421cbbf0df34474dcf91971d1fe84411c29e
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:19 2017

UPSTREAM: KEYS: fix writing past end of user-supplied buffer in keyring_read()

commit e645016abc803dafc75e4b8f6e4118f088900ffb upstream.

Userspace can call keyctl_read() on a keyring to get the list of IDs of
keys in the keyring.  But if the user-supplied buffer is too small, the
kernel would write the full list anyway --- which will corrupt whatever
userspace memory happened to be past the end of the buffer.  Fix it by
only filling the space that is available.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I02f616f6fe9e376eea3231429b8d1d942cee11f7
Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 50f17a4b9141)
Reviewed-on: https://chromium-review.googlesource.com/761373

[modify] https://crrev.com/5583421cbbf0df34474dcf91971d1fe84411c29e/security/keys/keyring.c

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/88136c911611c390bf79c01ab24f44909f62181a

commit 88136c911611c390bf79c01ab24f44909f62181a
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:20 2017

UPSTREAM: KEYS: prevent creating a different user's keyrings

commit 237bbd29f7a049d310d907f4b2716a7feef9abf3 upstream.

It was possible for an unprivileged user to create the user and user
session keyrings for another user.  For example:

    sudo -u '#3000' sh -c 'keyctl add keyring _uid.4000 "" @u
                           keyctl add keyring _uid_ses.4000 "" @u
                           sleep 15' &
    sleep 1
    sudo -u '#4000' keyctl describe @u
    sudo -u '#4000' keyctl describe @us

This is problematic because these "fake" keyrings won't have the right
permissions.  In particular, the user who created them first will own
them and will have full access to them via the possessor permissions,
which can be used to compromise the security of a user's keys:

    -4: alswrv-----v------------  3000     0 keyring: _uid.4000
    -5: alswrv-----v------------  3000     0 keyring: _uid_ses.4000

Fix it by marking user and user session keyrings with a flag
KEY_FLAG_UID_KEYRING.  Then, when searching for a user or user session
keyring by name, skip all keyrings that don't have the flag set.

BUG= chromium:783373 
TEST=Build and run

Change-Id: Ic0f054779e3eb5a3735d96c4f1a4f05c655fee17
Fixes: 69664cf16af4 ("keys: don't generate user and user session keyrings unless they're accessed")
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit e3b663ba2ddd)
Reviewed-on: https://chromium-review.googlesource.com/761374

[modify] https://crrev.com/88136c911611c390bf79c01ab24f44909f62181a/security/keys/internal.h
[modify] https://crrev.com/88136c911611c390bf79c01ab24f44909f62181a/security/keys/process_keys.c
[modify] https://crrev.com/88136c911611c390bf79c01ab24f44909f62181a/security/keys/keyring.c
[modify] https://crrev.com/88136c911611c390bf79c01ab24f44909f62181a/include/linux/key.h
[modify] https://crrev.com/88136c911611c390bf79c01ab24f44909f62181a/security/keys/key.c

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/a7388930a8bee7357487ae068c7c93dfca632d4c

commit a7388930a8bee7357487ae068c7c93dfca632d4c
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:23 2017

UPSTREAM: KEYS: encrypted: fix dereference of NULL user_key_payload

commit 13923d0865ca96312197962522e88bc0aedccd74 upstream.

A key of type "encrypted" references a "master key" which is used to
encrypt and decrypt the encrypted key's payload.  However, when we
accessed the master key's payload, we failed to handle the case where
the master key has been revoked, which sets the payload pointer to NULL.
Note that request_key() *does* skip revoked keys, but there is still a
window where the key can be revoked before we acquire its semaphore.

Fix it by checking for a NULL payload, treating it like a key which was
already revoked at the time it was requested.

This was an issue for master keys of type "user" only.  Master keys can
also be of type "trusted", but those cannot be revoked.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I1c0522bdab26a46eea34c6af84c92d239591f76f
Fixes: 7e70cb497850 ("keys: add new key-type encrypted")
Reviewed-by: James Morris <james.l.morris@oracle.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@us.ibm.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 400f063f4dbc)
Reviewed-on: https://chromium-review.googlesource.com/761756

[modify] https://crrev.com/a7388930a8bee7357487ae068c7c93dfca632d4c/security/keys/encrypted-keys/encrypted.c

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e3f3180e75762a88e973ed203b1db07ec9e50414

commit e3f3180e75762a88e973ed203b1db07ec9e50414
Author: David Howells <dhowells@redhat.com>
Date: Sat Nov 11 00:27:24 2017

UPSTREAM: KEYS: don't let add_key() update an uninstantiated key

commit 60ff5b2f547af3828aebafd54daded44cfb0807a upstream.

Currently, when passed a key that already exists, add_key() will call the
key's ->update() method if such exists.  But this is heavily broken in the
case where the key is uninstantiated because it doesn't call
__key_instantiate_and_link().  Consequently, it doesn't do most of the
things that are supposed to happen when the key is instantiated, such as
setting the instantiation state, clearing KEY_FLAG_USER_CONSTRUCT and
awakening tasks waiting on it, and incrementing key->user->nikeys.

It also never takes key_construction_mutex, which means that
->instantiate() can run concurrently with ->update() on the same key.  In
the case of the "user" and "logon" key types this causes a memory leak, at
best.  Maybe even worse, the ->update() methods of the "encrypted" and
"trusted" key types actually just dereference a NULL pointer when passed an
uninstantiated key.

Change key_create_or_update() to wait interruptibly for the key to finish
construction before continuing.

This patch only affects *uninstantiated* keys.  For now we still allow a
negatively instantiated key to be updated (thereby positively
instantiating it), although that's broken too (the next patch fixes it)
and I'm not sure that anyone actually uses that functionality either.

Here is a simple reproducer for the bug using the "encrypted" key type
(requires CONFIG_ENCRYPTED_KEYS=y), though as noted above the bug
pertained to more than just the "encrypted" key type:

    #include <stdlib.h>
    #include <unistd.h>
    #include <keyutils.h>

    int main(void)
    {
        int ringid = keyctl_join_session_keyring(NULL);

        if (fork()) {
            for (;;) {
                const char payload[] = "update user:foo 32";

                usleep(rand() % 10000);
                add_key("encrypted", "desc", payload, sizeof(payload), ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("encrypted", "desc", "callout_info", ringid);
        }
    }

It causes:

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
    IP: encrypted_update+0xb0/0x170
    PGD 7a178067 P4D 7a178067 PUD 77269067 PMD 0
    PREEMPT SMP
    CPU: 0 PID: 340 Comm: reproduce Tainted: G      D         4.14.0-rc1-00025-g428490e38b2e #796
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
    task: ffff8a467a39a340 task.stack: ffffb15c40770000
    RIP: 0010:encrypted_update+0xb0/0x170
    RSP: 0018:ffffb15c40773de8 EFLAGS: 00010246
    RAX: 0000000000000000 RBX: ffff8a467a275b00 RCX: 0000000000000000
    RDX: 0000000000000005 RSI: ffff8a467a275b14 RDI: ffffffffb742f303
    RBP: ffffb15c40773e20 R08: 0000000000000000 R09: ffff8a467a275b17
    R10: 0000000000000020 R11: 0000000000000000 R12: 0000000000000000
    R13: 0000000000000000 R14: ffff8a4677057180 R15: ffff8a467a275b0f
    FS:  00007f5d7fb08700(0000) GS:ffff8a467f200000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000000018 CR3: 0000000077262005 CR4: 00000000001606f0
    Call Trace:
     key_create_or_update+0x2bc/0x460
     SyS_add_key+0x10c/0x1d0
     entry_SYSCALL_64_fastpath+0x1f/0xbe
    RIP: 0033:0x7f5d7f211259
    RSP: 002b:00007ffed03904c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
    RAX: ffffffffffffffda RBX: 000000003b2a7955 RCX: 00007f5d7f211259
    RDX: 00000000004009e4 RSI: 00000000004009ff RDI: 0000000000400a04
    RBP: 0000000068db8bad R08: 000000003b2a7955 R09: 0000000000000004
    R10: 000000000000001a R11: 0000000000000246 R12: 0000000000400868
    R13: 00007ffed03905d0 R14: 0000000000000000 R15: 0000000000000000
    Code: 77 28 e8 64 34 1f 00 45 31 c0 31 c9 48 8d 55 c8 48 89 df 48 8d 75 d0 e8 ff f9 ff ff 85 c0 41 89 c4 0f 88 84 00 00 00 4c 8b 7d c8 <49> 8b 75 18 4c 89 ff e8 24 f8 ff ff 85 c0 41 89 c4 78 6d 49 8b
    RIP: encrypted_update+0xb0/0x170 RSP: ffffb15c40773de8
    CR2: 0000000000000018

BUG= chromium:783373 
TEST=Build and run

Change-Id: I4031c52661427a5a221acf76bab8a8ca2ac684f9
Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 98c4e5cae520)
Reviewed-on: https://chromium-review.googlesource.com/761757

[modify] https://crrev.com/e3f3180e75762a88e973ed203b1db07ec9e50414/security/keys/key.c

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/dd4c0e566d4f855d4b9f42d3a3ccf298de096721

commit dd4c0e566d4f855d4b9f42d3a3ccf298de096721
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:26 2017

UPSTREAM: KEYS: return full count in keyring_read() if buffer is too small

commit 3239b6f29bdfb4b0a2ba59df995fc9e6f4df7f1f upstream.

Commit e645016abc80 ("KEYS: fix writing past end of user-supplied buffer
in keyring_read()") made keyring_read() stop corrupting userspace memory
when the user-supplied buffer is too small.  However it also made the
return value in that case be the short buffer size rather than the size
required, yet keyctl_read() is actually documented to return the size
required.  Therefore, switch it over to the documented behavior.

Note that for now we continue to have it fill the short buffer, since it
did that before (pre-v3.13) and dump_key_tree_aux() in keyutils arguably
relies on it.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I6e95ea014dd5ae6ed41792b44b762cd5b126aee4
Fixes: e645016abc80 ("KEYS: fix writing past end of user-supplied buffer in keyring_read()")
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit bcb91ec291c1)
Reviewed-on: https://chromium-review.googlesource.com/761758

[modify] https://crrev.com/dd4c0e566d4f855d4b9f42d3a3ccf298de096721/security/keys/keyring.c

Project Member

Comment 17 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/22c71d40f2e49fa45ac4c850b6b0bf1a974cde69

commit 22c71d40f2e49fa45ac4c850b6b0bf1a974cde69
Author: Eric Biggers <ebiggers@google.com>
Date: Sat Nov 11 00:27:27 2017

UPSTREAM: KEYS: fix out-of-bounds read during ASN.1 parsing

commit 2eb9eabf1e868fda15808954fb29b0f105ed65f1 upstream.

syzkaller with KASAN reported an out-of-bounds read in
asn1_ber_decoder().  It can be reproduced by the following command,
assuming CONFIG_X509_CERTIFICATE_PARSER=y and CONFIG_KASAN=y:

    keyctl add asymmetric desc $'\x30\x30' @s

The bug is that the length of an ASN.1 data value isn't validated in the
case where it is encoded using the short form, causing the decoder to
read past the end of the input buffer.  Fix it by validating the length.

The bug report was:

    BUG: KASAN: slab-out-of-bounds in asn1_ber_decoder+0x10cb/0x1730 lib/asn1_decoder.c:233
    Read of size 1 at addr ffff88003cccfa02 by task syz-executor0/6818

    CPU: 1 PID: 6818 Comm: syz-executor0 Not tainted 4.14.0-rc7-00008-g5f479447d983 #2
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
    Call Trace:
     __dump_stack lib/dump_stack.c:16 [inline]
     dump_stack+0xb3/0x10b lib/dump_stack.c:52
     print_address_description+0x79/0x2a0 mm/kasan/report.c:252
     kasan_report_error mm/kasan/report.c:351 [inline]
     kasan_report+0x236/0x340 mm/kasan/report.c:409
     __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:427
     asn1_ber_decoder+0x10cb/0x1730 lib/asn1_decoder.c:233
     x509_cert_parse+0x1db/0x650 crypto/asymmetric_keys/x509_cert_parser.c:89
     x509_key_preparse+0x64/0x7a0 crypto/asymmetric_keys/x509_public_key.c:174
     asymmetric_key_preparse+0xcb/0x1a0 crypto/asymmetric_keys/asymmetric_type.c:388
     key_create_or_update+0x347/0xb20 security/keys/key.c:855
     SYSC_add_key security/keys/keyctl.c:122 [inline]
     SyS_add_key+0x1cd/0x340 security/keys/keyctl.c:62
     entry_SYSCALL_64_fastpath+0x1f/0xbe
    RIP: 0033:0x447c89
    RSP: 002b:00007fca7a5d3bd8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
    RAX: ffffffffffffffda RBX: 00007fca7a5d46cc RCX: 0000000000447c89
    RDX: 0000000020006f4a RSI: 0000000020006000 RDI: 0000000020001ff5
    RBP: 0000000000000046 R08: fffffffffffffffd R09: 0000000000000000
    R10: 0000000000000002 R11: 0000000000000246 R12: 0000000000000000
    R13: 0000000000000000 R14: 00007fca7a5d49c0 R15: 00007fca7a5d4700

BUG= chromium:783373 
TEST=Build and run

Change-Id: Iafda82eaf09f4d45025c6a8e9b6873e3031ab84f
Fixes: 42d5ec27f873 ("X.509: Add an ASN.1 decoder")
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit ef2518bac630)
Reviewed-on: https://chromium-review.googlesource.com/761759
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/22c71d40f2e49fa45ac4c850b6b0bf1a974cde69/lib/asn1_decoder.c

Status: Fixed (was: Started)
Status: Assigned (was: Fixed)
More patches to go (from linux-3.18.y):

86fa24082f80 security/keys: add CONFIG_KEYS_COMPAT to Kconfig
1f166fb65e89 KEYS: fix NULL pointer dereference during ASN.1 parsing [ver #2]
c64cc4117fec KEYS: trusted: fix writing past end of buffer in trusted_read()
cdd1a3fd76d5 KEYS: trusted: sanitize all key material

Reopening.

Status: Started (was: Assigned)
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/aa2c4e1093076f0faee0cbaa7f4e444f7eae87a9

commit aa2c4e1093076f0faee0cbaa7f4e444f7eae87a9
Author: Eric Biggers <ebiggers@google.com>
Date: Tue Nov 28 06:50:36 2017

UPSTREAM: KEYS: trusted: sanitize all key material

commit ee618b4619b72527aaed765f0f0b74072b281159 upstream.

As the previous patch did for encrypted-keys, zero sensitive any
potentially sensitive data related to the "trusted" key type before it
is freed.  Notably, we were not zeroing the tpm_buf structures in which
the actual key is stored for TPM seal and unseal, nor were we zeroing
the trusted_key_payload in certain error paths.

BUG= chromium:783373 
TEST=Build and run

Change-Id: Ifc9c4fbb22de5a4a6b38a30bcfbafc5a9ea8ccda
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@us.ibm.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit cdd1a3fd76d5)
Reviewed-on: https://chromium-review.googlesource.com/791495
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/aa2c4e1093076f0faee0cbaa7f4e444f7eae87a9/security/keys/trusted.c

Project Member

Comment 22 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e72937a9128e6c038d5a4fab0e3aa555fb043e98

commit e72937a9128e6c038d5a4fab0e3aa555fb043e98
Author: Eric Biggers <ebiggers@google.com>
Date: Tue Nov 28 06:50:37 2017

UPSTREAM: KEYS: trusted: fix writing past end of buffer in trusted_read()

commit a3c812f7cfd80cf51e8f5b7034f7418f6beb56c1 upstream.

When calling keyctl_read() on a key of type "trusted", if the
user-supplied buffer was too small, the kernel ignored the buffer length
and just wrote past the end of the buffer, potentially corrupting
userspace memory.  Fix it by instead returning the size required, as per
the documentation for keyctl_read().

We also don't even fill the buffer at all in this case, as this is
slightly easier to implement than doing a short read, and either
behavior appears to be permitted.  It also makes it match the behavior
of the "encrypted" key type.

BUG= chromium:783373 
TEST=Build and run

Change-Id: I1981db3e281843042cfecf7c33d37f5329e186c0
Fixes: d00a1c72f7f4 ("keys: add new trusted key-type")
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Cc: <stable@vger.kernel.org> # v2.6.38+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Reviewed-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit c64cc4117fec)
Reviewed-on: https://chromium-review.googlesource.com/791496
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/e72937a9128e6c038d5a4fab0e3aa555fb043e98/security/keys/trusted.c

Project Member

Comment 23 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/10251b1bdbeb338581adce11e4fc69255f66def7

commit 10251b1bdbeb338581adce11e4fc69255f66def7
Author: Eric Biggers <ebiggers@google.com>
Date: Tue Nov 28 06:50:38 2017

UPSTREAM: KEYS: fix NULL pointer dereference during ASN.1 parsing [ver #2]

commit 624f5ab8720b3371367327a822c267699c1823b8 upstream.

syzkaller reported a NULL pointer dereference in asn1_ber_decoder().  It
can be reproduced by the following command, assuming
CONFIG_PKCS7_TEST_KEY=y:

        keyctl add pkcs7_test desc '' @s

The bug is that if the data buffer is empty, an integer underflow occurs
in the following check:

        if (unlikely(dp >= datalen - 1))
                goto data_overrun_error;

This results in the NULL data pointer being dereferenced.

Fix it by checking for 'datalen - dp < 2' instead.

Also fix the similar check for 'dp >= datalen - n' later in the same
function.  That one possibly could result in a buffer overread.

The NULL pointer dereference was reproducible using the "pkcs7_test" key
type but not the "asymmetric" key type because the "asymmetric" key type
checks for a 0-length payload before calling into the ASN.1 decoder but
the "pkcs7_test" key type does not.

The bug report was:

    BUG: unable to handle kernel NULL pointer dereference at           (null)
    IP: asn1_ber_decoder+0x17f/0xe60 lib/asn1_decoder.c:233
    PGD 7b708067 P4D 7b708067 PUD 7b6ee067 PMD 0
    Oops: 0000 [#1] SMP
    Modules linked in:
    CPU: 0 PID: 522 Comm: syz-executor1 Not tainted 4.14.0-rc8 #7
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.3-20171021_125229-anatol 04/01/2014
    task: ffff9b6b3798c040 task.stack: ffff9b6b37970000
    RIP: 0010:asn1_ber_decoder+0x17f/0xe60 lib/asn1_decoder.c:233
    RSP: 0018:ffff9b6b37973c78 EFLAGS: 00010216
    RAX: 0000000000000000 RBX: 0000000000000000 RCX: 000000000000021c
    RDX: ffffffff814a04ed RSI: ffffb1524066e000 RDI: ffffffff910759e0
    RBP: ffff9b6b37973d60 R08: 0000000000000001 R09: ffff9b6b3caa4180
    R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000002
    R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
    FS:  00007f10ed1f2700(0000) GS:ffff9b6b3ea00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000000000 CR3: 000000007b6f3000 CR4: 00000000000006f0
    Call Trace:
     pkcs7_parse_message+0xee/0x240 crypto/asymmetric_keys/pkcs7_parser.c:139
     verify_pkcs7_signature+0x33/0x180 certs/system_keyring.c:216
     pkcs7_preparse+0x41/0x70 crypto/asymmetric_keys/pkcs7_key_type.c:63
     key_create_or_update+0x180/0x530 security/keys/key.c:855
     SYSC_add_key security/keys/keyctl.c:122 [inline]
     SyS_add_key+0xbf/0x250 security/keys/keyctl.c:62
     entry_SYSCALL_64_fastpath+0x1f/0xbe
    RIP: 0033:0x4585c9
    RSP: 002b:00007f10ed1f1bd8 EFLAGS: 00000216 ORIG_RAX: 00000000000000f8
    RAX: ffffffffffffffda RBX: 00007f10ed1f2700 RCX: 00000000004585c9
    RDX: 0000000020000000 RSI: 0000000020008ffb RDI: 0000000020008000
    RBP: 0000000000000000 R08: ffffffffffffffff R09: 0000000000000000
    R10: 0000000000000000 R11: 0000000000000216 R12: 00007fff1b2260ae
    R13: 00007fff1b2260af R14: 00007f10ed1f2700 R15: 0000000000000000
    Code: dd ca ff 48 8b 45 88 48 83 e8 01 4c 39 f0 0f 86 a8 07 00 00 e8 53 dd ca ff 49 8d 46 01 48 89 85 58 ff ff ff 48 8b 85 60 ff ff ff <42> 0f b6 0c 30 89 c8 88 8d 75 ff ff ff 83 e0 1f 89 8d 28 ff ff
    RIP: asn1_ber_decoder+0x17f/0xe60 lib/asn1_decoder.c:233 RSP: ffff9b6b37973c78
    CR2: 0000000000000000

BUG= chromium:783373 
TEST=Build and run

Change-Id: I87acfb6746173dcd02e350d7a633e49b73818d09
Fixes: 42d5ec27f873 ("X.509: Add an ASN.1 decoder")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
(cherry picked from commit 1f166fb65e89)
Reviewed-on: https://chromium-review.googlesource.com/791497
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/10251b1bdbeb338581adce11e4fc69255f66def7/lib/asn1_decoder.c

Status: Fixed (was: Started)

Sign in to add a comment