* [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()
@ 2017-11-30 16:52 Paul Moore
2017-11-30 23:44 ` William Roberts
2017-12-01 0:33 ` James Morris
0 siblings, 2 replies; 6+ messages in thread
From: Paul Moore @ 2017-11-30 16:52 UTC (permalink / raw)
To: selinux
From: Paul Moore <paul@paul-moore.com>
The syzbot/syzkaller automated tests found a problem in
security_context_to_sid_core() during early boot (before we load the
SELinux policy) where we could potentially feed context strings without
NULL terminators into the strcmp() function.
We already guard against this during normal operation (after the SELinux
policy has been loaded) by making a copy of the context strings and
explicitly adding a NULL terminator to the end. The patch extends this
protection to the early boot case (no loaded policy) by moving the context
copy earlier in security_context_to_sid_core().
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
security/selinux/ss/services.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 33cfe5d3d6cb..6ec4cdc8af8f 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1413,27 +1413,27 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
if (!scontext_len)
return -EINVAL;
+ /* Copy the string to allow changes and ensure a NULL terminator */
+ scontext2 = kmalloc(scontext_len + 1, gfp_flags);
+ if (!scontext2)
+ return -ENOMEM;
+ memcpy(scontext2, scontext, scontext_len);
+ scontext2[scontext_len] = 0;
+
if (!ss_initialized) {
int i;
for (i = 1; i < SECINITSID_NUM; i++) {
- if (!strcmp(initial_sid_to_string[i], scontext)) {
+ if (!strcmp(initial_sid_to_string[i], scontext2)) {
*sid = i;
- return 0;
+ goto out;
}
}
*sid = SECINITSID_KERNEL;
- return 0;
+ goto out;
}
*sid = SECSID_NULL;
- /* Copy the string so that we can modify the copy as we parse it. */
- scontext2 = kmalloc(scontext_len + 1, gfp_flags);
- if (!scontext2)
- return -ENOMEM;
- memcpy(scontext2, scontext, scontext_len);
- scontext2[scontext_len] = 0;
-
if (force) {
/* Save another copy for storing in uninterpreted form */
rc = -ENOMEM;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()
2017-11-30 16:52 [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core() Paul Moore
@ 2017-11-30 23:44 ` William Roberts
2017-12-01 15:34 ` Paul Moore
2017-12-01 0:33 ` James Morris
1 sibling, 1 reply; 6+ messages in thread
From: William Roberts @ 2017-11-30 23:44 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux@tycho.nsa.gov
On Thu, Nov 30, 2017 at 8:52 AM, Paul Moore <pmoore@redhat.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> The syzbot/syzkaller automated tests found a problem in
> security_context_to_sid_core() during early boot (before we load the
> SELinux policy) where we could potentially feed context strings without
> NULL terminators into the strcmp() function.
>
> We already guard against this during normal operation (after the SELinux
> policy has been loaded) by making a copy of the context strings and
> explicitly adding a NULL terminator to the end. The patch extends this
> protection to the early boot case (no loaded policy) by moving the context
> copy earlier in security_context_to_sid_core().
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> security/selinux/ss/services.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 33cfe5d3d6cb..6ec4cdc8af8f 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1413,27 +1413,27 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
> if (!scontext_len)
> return -EINVAL;
>
> + /* Copy the string to allow changes and ensure a NULL terminator */
> + scontext2 = kmalloc(scontext_len + 1, gfp_flags);
> + if (!scontext2)
> + return -ENOMEM;
> + memcpy(scontext2, scontext, scontext_len);
> + scontext2[scontext_len] = 0;
Call me crazy, but can't we use kmemdup_nul() here?
/**
* kmemdup_nul - Create a NUL-terminated string from unterminated data
* @s: The data to stringify
* @len: The size of the data
* @gfp: the GFP mask used in the kmalloc() call when allocating memory
*/
char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
> +
> if (!ss_initialized) {
> int i;
>
> for (i = 1; i < SECINITSID_NUM; i++) {
> - if (!strcmp(initial_sid_to_string[i], scontext)) {
> + if (!strcmp(initial_sid_to_string[i], scontext2)) {
> *sid = i;
> - return 0;
> + goto out;
> }
> }
> *sid = SECINITSID_KERNEL;
> - return 0;
> + goto out;
> }
> *sid = SECSID_NULL;
>
> - /* Copy the string so that we can modify the copy as we parse it. */
> - scontext2 = kmalloc(scontext_len + 1, gfp_flags);
> - if (!scontext2)
> - return -ENOMEM;
> - memcpy(scontext2, scontext, scontext_len);
> - scontext2[scontext_len] = 0;
> -
> if (force) {
> /* Save another copy for storing in uninterpreted form */
> rc = -ENOMEM;
>
>
--
Respectfully,
William C Roberts
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()
2017-11-30 16:52 [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core() Paul Moore
2017-11-30 23:44 ` William Roberts
@ 2017-12-01 0:33 ` James Morris
1 sibling, 0 replies; 6+ messages in thread
From: James Morris @ 2017-12-01 0:33 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux
On Thu, 30 Nov 2017, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> The syzbot/syzkaller automated tests found a problem in
> security_context_to_sid_core() during early boot (before we load the
> SELinux policy) where we could potentially feed context strings without
> NULL terminators into the strcmp() function.
>
> We already guard against this during normal operation (after the SELinux
> policy has been loaded) by making a copy of the context strings and
> explicitly adding a NULL terminator to the end. The patch extends this
> protection to the early boot case (no loaded policy) by moving the context
> copy earlier in security_context_to_sid_core().
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
Reviewed-by: James Morris <james.l.morris@oracle.com>
--
James Morris
<james.l.morris@oracle.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()
2017-11-30 23:44 ` William Roberts
@ 2017-12-01 15:34 ` Paul Moore
2017-12-01 19:20 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2017-12-01 15:34 UTC (permalink / raw)
To: William Roberts; +Cc: selinux@tycho.nsa.gov
On Thu, Nov 30, 2017 at 6:44 PM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> On Thu, Nov 30, 2017 at 8:52 AM, Paul Moore <pmoore@redhat.com> wrote:
>> From: Paul Moore <paul@paul-moore.com>
>>
>> The syzbot/syzkaller automated tests found a problem in
>> security_context_to_sid_core() during early boot (before we load the
>> SELinux policy) where we could potentially feed context strings without
>> NULL terminators into the strcmp() function.
>>
>> We already guard against this during normal operation (after the SELinux
>> policy has been loaded) by making a copy of the context strings and
>> explicitly adding a NULL terminator to the end. The patch extends this
>> protection to the early boot case (no loaded policy) by moving the context
>> copy earlier in security_context_to_sid_core().
>>
>> Reported-by: syzbot <syzkaller@googlegroups.com>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>> security/selinux/ss/services.c | 20 ++++++++++----------
>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>> index 33cfe5d3d6cb..6ec4cdc8af8f 100644
>> --- a/security/selinux/ss/services.c
>> +++ b/security/selinux/ss/services.c
>> @@ -1413,27 +1413,27 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
>> if (!scontext_len)
>> return -EINVAL;
>>
>> + /* Copy the string to allow changes and ensure a NULL terminator */
>> + scontext2 = kmalloc(scontext_len + 1, gfp_flags);
>> + if (!scontext2)
>> + return -ENOMEM;
>> + memcpy(scontext2, scontext, scontext_len);
>> + scontext2[scontext_len] = 0;
>
> Call me crazy, but can't we use kmemdup_nul() here?
Crazy good idea ;)
I didn't realize that function existed, I'll respin. Thanks.
> /**
> * kmemdup_nul - Create a NUL-terminated string from unterminated data
> * @s: The data to stringify
> * @len: The size of the data
> * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> */
> char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
--
paul moore
security @ redhat
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()
2017-12-01 15:34 ` Paul Moore
@ 2017-12-01 19:20 ` Stephen Smalley
2017-12-01 21:32 ` Paul Moore
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2017-12-01 19:20 UTC (permalink / raw)
To: Paul Moore, William Roberts; +Cc: selinux@tycho.nsa.gov
On Fri, 2017-12-01 at 10:34 -0500, Paul Moore wrote:
> On Thu, Nov 30, 2017 at 6:44 PM, William Roberts
> <bill.c.roberts@gmail.com> wrote:
> > On Thu, Nov 30, 2017 at 8:52 AM, Paul Moore <pmoore@redhat.com>
> > wrote:
> > > From: Paul Moore <paul@paul-moore.com>
> > >
> > > The syzbot/syzkaller automated tests found a problem in
> > > security_context_to_sid_core() during early boot (before we load
> > > the
> > > SELinux policy) where we could potentially feed context strings
> > > without
> > > NULL terminators into the strcmp() function.
> > >
> > > We already guard against this during normal operation (after the
> > > SELinux
> > > policy has been loaded) by making a copy of the context strings
> > > and
> > > explicitly adding a NULL terminator to the end. The patch
> > > extends this
> > > protection to the early boot case (no loaded policy) by moving
> > > the context
> > > copy earlier in security_context_to_sid_core().
> > >
> > > Reported-by: syzbot <syzkaller@googlegroups.com>
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > ---
> > > security/selinux/ss/services.c | 20 ++++++++++----------
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/security/selinux/ss/services.c
> > > b/security/selinux/ss/services.c
> > > index 33cfe5d3d6cb..6ec4cdc8af8f 100644
> > > --- a/security/selinux/ss/services.c
> > > +++ b/security/selinux/ss/services.c
> > > @@ -1413,27 +1413,27 @@ static int
> > > security_context_to_sid_core(const char *scontext, u32
> > > scontext_len,
> > > if (!scontext_len)
> > > return -EINVAL;
> > >
> > > + /* Copy the string to allow changes and ensure a NULL
> > > terminator */
> > > + scontext2 = kmalloc(scontext_len + 1, gfp_flags);
> > > + if (!scontext2)
> > > + return -ENOMEM;
> > > + memcpy(scontext2, scontext, scontext_len);
> > > + scontext2[scontext_len] = 0;
> >
> > Call me crazy, but can't we use kmemdup_nul() here?
>
> Crazy good idea ;)
>
> I didn't realize that function existed, I'll respin. Thanks.
Also note that it should be NUL not NULL in the patch subject and
description. '\0' vs (void*)0
>
> > /**
> > * kmemdup_nul - Create a NUL-terminated string from unterminated
> > data
> > * @s: The data to stringify
> > * @len: The size of the data
> > * @gfp: the GFP mask used in the kmalloc() call when allocating
> > memory
> > */
> > char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core()
2017-12-01 19:20 ` Stephen Smalley
@ 2017-12-01 21:32 ` Paul Moore
0 siblings, 0 replies; 6+ messages in thread
From: Paul Moore @ 2017-12-01 21:32 UTC (permalink / raw)
To: Stephen Smalley; +Cc: William Roberts, selinux@tycho.nsa.gov
On Fri, Dec 1, 2017 at 2:20 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Fri, 2017-12-01 at 10:34 -0500, Paul Moore wrote:
>> On Thu, Nov 30, 2017 at 6:44 PM, William Roberts
>> <bill.c.roberts@gmail.com> wrote:
>> > On Thu, Nov 30, 2017 at 8:52 AM, Paul Moore <pmoore@redhat.com>
>> > wrote:
>> > > From: Paul Moore <paul@paul-moore.com>
>> > >
>> > > The syzbot/syzkaller automated tests found a problem in
>> > > security_context_to_sid_core() during early boot (before we load
>> > > the
>> > > SELinux policy) where we could potentially feed context strings
>> > > without
>> > > NULL terminators into the strcmp() function.
>> > >
>> > > We already guard against this during normal operation (after the
>> > > SELinux
>> > > policy has been loaded) by making a copy of the context strings
>> > > and
>> > > explicitly adding a NULL terminator to the end. The patch
>> > > extends this
>> > > protection to the early boot case (no loaded policy) by moving
>> > > the context
>> > > copy earlier in security_context_to_sid_core().
>> > >
>> > > Reported-by: syzbot <syzkaller@googlegroups.com>
>> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
>> > > ---
>> > > security/selinux/ss/services.c | 20 ++++++++++----------
>> > > 1 file changed, 10 insertions(+), 10 deletions(-)
>> > >
>> > > diff --git a/security/selinux/ss/services.c
>> > > b/security/selinux/ss/services.c
>> > > index 33cfe5d3d6cb..6ec4cdc8af8f 100644
>> > > --- a/security/selinux/ss/services.c
>> > > +++ b/security/selinux/ss/services.c
>> > > @@ -1413,27 +1413,27 @@ static int
>> > > security_context_to_sid_core(const char *scontext, u32
>> > > scontext_len,
>> > > if (!scontext_len)
>> > > return -EINVAL;
>> > >
>> > > + /* Copy the string to allow changes and ensure a NULL
>> > > terminator */
>> > > + scontext2 = kmalloc(scontext_len + 1, gfp_flags);
>> > > + if (!scontext2)
>> > > + return -ENOMEM;
>> > > + memcpy(scontext2, scontext, scontext_len);
>> > > + scontext2[scontext_len] = 0;
>> >
>> > Call me crazy, but can't we use kmemdup_nul() here?
>>
>> Crazy good idea ;)
>>
>> I didn't realize that function existed, I'll respin. Thanks.
>
> Also note that it should be NUL not NULL in the patch subject and
> description. '\0' vs (void*)0
Yes, thanks. Muscle memory has me just typing "NULL" everywhere,
regardless of the context.
--
paul moore
security @ redhat
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-01 21:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-30 16:52 [PATCH] selinux: ensure the context is NULL terminated in security_context_to_sid_core() Paul Moore
2017-11-30 23:44 ` William Roberts
2017-12-01 15:34 ` Paul Moore
2017-12-01 19:20 ` Stephen Smalley
2017-12-01 21:32 ` Paul Moore
2017-12-01 0:33 ` James Morris
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.