Hi,
My POC uses fscreate() to modify the current SELinux context of the running
process, it then creates a new node via mknod(), (), which is then going to
assign the current SLEinux context over to that object.
In the call path I am seeing security_sid_to_context_core().
I see a code path where it in fact seem to correctly take the strlen() of the
incoming context, based on the sid, and use kmemdup(), but that only occurs
when SELinux is not initialized.
if (!ss_initialized) {
if (sid <= SECINITSID_NUM) {
char *scontextp;
*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
if (!scontext)
goto out;
scontextp = kmemdup(initial_sid_to_string[sid],
*scontext_len, GFP_ATOMIC);
if (!scontextp) {
rc = -ENOMEM;
goto out;
}
*scontext = scontextp;
goto out;
}
printk(KERN_ERR "SELinux: %s: called before initial "
"load_policy on unknown SID %d\n", __func__, sid);
rc = -EINVAL;
goto out;
}
Once SELinux is initialized that block will be skipped and further down you’ll
see the call to context_struct_to_string() which copies the context using
kstrdup() and uses the incoming length rather than the strlen() length. In
addition to this I’m not sure how safe using strlen() is in general as there
seems to be an assumption that context strings can contain NULL characters
based on this comment.
/* We strip a nul only if it is at the end, otherwise
the
* context contains a nul and we should audit that */
if (str[size - 1] == '\0')
audit_size = size - 1;
else
audit_size = size;
Call StacK:
[<ffffffff80430b25>] kasan_report+0x32c/0x485 mm/kasan/report.c:309
[<ffffffff8042fafb>] check_memory_region_inline mm/kasan/kasan.c:301 [inline]
[<ffffffff8042fafb>] check_memory_region+0x2b/0x130 mm/kasan/kasan.c:315
[<ffffffff8042fc38>] __asan_loadN+0xf/0x11 mm/kasan/kasan.c:745
[<ffffffff805775be>] ext4_xattr_set_entry+0x6a1/0x6d3 fs/ext4/xattr.c:747
[<ffffffff805790ab>] ext4_xattr_ibody_set.isra.9+0x4f/0x118 fs/ext4/xattr.c:1118
[<ffffffff8057a1cb>] ext4_xattr_set_handle+0x39a/0x7b4 fs/ext4/xattr.c:1224
[<ffffffff80582683>] ext4_initxattrs+0x6b/0x96 fs/ext4/xattr_security.c:42
[<ffffffff805ecd43>] security_inode_init_security+0x170/0x1c9
security/security.c:403
[<ffffffff8058275d>] ext4_init_security+0x35/0x3d fs/ext4/xattr_security.c:56
[<ffffffff8050295a>] __ext4_new_inode+0x1f24/0x2266 fs/ext4/ialloc.c:1059
[<ffffffff8051f771>] ext4_mknod+0x11a/0x263 fs/ext4/namei.c:2510
[<ffffffff8044d8e6>] vfs_mknod2+0x1ee/0x29f fs/namei.c:3722
[<ffffffff804554fb>] SYSC_mknodat fs/namei.c:3780 [inline]
[<ffffffff804554fb>] SyS_mknodat+0x19a/0x22a fs/namei.c:3752
[<ffffffff802028f9>] do_syscall_64+0xf8/0x191 arch/x86/entry/common.c:282
[<ffffffff80fa963e>] entry_SYSCALL_64_after_swapgs+0x58/0xc6
Syzkaller reproducer:
# {Threaded:false Collide:false Repeat:false Procs:1 Sandbox:none Fault:false
FaultCall:-1 FaultNth:0 EnableTun:false UseTmpDir:false EnableCgroups:false
EnableNetdev:false ResetNet:false HandleSegv:false WaitRepeat:false Debug:false
Repro:false}
r0 = syz_open_procfs(0xffffffffffffffff, &(0x7f00000002c0)='attr/fscreate\x00')
write$cgroup_pid(r0, &(0x7f0000000180)=ANY=[
ANYBLOB<https://people.qualcomm.com/People?id=ANYBLOB>
="37393430ad3c35020000000000000023959c533574eab7032c45f6d7384f73ab"], 0x20)
syz_fuseblk_mount(&(0x7f0000000000)='./file0\x00',
&(0x7f0000000080)='./file0\x00', 0xe003, 0x0, 0x0, 0x100000000, 0x4, 0x0)
Please let me know if something is wrong with the way I am seeing this.
Regards,
Sachin Grover
________________________________
From: Stephen Smalley <s...@tycho.nsa.gov>
Sent: Monday, August 13, 2018 6:37 PM
To: Sachin Grover; selinux@tycho.nsa.gov; Paul Moore
Subject: Re: Possible OOB Read in Kernel Heap Memory in call to
ext4_xattr_set_entry()
On 08/13/2018 08:59 AM, Sachin Grover wrote:
> I agree with you that it cannot be exploitable on Android, but Kasan is
> able to find it as OOB if I run syzkaller on x86 based VM image. My last
> commit is actually only fixing one path, but there are multiple path
> which are having same issue, so it would be better if fix is given in
> context_struct_to_string() where length is calculated.
>
> Let me know what is your take on this.
Are you sure that your earlier commit is included in the kernels you are
testing? If so, can you provide one of the other code paths not
addressed by your earlier commit? It appears that context.len is only
ever set by security_context_to_sid_core(), and there it is set to
strlen(str)+1 after your commit (and context.str is set to str, which is
the result of kstrdup of the original context). Thus, when we reach
context_struct_to_string() later, context->len can only be
strlen(context->str)+1 AFAICS. What am I missing?
>
>
>
>
> From: Stephen Smalley
> Sent: Monday 13 August, 18:05
> Subject: Re: Possible OOB Read in Kernel Heap Memory in call to
> ext4_xattr_set_entry()
> To: Sachin Grover, selinux@tycho.nsa.gov, Paul Moore
>
>
> On 08/13/2018 08:23 AM, Stephen Smalley wrote: > On 08/13/2018 01:19 AM,
> Sachin Grover wrote: >> Hi Stephen/Paul, >> >> This issue was discovered
> using >> https://android.googlesource.com/kernel/common -b
> android-4.9-o, but >> I've verified the code path exists in msm-4.4. It
> likely exists in >> other kernel versions as well. >> >> As a privileged
> user, one can override the current SELinux context via >> a call to
> setfscreatecon() or by writing directly to >> /proc/self/attr/fscreate.
> The context is then used to label the next >> newly created file object,
> e.g. mknod(). Upon handling the creation of >> the new object on the
> EXT4 FS we end up calling >> selinux_inode_init_security() to create a
> new xattr object for the >> inode. This will end up calling
> context_struct_to_string() to convert >> the SID back to the string
> version of the SELinux security context >> that was stored. >> >> static
> int context_struct_to_string(struct context *context, char >>
> **scontext, u32 *scontext_len) >> { >> char *scontextp; >> >>
> if (scontext) >> *scontext = NULL; >> *scontext_len
> = 0; >> >> if (context->len) { >> *scontext_len =
> context->len; >> if (scontext) { >> *scontext
> = kstrdup(context->str, GFP_ATOMIC); >> if
> (!(*scontext)) >> return -ENOMEM; >> }
> >> return 0; >> } >> >> scontext is populated with the
> context length, directly from the >> context structure, and using
> kstrdup() to copy the string. Much later >> down the call stack
> ext4_xattr_set_entry() is called with the SELinux >> xattr object. >> >>
> >> if (i->value == EXT4_ZERO_XATTR_VALUE) { >>
> memset(val, 0, size); >> } else { >>
> /* Clear the pad bytes first. */ >> memset(val +
> size - EXT4_XATTR_PAD, 0, >> EXT4_XATTR_PAD);
> >> memcpy(val, i->value, i->value_len); >>
> } >> >> SELinux allows for the creation of contexts that include
> NULL bytes. >> Unfortunately in this case if a NULL byte occurs not as
> the string >> terminator, then kstrdup() will allocate a memory region
> smaller than >> what context->len represents. Later in
> ext4_xattr_set_entry() the >> original context->len will be used in
> memcpy() and will exceed the >> heap memory allocated by kstrdup()
> leading to an OOB issue. There may >> also be the potential for memory
> corruption depending on the total >> length of the SELinux context, but
> this would require further analysis. >> >> Can you please let me know if
> my analysis is correct. >> As per me, it looks like there is problem in
> the code. > > Your earlier commit
> efe3de79e0b52ca281ef6691480c8c68c82a4657 ensures > that context->len is
> only ever set to strlen(str)+1. So is it still > possible for this
> condition to occur? Also, as with the earlier bug, this one is only
> exploitable for a process with CAP_MAC_ADMIN that is allowed mac_admin
> permission in SELinux policy, and this is prohibited in Android policy
> via neverallow and checked by CTS.
>
_______________________________________________
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
