|
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=[ ="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 <sds@xxxxxxxxxxxxx>
Sent: Monday, August 13, 2018 6:37 PM To: Sachin Grover; selinux@xxxxxxxxxxxxx; 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@xxxxxxxxxxxxx, 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@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
