LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity
@ 2018-05-25  8:31 Sachin Grover
  2018-05-30 15:19 ` Paul Moore
  0 siblings, 1 reply; 4+ messages in thread
From: Sachin Grover @ 2018-05-25  8:31 UTC (permalink / raw)
  To: paul, sds; +Cc: linux-security-module, linux-kernel, selinux, Sachin Grover

Call trace:
 [<ffffff9203a8d7a8>] dump_backtrace+0x0/0x428
 [<ffffff9203a8dbf8>] show_stack+0x28/0x38
 [<ffffff920409bfb8>] dump_stack+0xd4/0x124
 [<ffffff9203d187e8>] print_address_description+0x68/0x258
 [<ffffff9203d18c00>] kasan_report.part.2+0x228/0x2f0
 [<ffffff9203d1927c>] kasan_report+0x5c/0x70
 [<ffffff9203d1776c>] check_memory_region+0x12c/0x1c0
 [<ffffff9203d17cdc>] memcpy+0x34/0x68
 [<ffffff9203d75348>] xattr_getsecurity+0xe0/0x160
 [<ffffff9203d75490>] vfs_getxattr+0xc8/0x120
 [<ffffff9203d75d68>] getxattr+0x100/0x2c8
 [<ffffff9203d76fb4>] SyS_fgetxattr+0x64/0xa0
 [<ffffff9203a83f70>] el0_svc_naked+0x24/0x28

If user get root access and calls security.selinux setxattr() with an
embedded NUL on a file and then if some process performs a getxattr()
on that file with a length greater than the actual length of the string,
it would result in a panic.

To fix this, add the actual length of the string to the security context
instead of the length passed by the userspace process.

Signed-off-by: Sachin Grover <sgrover@codeaurora.org>
---
 security/selinux/ss/services.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 66ea81c..d17f5b4 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1434,7 +1434,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 				      scontext_len, &context, def_sid);
 	if (rc == -EINVAL && force) {
 		context.str = str;
-		context.len = scontext_len;
+		context.len = strlen(str) + 1;
 		str = NULL;
 	} else if (rc)
 		goto out_unlock;
-- 
1.9.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity
  2018-05-25  8:31 [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity Sachin Grover
@ 2018-05-30 15:19 ` Paul Moore
  2018-05-30 15:23   ` Stephen Smalley
  0 siblings, 1 reply; 4+ messages in thread
From: Paul Moore @ 2018-05-30 15:19 UTC (permalink / raw)
  To: Sachin Grover
  Cc: Stephen Smalley, linux-security-module, linux-kernel, selinux

On Fri, May 25, 2018 at 4:31 AM, Sachin Grover <sgrover@codeaurora.org> wrote:
> Call trace:
>  [<ffffff9203a8d7a8>] dump_backtrace+0x0/0x428
>  [<ffffff9203a8dbf8>] show_stack+0x28/0x38
>  [<ffffff920409bfb8>] dump_stack+0xd4/0x124
>  [<ffffff9203d187e8>] print_address_description+0x68/0x258
>  [<ffffff9203d18c00>] kasan_report.part.2+0x228/0x2f0
>  [<ffffff9203d1927c>] kasan_report+0x5c/0x70
>  [<ffffff9203d1776c>] check_memory_region+0x12c/0x1c0
>  [<ffffff9203d17cdc>] memcpy+0x34/0x68
>  [<ffffff9203d75348>] xattr_getsecurity+0xe0/0x160
>  [<ffffff9203d75490>] vfs_getxattr+0xc8/0x120
>  [<ffffff9203d75d68>] getxattr+0x100/0x2c8
>  [<ffffff9203d76fb4>] SyS_fgetxattr+0x64/0xa0
>  [<ffffff9203a83f70>] el0_svc_naked+0x24/0x28
>
> If user get root access and calls security.selinux setxattr() with an
> embedded NUL on a file and then if some process performs a getxattr()
> on that file with a length greater than the actual length of the string,
> it would result in a panic.
>
> To fix this, add the actual length of the string to the security context
> instead of the length passed by the userspace process.
>
> Signed-off-by: Sachin Grover <sgrover@codeaurora.org>
> ---
>  security/selinux/ss/services.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks for reporting this and providing a patch.  It's small enough,
and passes all the regular tests, so I've merged it into
selinux/stable-4.17 (adding the stable metadata) and I'm going to send
it up to Linus today.

If Linus doesn't pull the fix in time for v4.17 I'll send it up during
the upcoming merge window.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 66ea81c..d17f5b4 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1434,7 +1434,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
>                                       scontext_len, &context, def_sid);
>         if (rc == -EINVAL && force) {
>                 context.str = str;
> -               context.len = scontext_len;
> +               context.len = strlen(str) + 1;
>                 str = NULL;
>         } else if (rc)
>                 goto out_unlock;
> --
> 1.9.1

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity
  2018-05-30 15:19 ` Paul Moore
@ 2018-05-30 15:23   ` Stephen Smalley
  2018-05-30 15:31     ` Paul Moore
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Smalley @ 2018-05-30 15:23 UTC (permalink / raw)
  To: Paul Moore, Sachin Grover; +Cc: linux-security-module, linux-kernel, selinux

On 05/30/2018 11:19 AM, Paul Moore wrote:
> On Fri, May 25, 2018 at 4:31 AM, Sachin Grover <sgrover@codeaurora.org> wrote:
>> Call trace:
>>  [<ffffff9203a8d7a8>] dump_backtrace+0x0/0x428
>>  [<ffffff9203a8dbf8>] show_stack+0x28/0x38
>>  [<ffffff920409bfb8>] dump_stack+0xd4/0x124
>>  [<ffffff9203d187e8>] print_address_description+0x68/0x258
>>  [<ffffff9203d18c00>] kasan_report.part.2+0x228/0x2f0
>>  [<ffffff9203d1927c>] kasan_report+0x5c/0x70
>>  [<ffffff9203d1776c>] check_memory_region+0x12c/0x1c0
>>  [<ffffff9203d17cdc>] memcpy+0x34/0x68
>>  [<ffffff9203d75348>] xattr_getsecurity+0xe0/0x160
>>  [<ffffff9203d75490>] vfs_getxattr+0xc8/0x120
>>  [<ffffff9203d75d68>] getxattr+0x100/0x2c8
>>  [<ffffff9203d76fb4>] SyS_fgetxattr+0x64/0xa0
>>  [<ffffff9203a83f70>] el0_svc_naked+0x24/0x28
>>
>> If user get root access and calls security.selinux setxattr() with an
>> embedded NUL on a file and then if some process performs a getxattr()
>> on that file with a length greater than the actual length of the string,
>> it would result in a panic.
>>
>> To fix this, add the actual length of the string to the security context
>> instead of the length passed by the userspace process.
>>
>> Signed-off-by: Sachin Grover <sgrover@codeaurora.org>
>> ---
>>  security/selinux/ss/services.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thanks for reporting this and providing a patch.  It's small enough,
> and passes all the regular tests, so I've merged it into
> selinux/stable-4.17 (adding the stable metadata) and I'm going to send
> it up to Linus today.
> 
> If Linus doesn't pull the fix in time for v4.17 I'll send it up during
> the upcoming merge window.

NB Such a setxattr() call can only be performed by a process with CAP_MAC_ADMIN that is also allowed mac_admin permission in SELinux policy. Consequently, this is never possible on Android (no process is allowed mac_admin permission, always enforcing) and is only possible in Fedora/RHEL for a few domains (if enforcing).

Fixes: 9a59daa03df72526d234b91dd3e32ded5aebd3ef ("SELinux: fix sleeping allocation in security_context_to_sid")

> 
>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>> index 66ea81c..d17f5b4 100644
>> --- a/security/selinux/ss/services.c
>> +++ b/security/selinux/ss/services.c
>> @@ -1434,7 +1434,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
>>                                       scontext_len, &context, def_sid);
>>         if (rc == -EINVAL && force) {
>>                 context.str = str;
>> -               context.len = scontext_len;
>> +               context.len = strlen(str) + 1;
>>                 str = NULL;
>>         } else if (rc)
>>                 goto out_unlock;
>> --
>> 1.9.1
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity
  2018-05-30 15:23   ` Stephen Smalley
@ 2018-05-30 15:31     ` Paul Moore
  0 siblings, 0 replies; 4+ messages in thread
From: Paul Moore @ 2018-05-30 15:31 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Sachin Grover, linux-security-module, linux-kernel, selinux

On Wed, May 30, 2018 at 11:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 05/30/2018 11:19 AM, Paul Moore wrote:
>> On Fri, May 25, 2018 at 4:31 AM, Sachin Grover <sgrover@codeaurora.org> wrote:
>>> Call trace:
>>>  [<ffffff9203a8d7a8>] dump_backtrace+0x0/0x428
>>>  [<ffffff9203a8dbf8>] show_stack+0x28/0x38
>>>  [<ffffff920409bfb8>] dump_stack+0xd4/0x124
>>>  [<ffffff9203d187e8>] print_address_description+0x68/0x258
>>>  [<ffffff9203d18c00>] kasan_report.part.2+0x228/0x2f0
>>>  [<ffffff9203d1927c>] kasan_report+0x5c/0x70
>>>  [<ffffff9203d1776c>] check_memory_region+0x12c/0x1c0
>>>  [<ffffff9203d17cdc>] memcpy+0x34/0x68
>>>  [<ffffff9203d75348>] xattr_getsecurity+0xe0/0x160
>>>  [<ffffff9203d75490>] vfs_getxattr+0xc8/0x120
>>>  [<ffffff9203d75d68>] getxattr+0x100/0x2c8
>>>  [<ffffff9203d76fb4>] SyS_fgetxattr+0x64/0xa0
>>>  [<ffffff9203a83f70>] el0_svc_naked+0x24/0x28
>>>
>>> If user get root access and calls security.selinux setxattr() with an
>>> embedded NUL on a file and then if some process performs a getxattr()
>>> on that file with a length greater than the actual length of the string,
>>> it would result in a panic.
>>>
>>> To fix this, add the actual length of the string to the security context
>>> instead of the length passed by the userspace process.
>>>
>>> Signed-off-by: Sachin Grover <sgrover@codeaurora.org>
>>> ---
>>>  security/selinux/ss/services.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Thanks for reporting this and providing a patch.  It's small enough,
>> and passes all the regular tests, so I've merged it into
>> selinux/stable-4.17 (adding the stable metadata) and I'm going to send
>> it up to Linus today.
>>
>> If Linus doesn't pull the fix in time for v4.17 I'll send it up during
>> the upcoming merge window.
>
> NB Such a setxattr() call can only be performed by a process with CAP_MAC_ADMIN that is also allowed mac_admin permission in SELinux policy. Consequently, this is never possible on Android (no process is allowed mac_admin permission, always enforcing) and is only possible in Fedora/RHEL for a few domains (if enforcing).

Yes the risk is small, and if it wasn't such a trivial and
self-contained patch I probably would have just deferred it for the
merge window, but considering everything I think there is value in
getting this in for v4.17.  If Linus decides not to merge this into
v4.17 I think that is okay too.

> Fixes: 9a59daa03df72526d234b91dd3e32ded5aebd3ef ("SELinux: fix sleeping allocation in security_context_to_sid")
>
>>
>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>> index 66ea81c..d17f5b4 100644
>>> --- a/security/selinux/ss/services.c
>>> +++ b/security/selinux/ss/services.c
>>> @@ -1434,7 +1434,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
>>>                                       scontext_len, &context, def_sid);
>>>         if (rc == -EINVAL && force) {
>>>                 context.str = str;
>>> -               context.len = scontext_len;
>>> +               context.len = strlen(str) + 1;
>>>                 str = NULL;
>>>         } else if (rc)
>>>                 goto out_unlock;
>>> --
>>> 1.9.1

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-05-30 15:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-25  8:31 [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity Sachin Grover
2018-05-30 15:19 ` Paul Moore
2018-05-30 15:23   ` Stephen Smalley
2018-05-30 15:31     ` Paul Moore

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).