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

[Thadeu Lima de Souza Cascardo]

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 at 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 at tycho.nsa.gov>
> > [PM: added the update about CVE-2017-2618 to the commit description]
> > Cc: stable at vger.kernel.org # 3.5: d6ea83ec6864e
> > Signed-off-by: Paul Moore <paul at paul-moore.com>
> > 
> > Signed-off-by: James Morris <james.l.morris at oracle.com>
> > (cherry picked from commit 0c461cb727d146c9ef2d3e86214f498b78b7d125)
> > 
> > Signed-off-by: Po-Hsu Lin <po-hsu.lin at canonical.com>
> 
> Acked-by: Stefan Bader <stefan.bader at 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 at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team