Re: Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
On 08/20/2018 10:02 AM, Stephen Smalley wrote:
On 08/20/2018 02:29 AM, Sachin Grover wrote:
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.
context_struct_to_string() sets *scontext_len to context->len, which was
previously set by security_context_to_sid_core() to strlen(str) + 1
after your prior commit. Correct?
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;
This audit-related code has to deal with the possibility that there is
an embedded NUL (and might also lack a terminating NUL) because it is
dealing with the original user-supplied value, not a string that has
been stored in a context structure. After this audit-related code, we
proceed to call security_context_to_sid_force() which calls
security_context_to_sid_core() with force=1. This first creates a
NUL-terminated copy of the value (scontext) as scontext2 using
kmemdup_nul(), which could potentially contain embedded NULs if the
original did. In the force=1 case, it then creates a copy of scontext2
as str using kstrdup(), yielding a NUL-terminated string with no
embedded NULs. It is this NUL-terminated string (str) that is stored in
the context structure as context.str and context.len is set to
strlen(str)+1 after your earlier commit.
How would we end up with a context->len other than
strlen(context->str)+1 in security_sid_to_context_core()?
The point is that we ought to sanitize context->len when it is set, not
when it is read, and in theory that is exactly what we are doing in
security_context_to_sid_core(), such that security_sid_to_context_core()
can read that value without recomputing it. If it is being set
incorrectly somewhere else, we ought to fix it there. If it is being
corrupted through memory corruption, then we ought to fix the source of
the memory corruption.
Call StacK:
[] kasan_report+0x32c/0x485 mm/kasan/report.c:309
[] check_memory_region_inline mm/kasan/kasan.c:301
[inline]
[] check_memory_region+0x2b/0x130 mm/kasan/kasan.c:315
[] __asan_loadN+0xf/0x11 mm/kasan/kasan.c:745
[] ext4_xattr_set_entry+0x6a1/0x6d3 fs/ext4/xattr.c:747
[] ext4_xattr_ibody_set.isra.9+0x4f/0x118
fs/ext4/xattr.c:1118
[] ext4_xattr_set_handle+0x39a/0x7b4
fs/ext4/xattr.c:1224
[] ext4_initxattrs+0x6b/0x96
fs/ext4/xattr_security.c:42
[] security_inode_init_security+0x170/0x1c9
security/security.c:403
[] ext4_init_security+0x35/0x3d
fs/ext4/xattr_security.c:56
[] __ext4_new_inode+0x1f24/0x2266 fs/ext4/ialloc.c:1059
[] ext4_mknod+0x11a/0x263 fs/ext4/namei.c:2510
[] vfs_mknod2+0x1ee/0x29f fs/namei.c:3722
[] SYSC_mknodat fs/namei.c:3780 [inline]
[] SyS_mknodat+0x19a/0x22a fs/namei.c:3752
[] do_syscall_64+0xf8/0x191 arch/x86/entry/common.c:282
[] 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 =
Re: Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
On 08/20/2018 02:29 AM, Sachin Grover wrote:
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.
context_struct_to_string() sets *scontext_len to context->len, which was
previously set by security_context_to_sid_core() to strlen(str) + 1
after your prior commit. Correct?
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;
This audit-related code has to deal with the possibility that there is
an embedded NUL (and might also lack a terminating NUL) because it is
dealing with the original user-supplied value, not a string that has
been stored in a context structure. After this audit-related code, we
proceed to call security_context_to_sid_force() which calls
security_context_to_sid_core() with force=1. This first creates a
NUL-terminated copy of the value (scontext) as scontext2 using
kmemdup_nul(), which could potentially contain embedded NULs if the
original did. In the force=1 case, it then creates a copy of scontext2
as str using kstrdup(), yielding a NUL-terminated string with no
embedded NULs. It is this NUL-terminated string (str) that is stored in
the context structure as context.str and context.len is set to
strlen(str)+1 after your earlier commit.
How would we end up with a context->len other than
strlen(context->str)+1 in security_sid_to_context_core()?
The point is that we ought to sanitize context->len when it is set, not
when it is read, and in theory that is exactly what we are doing in
security_context_to_sid_core(), such that security_sid_to_context_core()
can read that value without recomputing it. If it is being set
incorrectly somewhere else, we ought to fix it there. If it is being
corrupted through memory corruption, then we ought to fix the source of
the memory corruption.
Call StacK:
[] kasan_report+0x32c/0x485 mm/kasan/report.c:309
[] check_memory_region_inline mm/kasan/kasan.c:301
[inline]
[] check_memory_region+0x2b/0x130 mm/kasan/kasan.c:315
[] __asan_loadN+0xf/0x11 mm/kasan/kasan.c:745
[] ext4_xattr_set_entry+0x6a1/0x6d3 fs/ext4/xattr.c:747
[] ext4_xattr_ibody_set.isra.9+0x4f/0x118
fs/ext4/xattr.c:1118
[] ext4_xattr_set_handle+0x39a/0x7b4 fs/ext4/xattr.c:1224
[] ext4_initxattrs+0x6b/0x96 fs/ext4/xattr_security.c:42
[] security_inode_init_security+0x170/0x1c9
security/security.c:403
[] ext4_init_security+0x35/0x3d
fs/ext4/xattr_security.c:56
[] __ext4_new_inode+0x1f24/0x2266 fs/ext4/ialloc.c:1059
[] ext4_mknod+0x11a/0x263 fs/ext4/namei.c:2510
[] vfs_mknod2+0x1ee/0x29f fs/namei.c:3722
[] SYSC_mknodat fs/namei.c:3780 [inline]
[] SyS_mknodat+0x19a/0x22a fs/namei.c:3752
[] do_syscall_64+0xf8/0x191 arch/x86/entry/common.c:282
[] 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(0x,
&(0x7f0002c0)='attr/fscreate\x00')
Re: Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
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:
[] kasan_report+0x32c/0x485 mm/kasan/report.c:309
[] check_memory_region_inline mm/kasan/kasan.c:301 [inline]
[] check_memory_region+0x2b/0x130 mm/kasan/kasan.c:315
[] __asan_loadN+0xf/0x11 mm/kasan/kasan.c:745
[] ext4_xattr_set_entry+0x6a1/0x6d3 fs/ext4/xattr.c:747
[] ext4_xattr_ibody_set.isra.9+0x4f/0x118 fs/ext4/xattr.c:1118
[] ext4_xattr_set_handle+0x39a/0x7b4 fs/ext4/xattr.c:1224
[] ext4_initxattrs+0x6b/0x96 fs/ext4/xattr_security.c:42
[] security_inode_init_security+0x170/0x1c9
security/security.c:403
[] ext4_init_security+0x35/0x3d fs/ext4/xattr_security.c:56
[] __ext4_new_inode+0x1f24/0x2266 fs/ext4/ialloc.c:1059
[] ext4_mknod+0x11a/0x263 fs/ext4/namei.c:2510
[] vfs_mknod2+0x1ee/0x29f fs/namei.c:3722
[] SYSC_mknodat fs/namei.c:3780 [inline]
[] SyS_mknodat+0x19a/0x22a fs/namei.c:3752
[] do_syscall_64+0xf8/0x191 arch/x86/entry/common.c:282
[] 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(0x, &(0x7f0002c0)='attr/fscreate\x00')
write$cgroup_pid(r0, &(0x7f000180)=ANY=[
ANYBLOB
="37393430ad3c35020023959c533574eab7032c45f6d7384f73ab"], 0x20)
syz_fuseblk_mount(&(0x7f00)='./file0\x00',
&(0x7f80)='./file0\x00', 0xe003, 0x0, 0x0, 0x1, 0x4, 0x0)
Please let me know if something is wrong with the way I am seeing this.
Regards,
Sachin Grover
From: Stephen Smalley
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
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.
Re: Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
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.
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.
Re: Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
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.
Re: Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
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?
>
Regards,
Sachin Grover
___
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.
Possible OOB Read in Kernel Heap Memory in call to ext4_xattr_set_entry()
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.
Regards,
Sachin Grover
___
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.
