ACK: [SRU][Trusty][PATCH 1/1] KEYS: prevent KEYCTL_READ on negative key

[Colin Ian King]

On 08/12/17 16:16, Kleber Sacilotto de Souza wrote:
> From: Eric Biggers <ebiggers at google.com>
> 
> Because keyctl_read_key() looks up the key with no permissions
> requested, it may find a negatively instantiated key.  If the key is
> also possessed, we went ahead and called ->read() on the key.  But the
> key payload will actually contain the ->reject_error rather than the
> normal payload.  Thus, the kernel oopses trying to read the
> user_key_payload from memory address (int)-ENOKEY = 0x00000000ffffff82.
> 
> Fortunately the payload data is stored inline, so it shouldn't be
> possible to abuse this as an arbitrary memory read primitive...
> 
> Reproducer:
>     keyctl new_session
>     keyctl request2 user desc '' @s
>     keyctl read $(keyctl show | awk '/user: desc/ {print $1}')
> 
> It causes a crash like the following:
>      BUG: unable to handle kernel paging request at 00000000ffffff92
>      IP: user_read+0x33/0xa0
>      PGD 36a54067 P4D 36a54067 PUD 0
>      Oops: 0000 [#1] SMP
>      CPU: 0 PID: 211 Comm: keyctl Not tainted 4.14.0-rc1 #337
>      Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
>      task: ffff90aa3b74c3c0 task.stack: ffff9878c0478000
>      RIP: 0010:user_read+0x33/0xa0
>      RSP: 0018:ffff9878c047bee8 EFLAGS: 00010246
>      RAX: 0000000000000001 RBX: ffff90aa3d7da340 RCX: 0000000000000017
>      RDX: 0000000000000000 RSI: 00000000ffffff82 RDI: ffff90aa3d7da340
>      RBP: ffff9878c047bf00 R08: 00000024f95da94f R09: 0000000000000000
>      R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
>      R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>      FS:  00007f58ece69740(0000) GS:ffff90aa3e200000(0000) knlGS:0000000000000000
>      CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>      CR2: 00000000ffffff92 CR3: 0000000036adc001 CR4: 00000000003606f0
>      Call Trace:
>       keyctl_read_key+0xac/0xe0
>       SyS_keyctl+0x99/0x120
>       entry_SYSCALL_64_fastpath+0x1f/0xbe
>      RIP: 0033:0x7f58ec787bb9
>      RSP: 002b:00007ffc8d401678 EFLAGS: 00000206 ORIG_RAX: 00000000000000fa
>      RAX: ffffffffffffffda RBX: 00007ffc8d402800 RCX: 00007f58ec787bb9
>      RDX: 0000000000000000 RSI: 00000000174a63ac RDI: 000000000000000b
>      RBP: 0000000000000004 R08: 00007ffc8d402809 R09: 0000000000000020
>      R10: 0000000000000000 R11: 0000000000000206 R12: 00007ffc8d402800
>      R13: 00007ffc8d4016e0 R14: 0000000000000000 R15: 0000000000000000
>      Code: e5 41 55 49 89 f5 41 54 49 89 d4 53 48 89 fb e8 a4 b4 ad ff 85 c0 74 09 80 3d b9 4c 96 00 00 74 43 48 8b b3 20 01 00 00 4d 85 ed <0f> b7 5e 10 74 29 4d 85 e4 74 24 4c 39 e3 4c 89 e2 4c 89 ef 48
>      RIP: user_read+0x33/0xa0 RSP: ffff9878c047bee8
>      CR2: 00000000ffffff92
> 
> Fixes: 61ea0c0ba904 ("KEYS: Skip key state checks when checking for possession")
> Cc: <stable at vger.kernel.org>	[v3.13+]
> Signed-off-by: Eric Biggers <ebiggers at google.com>
> Signed-off-by: David Howells <dhowells at redhat.com>
> 
> CVE-2017-12192
> (backported from commit 37863c43b2c6464f252862bf2e9768264e961678)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> ---
>  security/keys/keyctl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 4e3fecc72f43..d2bfb63038f6 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -731,6 +731,11 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  
>  	key = key_ref_to_ptr(key_ref);
>  
> +	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
> +		ret = -ENOKEY;
> +		goto error2;
> +	}
> +
>  	/* see if we can read it directly */
>  	ret = key_permission(key_ref, KEY_READ);
>  	if (ret == 0)
> 
Backport looks good.

Acked-by: Colin Ian King <colin.king at canonical.com>