| Message ID | 1498807277-6635-2-git-send-email-po-hsu.lin@canonical.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Fri, Jun 30, 2017 at 3:21 PM, Po-Hsu Lin <po-hsu.lin@canonical.com> wrote: > From: Stephen Smalley <sds@tycho.nsa.gov> > > 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?
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 <kai.heng.feng@canonical.com> wrote: > Hi, > > On Fri, Jun 30, 2017 at 3:21 PM, Po-Hsu Lin <po-hsu.lin@canonical.com> wrote: >> From: Stephen Smalley <sds@tycho.nsa.gov> >> >> 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?
On 30.06.2017 09:21, Po-Hsu Lin wrote: > From: Stephen Smalley <sds@tycho.nsa.gov> > > 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 <sds@tycho.nsa.gov> > [PM: added the update about CVE-2017-2618 to the commit description] > Cc: stable@vger.kernel.org # 3.5: d6ea83ec6864e > Signed-off-by: Paul Moore <paul@paul-moore.com> > > Signed-off-by: James Morris <james.l.morris@oracle.com> > (cherry picked from commit 0c461cb727d146c9ef2d3e86214f498b78b7d125) > > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- 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--; >
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 <sds@tycho.nsa.gov> > > > > 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 <sds@tycho.nsa.gov> > > [PM: added the update about CVE-2017-2618 to the commit description] > > Cc: stable@vger.kernel.org # 3.5: d6ea83ec6864e > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > Signed-off-by: James Morris <james.l.morris@oracle.com> > > (cherry picked from commit 0c461cb727d146c9ef2d3e86214f498b78b7d125) > > > > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > > --- > > 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 <cascardo@canonical.com> 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 > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
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--;