diff mbox

KEYS: fix dereferencing NULL payload with nonzero length

Message ID 20170401213428.17097-1-ebiggers3@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Biggers April 1, 2017, 9:34 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

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.

Cc: stable@vger.kernel.org # 2.6.10+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/keyctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

David Howells April 3, 2017, 3:46 p.m. UTC | #1 
Eric Biggers <ebiggers3@gmail.com> wrote:

> -	if (_payload) {
> +	if (plen) {

"if (_payload && plen)" would be better.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers April 3, 2017, 5:59 p.m. UTC | #2 
On Mon, Apr 03, 2017 at 04:46:42PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > -	if (_payload) {
> > +	if (plen) {
> 
> "if (_payload && plen)" would be better.
> 
> David

No, that doesn't solve the problem.  The problem is that userspace can pass in a
NULL payload with nonzero length, causing the kernel to dereference a NULL
pointer for some key types.  For example:

	add_key("asymmetric", "desc", NULL, 1000, KEY_SPEC_SESSION_KEYRING)

Results in (assuming CONFIG_X509_CERTIFICATE_PARSER=y):

	[    6.046093] BUG: unable to handle kernel NULL pointer dereference at           (null)
	[    6.047781] IP: asn1_ber_decoder+0xe0/0x588
	[    6.048723] PGD 79570067 
	[    6.048726] PUD 7a7d4067 
	[    6.048999] PMD 0 
	[    6.048999] 
	[    6.048999] Oops: 0000 [#1] SMP
	[    6.048999] CPU: 0 PID: 2509 Comm: add_key Not tainted 4.11.0-rc5-ext4-00007-g4ad72555b842-dirty #136
	[    6.048999] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
	[    6.048999] task: ffff88007a664640 task.stack: ffffc90000a20000
	[    6.048999] RIP: 0010:asn1_ber_decoder+0xe0/0x588
	[    6.048999] RSP: 0018:ffffc90000a23ce0 EFLAGS: 00010293
	[    6.048999] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
	[    6.048999] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000002
	[    6.048999] RBP: ffffc90000a23d80 R08: 0000000000000060 R09: ffffffff81a7c510
	[    6.048999] R10: ffffc90000a23c00 R11: 0000000088092f04 R12: 0000000000000000
	[    6.048999] R13: 00000000000003e8 R14: 0000000000000000 R15: 0000000000000000
	[    6.048999] FS:  0000000001af5880(0000) GS:ffff88007f200000(0000) knlGS:0000000000000000
	[    6.048999] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	[    6.048999] CR2: 0000000000000000 CR3: 0000000079566000 CR4: 00000000000006f0
	[    6.048999] Call Trace:
	[    6.048999]  ? rcu_read_lock_sched_held+0x40/0x47
	[    6.048999]  ? kmem_cache_alloc_trace+0x1eb/0x29b
	[    6.048999]  ? x509_cert_parse+0x98/0x19f
	[    6.048999]  ? x509_cert_parse+0x98/0x19f
	[    6.048999]  x509_cert_parse+0xbc/0x19f
	[    6.048999]  x509_key_preparse+0x26/0x190
	[    6.048999]  asymmetric_key_preparse+0x3a/0x6a
	[    6.048999]  key_create_or_update+0x140/0x39d
	[    6.048999]  SyS_add_key+0x157/0x1ac
	[    6.048999]  entry_SYSCALL_64_fastpath+0x1f/0xc2
	[    6.048999] RIP: 0033:0x435389
	[    6.048999] RSP: 002b:00007ffd6792ae88 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
	[    6.048999] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000435389
	[    6.048999] RDX: 0000000000000000 RSI: 0000000000493ee4 RDI: 0000000000493ee9
	[    6.048999] RBP: 00007ffd6792ae70 R08: 00000000fffffffd R09: 0000000000000000
	[    6.048999] R10: 00000000000003e8 R11: 0000000000000246 R12: 00007ffd6792af88
	[    6.048999] R13: 00007ffd6792af98 R14: 0000000000000002 R15: 0000000000000000
	[    6.048999] Code: 75 0e 41 88 d2 41 80 e2 01 74 0f 4c 39 eb 75 0a 41 83 e6 fb 48 8b 45 80 eb 97 49 8d 4d ff 48 39 cb 0f 83 1c 03 00 00 49 8d 0c 1f <40> 8a 39 4c 8d 43 01 40 88 7d 8d 83 e7 1f 40 80 ff 1f 0f 84 00 
	[    6.048999] RIP: asn1_ber_decoder+0xe0/0x588 RSP: ffffc90000a23ce0
	[    6.048999] CR2: 0000000000000000
	[    6.073968] ---[ end trace d27c036692bbc3da ]---

- Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells April 3, 2017, 7:20 p.m. UTC | #3 
Eric Biggers <ebiggers3@gmail.com> wrote:

> > > -	if (_payload) {
> > > +	if (plen) {
> > 
> > "if (_payload && plen)" would be better.
> > 
> > David
> 
> No, that doesn't solve the problem.  The problem is that userspace can pass
> in a NULL payload with nonzero length, causing the kernel to dereference a
> NULL pointer for some key types.  For example:

Okay, in that case, I think there should be an else-statement that clears plen
if !_payload.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers April 3, 2017, 9:30 p.m. UTC | #4 
On Mon, Apr 03, 2017 at 08:20:44PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > > > -	if (_payload) {
> > > > +	if (plen) {
> > > 
> > > "if (_payload && plen)" would be better.
> > > 
> > > David
> > 
> > No, that doesn't solve the problem.  The problem is that userspace can pass
> > in a NULL payload with nonzero length, causing the kernel to dereference a
> > NULL pointer for some key types.  For example:
> 
> Okay, in that case, I think there should be an else-statement that clears plen
> if !_payload.
> 
> David

I think it's preferable to return EFAULT in the case in question.  Most syscalls
work like that, i.e. if you say you have 100 bytes (or any number > 0) at
address NULL you'll get EFAULT.

Also note that anyone doing this before would have been either crashing the
kernel or getting EINVAL.  So starting to return EFAULT would be very unlikely
to break anything.

- Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers April 17, 2017, 5:29 p.m. UTC | #5 
On Mon, Apr 17, 2017 at 02:26:41PM +0800, kernel test robot wrote:
> 
> FYI, we noticed the following commit:
> 
> commit: bdf7c0f8bf282ba44827ce3c7fd7936c8e90a18a ("KEYS: fix dereferencing NULL payload with nonzero length")
> url: https://github.com/0day-ci/linux/commits/Eric-Biggers/KEYS-fix-dereferencing-NULL-payload-with-nonzero-length/20170403-102013
> base: https://git.kernel.org/cgit/linux/kernel/git/jmorris/linux-security.git next
> 
...
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> 
> 
> user  :notice: [   45.447047] <<<test_start>>>
> 
> user  :notice: [   45.447365] tag=add_key02 stime=1492169102
> 
> user  :notice: [   45.447567] cmdline="add_key02"
> 
> user  :notice: [   45.447685] contacts=""
> 
> user  :notice: [   45.447826] analysis=exit
> 
> user  :notice: [   45.448011] <<<test_output>>>
> 
> user  :notice: [   45.448568] tst_test.c:760: INFO: Timeout per run is 0h 05m 00s
> 
> user  :notice: [   45.449439] add_key02.c:65: FAIL: add_key() failed unexpectedly, expected EINVAL: EFAULT

In my opinion this is a valid behavior, and the test is just weird; it's passing
in *both* an unaddressable payload and an invalid description, so it's not clear
which case it's meant to be testing.  (Generally, if a syscall will fail for
more than one reason, it's not guaranteed which error code you'll get.)

In any case, once we have a fix merged, it would be nice for there to be an ltp
test added for the "NULL payload with nonzero length" case with one of the key
types that crashed the kernel.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cyril Hrubis April 20, 2017, 12:57 p.m. UTC | #6 
Hi!
> > commit: bdf7c0f8bf282ba44827ce3c7fd7936c8e90a18a ("KEYS: fix dereferencing NULL payload with nonzero length")
> > url: https://github.com/0day-ci/linux/commits/Eric-Biggers/KEYS-fix-dereferencing-NULL-payload-with-nonzero-length/20170403-102013
> > base: https://git.kernel.org/cgit/linux/kernel/git/jmorris/linux-security.git next
> > 
> ...
> > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> > 
> > 
> > user  :notice: [   45.447047] <<<test_start>>>
> > 
> > user  :notice: [   45.447365] tag=add_key02 stime=1492169102
> > 
> > user  :notice: [   45.447567] cmdline="add_key02"
> > 
> > user  :notice: [   45.447685] contacts=""
> > 
> > user  :notice: [   45.447826] analysis=exit
> > 
> > user  :notice: [   45.448011] <<<test_output>>>
> > 
> > user  :notice: [   45.448568] tst_test.c:760: INFO: Timeout per run is 0h 05m 00s
> > 
> > user  :notice: [   45.449439] add_key02.c:65: FAIL: add_key() failed unexpectedly, expected EINVAL: EFAULT
> 
> In my opinion this is a valid behavior, and the test is just weird; it's passing
> in *both* an unaddressable payload and an invalid description, so it's not clear
> which case it's meant to be testing.  (Generally, if a syscall will fail for
> more than one reason, it's not guaranteed which error code you'll get.)

That is quite common problem with LTP testcases. Do you care to send a
patch or should I fix that?

> In any case, once we have a fix merged, it would be nice for there to be an ltp
> test added for the "NULL payload with nonzero length" case with one of the key
> types that crashed the kernel.

Here as well, feel free to send a patch or at least point us to a
reproducer that could be turned into a testcase.
Eric Biggers April 21, 2017, 4:43 a.m. UTC | #7 
Hi Cyril,

On Thu, Apr 20, 2017 at 02:57:50PM +0200, Cyril Hrubis wrote:
> > 
> > In my opinion this is a valid behavior, and the test is just weird; it's passing
> > in *both* an unaddressable payload and an invalid description, so it's not clear
> > which case it's meant to be testing.  (Generally, if a syscall will fail for
> > more than one reason, it's not guaranteed which error code you'll get.)
> 
> That is quite common problem with LTP testcases. Do you care to send a
> patch or should I fix that?
> 

I'll plan to send a patch.  Also, it looks like the testing that LTP does of
add_key() is very sparse, so I'll try to extend it a bit.

> > In any case, once we have a fix merged, it would be nice for there to be an ltp
> > test added for the "NULL payload with nonzero length" case with one of the key
> > types that crashed the kernel.
> 
> Here as well, feel free to send a patch or at least point us to a
> reproducer that could be turned into a testcase.
> 

I'll plan to send a patch for that as well.

Thanks,

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers May 31, 2017, 7:11 p.m. UTC | #8 
On Mon, Apr 03, 2017 at 02:30:41PM -0700, Eric Biggers wrote:
> On Mon, Apr 03, 2017 at 08:20:44PM +0100, David Howells wrote:
> > Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > > > > -	if (_payload) {
> > > > > +	if (plen) {
> > > > 
> > > > "if (_payload && plen)" would be better.
> > > > 
> > > > David
> > > 
> > > No, that doesn't solve the problem.  The problem is that userspace can pass
> > > in a NULL payload with nonzero length, causing the kernel to dereference a
> > > NULL pointer for some key types.  For example:
> > 
> > Okay, in that case, I think there should be an else-statement that clears plen
> > if !_payload.
> > 
> > David
> 
> I think it's preferable to return EFAULT in the case in question.  Most syscalls
> work like that, i.e. if you say you have 100 bytes (or any number > 0) at
> address NULL you'll get EFAULT.
> 
> Also note that anyone doing this before would have been either crashing the
> kernel or getting EINVAL.  So starting to return EFAULT would be very unlikely
> to break anything.
> 
> - Eric

David, can you please apply this?  Or if you haven't applied it because you
prefer the other solution then please explain your reasoning.

It's really not acceptable for unprivileged users to be able to trivially oops
the kernel.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells June 2, 2017, 1:43 p.m. UTC | #9 
Eric Biggers <ebiggers3@gmail.com> wrote:

> I'll plan to send a patch.  Also, it looks like the testing that LTP does of
> add_key() is very sparse, so I'll try to extend it a bit.

There's more testing in the testsuite that's with the keyutils package.

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

Patch

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 52c34532c785..57447cd29154 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -99,7 +99,7 @@  SYSCALL_DEFINE5(add_key, const char __user *, _type,
 	/* pull the payload in if one was supplied */
 	payload = NULL;
 
-	if (_payload) {
+	if (plen) {
 		ret = -ENOMEM;
 		payload = kmalloc(plen, GFP_KERNEL | __GFP_NOWARN);
 		if (!payload) {
@@ -324,7 +324,7 @@  long keyctl_update_key(key_serial_t id,
 
 	/* pull the payload in if one was supplied */
 	payload = NULL;
-	if (_payload) {
+	if (plen) {
 		ret = -ENOMEM;
 		payload = kmalloc(plen, GFP_KERNEL);
 		if (!payload)