From: Eric Biggers <[email protected]>
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: [email protected] # 2.6.10+
Signed-off-by: Eric Biggers <[email protected]>
---
security/keys/keyctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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)
--
2.12.1
Eric Biggers <[email protected]> wrote:
> - if (_payload) {
> + if (plen) {
"if (_payload && plen)" would be better.
David
On Mon, Apr 03, 2017 at 04:46:42PM +0100, David Howells wrote:
> Eric Biggers <[email protected]> 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
Eric Biggers <[email protected]> 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
On Mon, Apr 03, 2017 at 08:20:44PM +0100, David Howells wrote:
> Eric Biggers <[email protected]> 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
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
in testcase: ltp
with following parameters:
test: syscalls_part1
test-description: The LTP testsuite contains a collection of tools for testing the Linux kernel and related features.
test-url: http://linux-test-project.github.io/
on test machine: 88 threads Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz with 48G memory
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
To reproduce:
git clone https://github.com/01org/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml
Thanks,
Xiaolong
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
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.
--
Cyril Hrubis
[email protected]
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
Eric Biggers <[email protected]> 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