Re: [PATCH] security/keys: fix slab-out-of-bounds in key_task_permission

[Chen Ridong]

On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
> > On 2024/9/14 19:33, Jarkko Sakkinen wrote:
> > > On Fri Sep 13, 2024 at 10:09 AM EEST, Chen Ridong wrote:
> > > > We meet the same issue with the LINK, which reads memory out of bounds:
> > >
> > > Nit: don't use "we" anywhere".
> > >
> > > Tbh, I really don't understand the sentence above. I don't what
> > > "the same issue with the LINK" really is.
> >
> > Hello, Jarkko.
> > I apologize for any confusion caused.
> >
> > I've encountered a bug reported by syzkaller. I also found the same bug
> > reported at this LINK:
> > https://syzkaller.appspot.com/bug?id=68a5e206c2a8e08d317eb83f05610c0484ad10b9.
> >
> > > > BUG: KASAN: slab-out-of-bounds in __kuid_val include/linux/uidgid.h:36
> > > > BUG: KASAN: slab-out-of-bounds in uid_eq include/linux/uidgid.h:63 [inline]
> > > > BUG: KASAN: slab-out-of-bounds in key_task_permission+0x394/0x410
> > > > security/keys/permission.c:54
> > > > Read of size 4 at addr ffff88813c3ab618 by task stress-ng/4362
> > > >
> > > > CPU: 2 PID: 4362 Comm: stress-ng Not tainted 5.10.0-14930-gafbffd6c3ede #15
> > > > Call Trace:
> > > >    __dump_stack lib/dump_stack.c:82 [inline]
> > > >    dump_stack+0x107/0x167 lib/dump_stack.c:123
> > > >    print_address_description.constprop.0+0x19/0x170 mm/kasan/report.c:400
> > > >    __kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560
> > > >    kasan_report+0x3a/0x50 mm/kasan/report.c:585
> > > >    __kuid_val include/linux/uidgid.h:36 [inline]
> > > >    uid_eq include/linux/uidgid.h:63 [inline]
> > > >    key_task_permission+0x394/0x410 security/keys/permission.c:54
> > > >    search_nested_keyrings+0x90e/0xe90 security/keys/keyring.c:793
> > > >    keyring_search_rcu+0x1b6/0x310 security/keys/keyring.c:922
> > > >    search_cred_keyrings_rcu+0x111/0x2e0 security/keys/process_keys.c:459
> > > >    search_process_keyrings_rcu+0x1d/0x310 security/keys/process_keys.c:544
> > > >    lookup_user_key+0x782/0x12e0 security/keys/process_keys.c:762
> > > >    keyctl_invalidate_key+0x20/0x190 security/keys/keyctl.c:434
> > > >    __do_sys_keyctl security/keys/keyctl.c:1978 [inline]
> > > >    __se_sys_keyctl+0x1de/0x5b0 security/keys/keyctl.c:1880
> > > >    do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
> > > >    entry_SYSCALL_64_after_hwframe+0x67/0xd1
> > > >
> > > > However, we can't reproduce this issue.
> > >
> > > "The issue cannot be easily reproduced but by analyzing the code
> > > it can be broken into following steps:"
> >
> > Thank you for your correction.
> > Does this patch address the issue correctly? Is this patch acceptable?
>
> I only comment new patch versions so not giving any promises but I can
> say that it is I think definitely in the correct direction :-)
>
> BR, Jarkko

Hello, Jarkko. I have reproduced this issue. It can be reproduced by 
following these steps:

1. Add the helper patch.

@@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct 
keyring_index_key *index_key)
        else if (index_key->type == &key_type_keyring && (hash & 
fan_mask) != 0)
                hash = (hash + (hash << level_shift)) & ~fan_mask;
        index_key->hash = hash;
+       if ((index_key->hash & 0xff) == 0xe6) {
+                       pr_err("hash_key_type_and_desc: type %s %s 
0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
+       }
 }

2. Pick up the inputs whose hash is xxe6 using the following cmd. If a 
key's hash is xxe6, it will be printed.

for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done

You have complile test_key whith following code.

#include <sys/types.h>
#include <keyutils.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int
main(int argc, char *argv[])
{
   key_serial_t key;

   if (argc != 4) {
	   fprintf(stderr, "Usage: %s type description payload\n",
			   argv[0]);
	   exit(EXIT_FAILURE);
   }

   key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
			   KEY_SPEC_SESSION_KEYRING);
   if (key == -1) {
	   perror("add_key");
	   exit(EXIT_FAILURE);
   }

   printf("Key ID is %jx\n", (uintmax_t) key);

   exit(EXIT_SUCCESS);
}


3. Have more than 32 inputs now. their hashes are xxe6.
eg.
hash_key_type_and_desc: type user user438 0xe3033fe6
hash_key_type_and_desc: type user user526 0xeb7eade6
...
hash_key_type_and_desc: type user user9955 0x44bc99e6

4. Reboot and add the keys obtained from step 3.
When adding keys to the ROOT that their hashes are all xxe6, and up to 
16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so 
the keys are dissimilar. The ROOT will then split NODE A without using a 
shortcut. When NODE A is filled with keys that have hashes of xxe6, the 
keys are similar. NODE A will split with a shortcut.

As my analysis, if a slot of the root is a shortcut(slot 6), it may be 
mistakenly be transferred to a key*, leading to an read out-of-bounds read.

                      NODE A
              +------>+---+
      ROOT    |       | 0 | xxe6
      +---+   |       +---+
 xxxx | 0 | shortcut  :   : xxe6
      +---+   |       +---+
 xxe6 :   :   |       |   | xxe6
      +---+   |       +---+
      | 6 |---+       :   : xxe6
      +---+           +---+
 xxe6 :   :           | f | xxe6
      +---+           +---+
 xxe6 | f |
      +---+

5. cat /proc/keys. and the issue is reproduced.