[CVE-2017-2618][T/Y SRU] selinux: fix off-by-one in setprocattr

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[CVE-2017-2618][T/Y SRU] selinux: fix off-by-one in setprocattr

Po-Hsu Lin (Sam)
This patch can be cherry-picked for both Trusty and Yakkety.
Other release are not affected.

Stephen Smalley (1):
  selinux: fix off-by-one in setprocattr

 security/selinux/hooks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
1.7.9.5


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[CVE-2017-2618][PATCH T/Y] selinux: fix off-by-one in setprocattr

Po-Hsu Lin (Sam)
From: Stephen Smalley <[hidden email]>

CVE-2017-2618

SELinux tries to support setting/clearing of /proc/pid/attr attributes
from the shell by ignoring terminating newlines and treating an
attribute value that begins with a NUL or newline as an attempt to
clear the attribute.  However, the test for clearing attributes has
always been wrong; it has an off-by-one error, and this could further
lead to reading past the end of the allocated buffer since commit
bb646cdb12e75d82258c2f2e7746d5952d3e321a ("proc_pid_attr_write():
switch to memdup_user()").  Fix the off-by-one error.

Even with this fix, setting and clearing /proc/pid/attr attributes
from the shell is not straightforward since the interface does not
support multiple write() calls (so shells that write the value and
newline separately will set and then immediately clear the attribute,
requiring use of echo -n to set the attribute), whereas trying to use
echo -n "" to clear the attribute causes the shell to skip the
write() call altogether since POSIX says that a zero-length write
causes no side effects. Thus, one must use echo -n to set and echo
without -n to clear, as in the following example:
$ echo -n unconfined_u:object_r:user_home_t:s0 > /proc/$$/attr/fscreate
$ cat /proc/$$/attr/fscreate
unconfined_u:object_r:user_home_t:s0
$ echo "" > /proc/$$/attr/fscreate
$ cat /proc/$$/attr/fscreate

Note the use of /proc/$$ rather than /proc/self, as otherwise
the cat command will read its own attribute value, not that of the shell.

There are no users of this facility to my knowledge; possibly we
should just get rid of it.

UPDATE: Upon further investigation it appears that a local process
with the process:setfscreate permission can cause a kernel panic as a
result of this bug.  This patch fixes CVE-2017-2618.

Signed-off-by: Stephen Smalley <[hidden email]>
[PM: added the update about CVE-2017-2618 to the commit description]
Cc: [hidden email] # 3.5: d6ea83ec6864e
Signed-off-by: Paul Moore <[hidden email]>

Signed-off-by: James Morris <[hidden email]>
(cherry picked from commit 0c461cb727d146c9ef2d3e86214f498b78b7d125)

Signed-off-by: Po-Hsu Lin <[hidden email]>
---
 security/selinux/hooks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 357762a..d2246a8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5548,7 +5548,7 @@ static int selinux_setprocattr(struct task_struct *p,
  return error;
 
  /* Obtain a SID for the context, if one was specified. */
- if (size && str[1] && str[1] != '\n') {
+ if (size && str[0] && str[0] != '\n') {
  if (str[size-1] == '\n') {
  str[size-1] = 0;
  size--;
--
1.7.9.5


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CVE-2017-2618][PATCH T/Y] selinux: fix off-by-one in setprocattr

Kai-Heng Feng
Hi,

On Fri, Jun 30, 2017 at 3:21 PM, Po-Hsu Lin <[hidden email]> wrote:

> From: Stephen Smalley <[hidden email]>
>
> CVE-2017-2618
>
> SELinux tries to support setting/clearing of /proc/pid/attr attributes
> from the shell by ignoring terminating newlines and treating an
> attribute value that begins with a NUL or newline as an attempt to
> clear the attribute.  However, the test for clearing attributes has
> always been wrong; it has an off-by-one error, and this could further
> lead to reading past the end of the allocated buffer since commit
> bb646cdb12e75d82258c2f2e7746d5952d3e321a ("proc_pid_attr_write():
> switch to memdup_user()").  Fix the off-by-one error.

I can't find commit bb646cdb12e75d82258c2f2e7746d5952d3e321a in Trusty.

IIUC the test case is wrong, but the CVE can only be triggered after
that particular commit.
So, I guess we don't need to apply it to Trusty. Can you double confirm for me?

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CVE-2017-2618][PATCH T/Y] selinux: fix off-by-one in setprocattr

Po-Hsu Lin (Sam)
Hi,

Yes this commit (bb646cdb) does not exist in Trusty.

I sent this for Trusty is because the security/selinux/hooks.c still
suffer from the off-by-one error. Not sure if we should only do this
on Yakkety.

Regarding the test case, you're mentioning the example in the description?

Thanks!

On Fri, Jun 30, 2017 at 5:32 PM, Kai-Heng Feng
<[hidden email]> wrote:

> Hi,
>
> On Fri, Jun 30, 2017 at 3:21 PM, Po-Hsu Lin <[hidden email]> wrote:
>> From: Stephen Smalley <[hidden email]>
>>
>> CVE-2017-2618
>>
>> SELinux tries to support setting/clearing of /proc/pid/attr attributes
>> from the shell by ignoring terminating newlines and treating an
>> attribute value that begins with a NUL or newline as an attempt to
>> clear the attribute.  However, the test for clearing attributes has
>> always been wrong; it has an off-by-one error, and this could further
>> lead to reading past the end of the allocated buffer since commit
>> bb646cdb12e75d82258c2f2e7746d5952d3e321a ("proc_pid_attr_write():
>> switch to memdup_user()").  Fix the off-by-one error.
>
> I can't find commit bb646cdb12e75d82258c2f2e7746d5952d3e321a in Trusty.
>
> IIUC the test case is wrong, but the CVE can only be triggered after
> that particular commit.
> So, I guess we don't need to apply it to Trusty. Can you double confirm for me?

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK/cmnt: [CVE-2017-2618][PATCH T/Y] selinux: fix off-by-one in setprocattr

Stefan Bader-2
In reply to this post by Po-Hsu Lin (Sam)
On 30.06.2017 09:21, Po-Hsu Lin wrote:

> From: Stephen Smalley <[hidden email]>
>
> CVE-2017-2618
>
> SELinux tries to support setting/clearing of /proc/pid/attr attributes
> from the shell by ignoring terminating newlines and treating an
> attribute value that begins with a NUL or newline as an attempt to
> clear the attribute.  However, the test for clearing attributes has
> always been wrong; it has an off-by-one error, and this could further
> lead to reading past the end of the allocated buffer since commit
> bb646cdb12e75d82258c2f2e7746d5952d3e321a ("proc_pid_attr_write():
> switch to memdup_user()").  Fix the off-by-one error.
>
> Even with this fix, setting and clearing /proc/pid/attr attributes
> from the shell is not straightforward since the interface does not
> support multiple write() calls (so shells that write the value and
> newline separately will set and then immediately clear the attribute,
> requiring use of echo -n to set the attribute), whereas trying to use
> echo -n "" to clear the attribute causes the shell to skip the
> write() call altogether since POSIX says that a zero-length write
> causes no side effects. Thus, one must use echo -n to set and echo
> without -n to clear, as in the following example:
> $ echo -n unconfined_u:object_r:user_home_t:s0 > /proc/$$/attr/fscreate
> $ cat /proc/$$/attr/fscreate
> unconfined_u:object_r:user_home_t:s0
> $ echo "" > /proc/$$/attr/fscreate
> $ cat /proc/$$/attr/fscreate
>
> Note the use of /proc/$$ rather than /proc/self, as otherwise
> the cat command will read its own attribute value, not that of the shell.
>
> There are no users of this facility to my knowledge; possibly we
> should just get rid of it.
>
> UPDATE: Upon further investigation it appears that a local process
> with the process:setfscreate permission can cause a kernel panic as a
> result of this bug.  This patch fixes CVE-2017-2618.
>
> Signed-off-by: Stephen Smalley <[hidden email]>
> [PM: added the update about CVE-2017-2618 to the commit description]
> Cc: [hidden email] # 3.5: d6ea83ec6864e
> Signed-off-by: Paul Moore <[hidden email]>
>
> Signed-off-by: James Morris <[hidden email]>
> (cherry picked from commit 0c461cb727d146c9ef2d3e86214f498b78b7d125)
>
> Signed-off-by: Po-Hsu Lin <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---

While the SHA1 mentioned in the commit message would be 4.5 and for that be
after Trusty, the cc for stable refers to a commit from 3.8 (possibly backported
up to 3.5). So it may or may not have the same impact. But regardless the change
looks sensible and for that I would say it should go into both Trusty and Yakkety.

-Stefan

>  security/selinux/hooks.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 357762a..d2246a8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5548,7 +5548,7 @@ static int selinux_setprocattr(struct task_struct *p,
>   return error;
>  
>   /* Obtain a SID for the context, if one was specified. */
> - if (size && str[1] && str[1] != '\n') {
> + if (size && str[0] && str[0] != '\n') {
>   if (str[size-1] == '\n') {
>   str[size-1] = 0;
>   size--;
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK/cmnt: [CVE-2017-2618][PATCH T/Y] selinux: fix off-by-one in setprocattr

Thadeu Lima de Souza Cascardo-3
On Tue, Jul 11, 2017 at 05:10:28PM +0200, Stefan Bader wrote:

> On 30.06.2017 09:21, Po-Hsu Lin wrote:
> > From: Stephen Smalley <[hidden email]>
> >
> > CVE-2017-2618
> >
> > SELinux tries to support setting/clearing of /proc/pid/attr attributes
> > from the shell by ignoring terminating newlines and treating an
> > attribute value that begins with a NUL or newline as an attempt to
> > clear the attribute.  However, the test for clearing attributes has
> > always been wrong; it has an off-by-one error, and this could further
> > lead to reading past the end of the allocated buffer since commit
> > bb646cdb12e75d82258c2f2e7746d5952d3e321a ("proc_pid_attr_write():
> > switch to memdup_user()").  Fix the off-by-one error.
> >
> > Even with this fix, setting and clearing /proc/pid/attr attributes
> > from the shell is not straightforward since the interface does not
> > support multiple write() calls (so shells that write the value and
> > newline separately will set and then immediately clear the attribute,
> > requiring use of echo -n to set the attribute), whereas trying to use
> > echo -n "" to clear the attribute causes the shell to skip the
> > write() call altogether since POSIX says that a zero-length write
> > causes no side effects. Thus, one must use echo -n to set and echo
> > without -n to clear, as in the following example:
> > $ echo -n unconfined_u:object_r:user_home_t:s0 > /proc/$$/attr/fscreate
> > $ cat /proc/$$/attr/fscreate
> > unconfined_u:object_r:user_home_t:s0
> > $ echo "" > /proc/$$/attr/fscreate
> > $ cat /proc/$$/attr/fscreate
> >
> > Note the use of /proc/$$ rather than /proc/self, as otherwise
> > the cat command will read its own attribute value, not that of the shell.
> >
> > There are no users of this facility to my knowledge; possibly we
> > should just get rid of it.
> >
> > UPDATE: Upon further investigation it appears that a local process
> > with the process:setfscreate permission can cause a kernel panic as a
> > result of this bug.  This patch fixes CVE-2017-2618.
> >
> > Signed-off-by: Stephen Smalley <[hidden email]>
> > [PM: added the update about CVE-2017-2618 to the commit description]
> > Cc: [hidden email] # 3.5: d6ea83ec6864e
> > Signed-off-by: Paul Moore <[hidden email]>
> >
> > Signed-off-by: James Morris <[hidden email]>
> > (cherry picked from commit 0c461cb727d146c9ef2d3e86214f498b78b7d125)
> >
> > Signed-off-by: Po-Hsu Lin <[hidden email]>
>
> Acked-by: Stefan Bader <[hidden email]>
>
> > ---
>
> While the SHA1 mentioned in the commit message would be 4.5 and for that be
> after Trusty, the cc for stable refers to a commit from 3.8 (possibly backported
> up to 3.5). So it may or may not have the same impact. But regardless the change
> looks sensible and for that I would say it should go into both Trusty and Yakkety.
>
> -Stefan

Acked-by: Thadeu Lima de Souza Cascardo <[hidden email]>

Yeah, that is d6ea83ec6864e9297fa8b00ec3dae183413a90e3 ("SELinux: audit
failed attempts to set invalid labels"), which does str[size-1], where
size could have been reduced from 1 to 0 in case it's a single '\n'
byte.

So, definitively needed on trusty.

Cascardo.

>
> >  security/selinux/hooks.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 357762a..d2246a8 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -5548,7 +5548,7 @@ static int selinux_setprocattr(struct task_struct *p,
> >   return error;
> >  
> >   /* Obtain a SID for the context, if one was specified. */
> > - if (size && str[1] && str[1] != '\n') {
> > + if (size && str[0] && str[0] != '\n') {
> >   if (str[size-1] == '\n') {
> >   str[size-1] = 0;
> >   size--;
> >
>
>




> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

APPLIED: [CVE-2017-2618][T/Y SRU] selinux: fix off-by-one in setprocattr

Thadeu Lima de Souza Cascardo-3
In reply to this post by Po-Hsu Lin (Sam)
Applied to trusty and yakkety master-next branches.

Thanks.
Cascardo.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Loading...