init script x11-common creates directories in insecure manners

Related Vulnerabilities: CVE-2012-1093  

Debian Bug report logs - #661627
init script x11-common creates directories in insecure manners

version graph

Reported by: vladz <vladz@devzero.fr>

Date: Tue, 28 Feb 2012 17:33:01 UTC

Severity: normal

Tags: security

Found in version xorg/1:7.5+8

Fixed in version xorg/1:7.6+12

Done: Julien Cristau <jcristau@debian.org>

Bug is archived. No further changes may be made.

Toggle useless messages

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Tue, 28 Feb 2012 17:33:04 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
New Bug report received and forwarded. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Tue, 28 Feb 2012 17:33:04 GMT) (full text, mbox, link).


Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: submit@bugs.debian.org
Subject: init script x11-common creates directories in insecure manners
Date: Tue, 28 Feb 2012 18:31:02 +0100
Package: x11-common
Version: 1:7.5+8
Tags: security


The init script "x11-common" creates directories "/tmp/.X11-unix" and
"/tmp/.ICE-unix" in insecure manners.

  $ cat -n /etc/init.d/x11-common
    [...]
    33    if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
    34      mv $SOCKET_DIR $SOCKET_DIR.$$
    35    fi
    36    mkdir -p $SOCKET_DIR
    37    chown root:root $SOCKET_DIR
    38    chmod 1777 $SOCKET_DIR
    [...]
    47    if [ -e $ICE_DIR ] && [ ! -d $ICE_DIR ]; then
    48      mv $ICE_DIR $ICE_DIR.$$
    49    fi
    50    mkdir -p $ICE_DIR
    51    chown root:root $ICE_DIR
    52    chmod 1777 $ICE_DIR

If a local user is able to place a symlink before the service starts
(for example before the package installation process), he could gain
root privileges.

For example, the symlink would point to an arbitrary directory (/etc),
so it won't match the conditions (lines 33 and 47) and the arbitrary
directory will get its permissions changed (lines 38 and 52).

As a solution, I would suggest to take care of the "mkdir" return codes 
(line 36 and 50).  To do not change permissions on failures.         

Thanks.
--
http://vladz.devzero.fr
PGP key 8F7E2D3C from pgp.mit.edu





Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Tue, 28 Feb 2012 17:45:11 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Tue, 28 Feb 2012 17:45:11 GMT) (full text, mbox, link).


Message #10 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>, 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Tue, 28 Feb 2012 18:42:59 +0100
On Tue, Feb 28, 2012 at 18:31:02 +0100, vladz wrote:

> Package: x11-common
> Version: 1:7.5+8
> Tags: security
> 
> 
> The init script "x11-common" creates directories "/tmp/.X11-unix" and
> "/tmp/.ICE-unix" in insecure manners.
> 
>   $ cat -n /etc/init.d/x11-common
>     [...]
>     33    if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
>     34      mv $SOCKET_DIR $SOCKET_DIR.$$
>     35    fi
>     36    mkdir -p $SOCKET_DIR
>     37    chown root:root $SOCKET_DIR
>     38    chmod 1777 $SOCKET_DIR
>     [...]
>     47    if [ -e $ICE_DIR ] && [ ! -d $ICE_DIR ]; then
>     48      mv $ICE_DIR $ICE_DIR.$$
>     49    fi
>     50    mkdir -p $ICE_DIR
>     51    chown root:root $ICE_DIR
>     52    chmod 1777 $ICE_DIR
> 
> If a local user is able to place a symlink before the service starts
> (for example before the package installation process), he could gain
> root privileges.
> 
> For example, the symlink would point to an arbitrary directory (/etc),
> so it won't match the conditions (lines 33 and 47) and the arbitrary
> directory will get its permissions changed (lines 38 and 52).
> 
> As a solution, I would suggest to take care of the "mkdir" return codes 
> (line 36 and 50).  To do not change permissions on failures.         
> 
This script is set -e AFAICT, which means it already does care about the
mkdir return code.

Cheers,
Julien




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Tue, 28 Feb 2012 18:09:07 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Tue, 28 Feb 2012 18:09:07 GMT) (full text, mbox, link).


Message #15 received at 661627@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: Julien Cristau <jcristau@debian.org>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Tue, 28 Feb 2012 19:05:23 +0100
On Tue, Feb 28, 2012 at 06:42:59PM +0100, Julien Cristau wrote:
> > As a solution, I would suggest to take care of the "mkdir" return codes 
> > (line 36 and 50).  To do not change permissions on failures.         
> > 
> This script is set -e AFAICT, which means it already does care about the
> mkdir return code.

Yes but with the "-p" option, mkdir always return 0 (success):

  $ mkdir /tmp/dir
  $ mkdir /tmp/dir
  mkdir: cannot create directory `/tmp/dir': File exists
  $ echo $?
  1
  $ mkdir -p /tmp/dir
  $ echo $?
  0

I have a working PoC if needed.

Regards,
--
http://vladz.devzero.fr
PGP key 8F7E2D3C from pgp.mit.edu





Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Tue, 28 Feb 2012 18:18:03 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Tue, 28 Feb 2012 18:18:03 GMT) (full text, mbox, link).


Message #20 received at 661627@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: Julien Cristau <jcristau@debian.org>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Tue, 28 Feb 2012 19:13:59 +0100
On Tue, Feb 28, 2012 at 06:42:59PM +0100, Julien Cristau wrote:
> > As a solution, I would suggest to take care of the "mkdir" return codes 
> > (line 36 and 50).  To do not change permissions on failures.         

And as a solution, I suggested to check the return code of "mkdir" (ran
without -p).




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Tue, 28 Feb 2012 19:24:09 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Tue, 28 Feb 2012 19:24:09 GMT) (full text, mbox, link).


Message #25 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Tue, 28 Feb 2012 20:21:39 +0100
[Message part 1 (text/plain, inline)]
On Tue, Feb 28, 2012 at 19:05:23 +0100, vladz wrote:

> On Tue, Feb 28, 2012 at 06:42:59PM +0100, Julien Cristau wrote:
> > > As a solution, I would suggest to take care of the "mkdir" return codes 
> > > (line 36 and 50).  To do not change permissions on failures.         
> > > 
> > This script is set -e AFAICT, which means it already does care about the
> > mkdir return code.
> 
> Yes but with the "-p" option, mkdir always return 0 (success):
> 
>   $ mkdir /tmp/dir
>   $ mkdir /tmp/dir
>   mkdir: cannot create directory `/tmp/dir': File exists
>   $ echo $?
>   1
>   $ mkdir -p /tmp/dir
>   $ echo $?
>   0
> 
Right, makes sense.  I can drop the -p, I guess.  Not sure what impact
that would have on things assuming they can use /tmp/.X11-unix (I
wouldn't really like to fix this just to have the same issue elsewhere).
Looking at trans_mkdir
(http://cgit.freedesktop.org/xorg/lib/libxtrans/tree/Xtransutil.c#n480)
it *looks* like it should be safe, though.

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Wed, 29 Feb 2012 11:06:32 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Wed, 29 Feb 2012 11:06:43 GMT) (full text, mbox, link).


Message #30 received at 661627@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: Julien Cristau <jcristau@debian.org>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Wed, 29 Feb 2012 12:03:30 +0100
CVE-2012-1093 has been assigned for this issue.

On Tue, Feb 28, 2012 at 08:21:39PM +0100, Julien Cristau wrote:
> Right, makes sense.  I can drop the -p, I guess.  Not sure what impact
> that would have on things assuming they can use /tmp/.X11-unix (I
> wouldn't really like to fix this just to have the same issue elsewhere).

Removing "-p" sounds good to me.

Thank you,
Regards,
--
http://vladz.devzero.fr
PGP key 8F7E2D3C from pgp.mit.edu




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Wed, 29 Feb 2012 20:33:03 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Wed, 29 Feb 2012 20:33:03 GMT) (full text, mbox, link).


Message #35 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Wed, 29 Feb 2012 21:29:37 +0100
[Message part 1 (text/plain, inline)]
On Tue, Feb 28, 2012 at 20:21:39 +0100, Julien Cristau wrote:

> On Tue, Feb 28, 2012 at 19:05:23 +0100, vladz wrote:
> 
> > On Tue, Feb 28, 2012 at 06:42:59PM +0100, Julien Cristau wrote:
> > > > As a solution, I would suggest to take care of the "mkdir" return codes 
> > > > (line 36 and 50).  To do not change permissions on failures.         
> > > > 
> > > This script is set -e AFAICT, which means it already does care about the
> > > mkdir return code.
> > 
> > Yes but with the "-p" option, mkdir always return 0 (success):
> > 
> >   $ mkdir /tmp/dir
> >   $ mkdir /tmp/dir
> >   mkdir: cannot create directory `/tmp/dir': File exists
> >   $ echo $?
> >   1
> >   $ mkdir -p /tmp/dir
> >   $ echo $?
> >   0
> > 
> Right, makes sense.  I can drop the -p, I guess.  Not sure what impact
> that would have on things assuming they can use /tmp/.X11-unix (I
> wouldn't really like to fix this just to have the same issue elsewhere).
> Looking at trans_mkdir
> (http://cgit.freedesktop.org/xorg/lib/libxtrans/tree/Xtransutil.c#n480)
> it *looks* like it should be safe, though.
> 
Actually it's not going to work.  If /tmp/.X11-unix exists and is a
directory (not a symlink), that's good enough for us, we don't want to
fail in that case.

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Wed, 29 Feb 2012 22:00:05 GMT) (full text, mbox, link).


Acknowledgement sent to Tim <tim-debian@sentinelchicken.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Wed, 29 Feb 2012 22:00:05 GMT) (full text, mbox, link).


Message #40 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Tim <tim-debian@sentinelchicken.org>
To: 661627@bugs.debian.org
Subject: Avoid /tmp ?
Date: Wed, 29 Feb 2012 13:51:18 -0800
This appears to be a pretty serious problem.  I agree, just dropping
'-p' won't work for functional reasons.

As a better long-term solution, have you considered just moving those
directories out of /tmp?  There's almost always a safer place to put
temporary files/directories.  For instance, under /var/lib or
/var/run, or whatever is most appropriate as an application-specific
directory, whose parent isn't world-writable.

tim




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Wed, 29 Feb 2012 22:15:05 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Wed, 29 Feb 2012 22:15:05 GMT) (full text, mbox, link).


Message #45 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Wed, 29 Feb 2012 23:10:28 +0100
[Message part 1 (text/plain, inline)]
On Wed, Feb 29, 2012 at 21:29:37 +0100, Julien Cristau wrote:

> On Tue, Feb 28, 2012 at 20:21:39 +0100, Julien Cristau wrote:
> 
> > On Tue, Feb 28, 2012 at 19:05:23 +0100, vladz wrote:
> > 
> > > On Tue, Feb 28, 2012 at 06:42:59PM +0100, Julien Cristau wrote:
> > > > > As a solution, I would suggest to take care of the "mkdir" return codes 
> > > > > (line 36 and 50).  To do not change permissions on failures.         
> > > > > 
> > > > This script is set -e AFAICT, which means it already does care about the
> > > > mkdir return code.
> > > 
> > > Yes but with the "-p" option, mkdir always return 0 (success):
> > > 
> > >   $ mkdir /tmp/dir
> > >   $ mkdir /tmp/dir
> > >   mkdir: cannot create directory `/tmp/dir': File exists
> > >   $ echo $?
> > >   1
> > >   $ mkdir -p /tmp/dir
> > >   $ echo $?
> > >   0
> > > 
> > Right, makes sense.  I can drop the -p, I guess.  Not sure what impact
> > that would have on things assuming they can use /tmp/.X11-unix (I
> > wouldn't really like to fix this just to have the same issue elsewhere).
> > Looking at trans_mkdir
> > (http://cgit.freedesktop.org/xorg/lib/libxtrans/tree/Xtransutil.c#n480)
> > it *looks* like it should be safe, though.
> > 
> Actually it's not going to work.  If /tmp/.X11-unix exists and is a
> directory (not a symlink), that's good enough for us, we don't want to
> fail in that case.
> 
And while I'm at it I'd also like to fix the $SOCKET_DIR.$$ thing
to use a random name instead (probably with mktemp).

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Wed, 29 Feb 2012 22:36:03 GMT) (full text, mbox, link).


Acknowledgement sent to "Bernhard R. Link" <brlink@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Wed, 29 Feb 2012 22:36:03 GMT) (full text, mbox, link).


Message #50 received at 661627@bugs.debian.org (full text, mbox, reply):

From: "Bernhard R. Link" <brlink@debian.org>
To: Tim <tim-debian@sentinelchicken.org>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Wed, 29 Feb 2012 23:33:49 +0100
* Tim <tim-debian@sentinelchicken.org> [120229 23:00]:
> As a better long-term solution, have you considered just moving those
> directories out of /tmp?

Those are for sockets whose name is part of the interface to access
them. So you cannot move them. And the directory itself needs to be
world-writeable, so it is best placed within /tmp.

        Bernhard R. Link




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Wed, 29 Feb 2012 22:42:03 GMT) (full text, mbox, link).


Acknowledgement sent to Tim <tim-debian@sentinelchicken.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Wed, 29 Feb 2012 22:42:03 GMT) (full text, mbox, link).


Message #55 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Tim <tim-debian@sentinelchicken.org>
To: "Bernhard R. Link" <brlink@debian.org>
Cc: 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Wed, 29 Feb 2012 14:39:19 -0800
Hi Bernhard,

> > As a better long-term solution, have you considered just moving those
> > directories out of /tmp?
> 
> Those are for sockets whose name is part of the interface to access
> them. So you cannot move them. And the directory itself needs to be
> world-writeable, so it is best placed within /tmp.


Hmm, sounds like a badly-designed interface.  Where is this interface
defined?  

I don't doubt you, I'm merely naive about X design/interface, and
curious as to how flexible this is.  Is this use of /tmp a Debian
thing, an Xorg thing, or an X11R6 thing?

thanks,
tim




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Thu, 01 Mar 2012 19:57:07 GMT) (full text, mbox, link).


Acknowledgement sent to Tim <tim-debian@sentinelchicken.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Thu, 01 Mar 2012 19:57:07 GMT) (full text, mbox, link).


Message #60 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Tim <tim-debian@sentinelchicken.org>
To: 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Thu, 1 Mar 2012 11:55:29 -0800
As far as the short-term solution to this problem goes, how about
this (untested)?


if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
    mv $SOCKET_DIR $SOCKET_DIR.$$ || exit $?
fi
if [ ! -e $SOCKET_DIR ]; then
    mkdir $SOCKET_DIR || exit $?
    chown root:root $SOCKET_DIR
    chmod 1777 $SOCKET_DIR
fi


First move other types of files out of the way, as before (is this
even necessary?).  After that, we should have either no SOCKET_DIR or
a directory by that name we have created previously.  If it doesn't
exist as a directory, create it.

If something by that name suddenly appears in the race after our
second existence test, then fail, since someone is clearly doing some
hanky-panky. Otherwise, we should own the file and there shouldn't be
a risk.  I realize that the "|| exit $?" items are redundant given the
script's "set -e", but I like to see things explicit when security
matters, since some future maintainer might accidentally remove the
"set -e" for seemingly unrelated reasons.

Note that the "chown root:root $SOCKET_DIR" also seems redundant to me
(if we didn't already own it, we would have bigger problems, right?).

tim




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Thu, 01 Mar 2012 20:18:03 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Thu, 01 Mar 2012 20:18:03 GMT) (full text, mbox, link).


Message #65 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: Tim <tim-debian@sentinelchicken.org>, 661627@bugs.debian.org
Cc: vladz <vladz@devzero.fr>
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Thu, 1 Mar 2012 21:16:06 +0100
[Message part 1 (text/plain, inline)]
On Thu, Mar  1, 2012 at 11:55:29 -0800, Tim wrote:

> As far as the short-term solution to this problem goes, how about
> this (untested)?
> 
> 
> if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
>     mv $SOCKET_DIR $SOCKET_DIR.$$ || exit $?
> fi
> if [ ! -e $SOCKET_DIR ]; then
>     mkdir $SOCKET_DIR || exit $?
>     chown root:root $SOCKET_DIR
>     chmod 1777 $SOCKET_DIR
> fi
> 
So far I have this:

diff --git a/debian/x11-common.init b/debian/x11-common.init
index 34835ac..71f9fd5 100644
--- a/debian/x11-common.init
+++ b/debian/x11-common.init
@@ -30,10 +30,12 @@ set_up_socket_dir () {
   if [ "$VERBOSE" != no ]; then
     log_begin_msg "Setting up X server socket directory $SOCKET_DIR..."
   fi
-  if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
-    mv $SOCKET_DIR $SOCKET_DIR.$$
+  # if $SOCKET_DIR exists and isn't a directory, move it aside
+  if [ -e $SOCKET_DIR ] && ! [ -d $SOCKET_DIR ] || [ -h $SOCKET_DIR ]; then
+    mv $SOCKET_DIR "$(mktemp -d $SOCKET_DIR.XXXXXX)"
   fi
-  mkdir -p $SOCKET_DIR
+  # make sure $SOCKET_DIR exists and isn't a symlink
+  mkdir $SOCKET_DIR 2>/dev/null || [ -d $SOCKET_DIR ] && ! [ -h $SOCKET_DIR ]
   chown root:root $SOCKET_DIR
   chmod 1777 $SOCKET_DIR
   do_restorecon $SOCKET_DIR
@@ -44,10 +46,10 @@ set_up_ice_dir () {
   if [ "$VERBOSE" != no ]; then
     log_begin_msg "Setting up ICE socket directory $ICE_DIR..."
   fi
-  if [ -e $ICE_DIR ] && [ ! -d $ICE_DIR ]; then
-    mv $ICE_DIR $ICE_DIR.$$
+  if [ -e $ICE_DIR ] && [ ! -d $ICE_DIR ] || [ -h $ICE_DIR ]; then
+    mv $ICE_DIR "$(mktemp -d $ICE_DIR.XXXXXX)"
   fi
-  mkdir -p $ICE_DIR
+  mkdir $ICE_DIR 2>/dev/null || [ -d $ICE_DIR ] && ! [ -h $ICE_DIR ]
   chown root:root $ICE_DIR
   chmod 1777 $ICE_DIR
   do_restorecon $ICE_DIR

Compared to your version it allows moving aside a broken symlink, but
other than that (and the mktemp use) they should be equivalent, I think.
Another pair of eyes would be appreciated though...

> First move other types of files out of the way, as before (is this
> even necessary?).  After that, we should have either no SOCKET_DIR or
> a directory by that name we have created previously.  If it doesn't
> exist as a directory, create it.
> 
> If something by that name suddenly appears in the race after our
> second existence test, then fail, since someone is clearly doing some
> hanky-panky. Otherwise, we should own the file and there shouldn't be
> a risk.  I realize that the "|| exit $?" items are redundant given the
> script's "set -e", but I like to see things explicit when security
> matters, since some future maintainer might accidentally remove the
> "set -e" for seemingly unrelated reasons.
> 
> Note that the "chown root:root $SOCKET_DIR" also seems redundant to me
> (if we didn't already own it, we would have bigger problems, right?).
> 
I guess it protects against some user doing mkdir /tmp/.X11-unix before
this runs (which probably means before the package is installed, so it's
not like this is a very likely race) and then owning the directory.

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Thu, 01 Mar 2012 20:42:03 GMT) (full text, mbox, link).


Acknowledgement sent to Tim <tim-debian@sentinelchicken.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Thu, 01 Mar 2012 20:42:03 GMT) (full text, mbox, link).


Message #70 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Tim <tim-debian@sentinelchicken.org>
To: Julien Cristau <jcristau@debian.org>
Cc: 661627@bugs.debian.org, vladz <vladz@devzero.fr>
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Thu, 1 Mar 2012 12:39:41 -0800
Hi Julien,



> > As far as the short-term solution to this problem goes, how about
> > this (untested)?
> > 
> > 
> > if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
> >     mv $SOCKET_DIR $SOCKET_DIR.$$ || exit $?
> > fi
> > if [ ! -e $SOCKET_DIR ]; then
> >     mkdir $SOCKET_DIR || exit $?
> >     chown root:root $SOCKET_DIR
> >     chmod 1777 $SOCKET_DIR
> > fi
> > 
> So far I have this:
> 
> diff --git a/debian/x11-common.init b/debian/x11-common.init
> index 34835ac..71f9fd5 100644
> --- a/debian/x11-common.init
> +++ b/debian/x11-common.init
> @@ -30,10 +30,12 @@ set_up_socket_dir () {
>    if [ "$VERBOSE" != no ]; then
>      log_begin_msg "Setting up X server socket directory $SOCKET_DIR..."
>    fi
> -  if [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; then
> -    mv $SOCKET_DIR $SOCKET_DIR.$$
> +  # if $SOCKET_DIR exists and isn't a directory, move it aside
> +  if [ -e $SOCKET_DIR ] && ! [ -d $SOCKET_DIR ] || [ -h $SOCKET_DIR ]; then
> +    mv $SOCKET_DIR "$(mktemp -d $SOCKET_DIR.XXXXXX)"
>    fi
> -  mkdir -p $SOCKET_DIR
> +  # make sure $SOCKET_DIR exists and isn't a symlink
> +  mkdir $SOCKET_DIR 2>/dev/null || [ -d $SOCKET_DIR ] && ! [ -h $SOCKET_DIR ]
>    chown root:root $SOCKET_DIR
>    chmod 1777 $SOCKET_DIR
>    do_restorecon $SOCKET_DIR
> @@ -44,10 +46,10 @@ set_up_ice_dir () {
>    if [ "$VERBOSE" != no ]; then
>      log_begin_msg "Setting up ICE socket directory $ICE_DIR..."
>    fi
> -  if [ -e $ICE_DIR ] && [ ! -d $ICE_DIR ]; then
> -    mv $ICE_DIR $ICE_DIR.$$
> +  if [ -e $ICE_DIR ] && [ ! -d $ICE_DIR ] || [ -h $ICE_DIR ]; then
> +    mv $ICE_DIR "$(mktemp -d $ICE_DIR.XXXXXX)"
>    fi
> -  mkdir -p $ICE_DIR
> +  mkdir $ICE_DIR 2>/dev/null || [ -d $ICE_DIR ] && ! [ -h $ICE_DIR ]
>    chown root:root $ICE_DIR
>    chmod 1777 $ICE_DIR
>    do_restorecon $ICE_DIR
> 
> Compared to your version it allows moving aside a broken symlink, but
> other than that (and the mktemp use) they should be equivalent, I think.
> Another pair of eyes would be appreciated though...

I think there is still a race in your version in the lines which look
like:

> +  mkdir $ICE_DIR 2>/dev/null || [ -d $ICE_DIR ] && ! [ -h $ICE_DIR ]

mkdir will fail if the file already exists for any reason.  After
mkdir fails, it is possible that another process will be able to run
and remove/create new versions of the path with different properties
after your tests run.

Here's a specific attack that could work, I think:

1. create a plain file named $ICE_DIR
2. monitor the file and wait for it to be moved to the new mktemp name
3. create a directory with the $ICE_DIR name before mkdir runs
4. mkdir will fail
5. After the -h test finishes, but before the chown is run, remove the
directory and replace it with a symlink


Yes, this is complicated and would require lots of luck given the
context of the script, but it would certainly be best to avoid any
dangerous races.

The reason that my version of the script avoids this issue is that it
relies on the atomicity of mkdir.  If that fails, simply bail out.
Don't do any additional tests afterward, since that introduces a race.
I can't guarantee my version doesn't have other races though, so
please do pick over it carefully...


> > Note that the "chown root:root $SOCKET_DIR" also seems redundant to me
> > (if we didn't already own it, we would have bigger problems, right?).
> > 
> I guess it protects against some user doing mkdir /tmp/.X11-unix before
> this runs (which probably means before the package is installed, so it's
> not like this is a very likely race) and then owning the directory.

Oh, right, duh.  Well, the dir is created every time the box boots,
since /tmp is cleared, so it is needed for sure.


thanks,
tim




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Thu, 01 Mar 2012 20:51:11 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Thu, 01 Mar 2012 20:51:11 GMT) (full text, mbox, link).


Message #75 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: Tim <tim-debian@sentinelchicken.org>, 661627@bugs.debian.org
Cc: vladz <vladz@devzero.fr>
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Thu, 1 Mar 2012 21:48:47 +0100
[Message part 1 (text/plain, inline)]
On Thu, Mar  1, 2012 at 12:39:41 -0800, Tim wrote:

> I think there is still a race in your version in the lines which look
> like:
> 
> > +  mkdir $ICE_DIR 2>/dev/null || [ -d $ICE_DIR ] && ! [ -h $ICE_DIR ]
> 
> mkdir will fail if the file already exists for any reason.  After
> mkdir fails, it is possible that another process will be able to run
> and remove/create new versions of the path with different properties
> after your tests run.
> 
doh.  You're right, of course.

[...]
> > > Note that the "chown root:root $SOCKET_DIR" also seems redundant to me
> > > (if we didn't already own it, we would have bigger problems, right?).
> > > 
> > I guess it protects against some user doing mkdir /tmp/.X11-unix before
> > this runs (which probably means before the package is installed, so it's
> > not like this is a very likely race) and then owning the directory.
> 
> Oh, right, duh.  Well, the dir is created every time the box boots,
> since /tmp is cleared, so it is needed for sure.
> 
/etc/init.d/x11-common on boot should run before any unprivileged user
has a chance to do anything (it's in rcS.d, and depends only on
$local_fs), so it's less of a problem than initial package installation
AFAICT.

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Thu, 01 Mar 2012 21:15:05 GMT) (full text, mbox, link).


Acknowledgement sent to Tim <tim-debian@sentinelchicken.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Thu, 01 Mar 2012 21:17:18 GMT) (full text, mbox, link).


Message #80 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Tim <tim-debian@sentinelchicken.org>
To: Julien Cristau <jcristau@debian.org>
Cc: 661627@bugs.debian.org, vladz <vladz@devzero.fr>
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Thu, 1 Mar 2012 13:08:02 -0800
> /etc/init.d/x11-common on boot should run before any unprivileged user
> has a chance to do anything (it's in rcS.d, and depends only on
> $local_fs), so it's less of a problem than initial package installation
> AFAICT.

I'm not that familiar with the newer dependency boot sequencing, but I
know someone who looked into this and seemed to think cron or atd
*could* run before it.  Looking at the scripts I have on my box, it
seems like you may be right, assuming nothing else has a similar set
of dependencies (namely, only $local_fs) which could hand off control
to a user non-root user.  However, my analysis doesn't cover every
init script in every possible package...

tim




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 11:48:40 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 11:48:45 GMT) (full text, mbox, link).


Message #85 received at 661627@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: Julien Cristau <jcristau@debian.org>
Cc: Tim <tim-debian@sentinelchicken.org>, 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 12:44:23 +0100
Julien, thank you for putting me back in CC. ;)

On Thu, Mar 01, 2012 at 09:48:47PM +0100, Julien Cristau wrote:
> On Thu, Mar  1, 2012 at 12:39:41 -0800, Tim wrote:
> > > > Note that the "chown root:root $SOCKET_DIR" also seems redundant to me
> > > > (if we didn't already own it, we would have bigger problems, right?).
> > > > 
> > > I guess it protects against some user doing mkdir /tmp/.X11-unix before
> > > this runs (which probably means before the package is installed, so it's
> > > not like this is a very likely race) and then owning the directory.
> > 
> > Oh, right, duh.  Well, the dir is created every time the box boots,
> > since /tmp is cleared, so it is needed for sure.

I think the obsolete chown command should be removed (as said Tim), and
also the chmod should by replaced by a single atomic operation (using 
"mkdir -m").  Those two things will avoid usages of dangerous commands
and then, reduce TOCTTOU risks.

So if chown is removed, another check has to be introduced.  It is to
check if the directory isn't owned by root.  Because a user could try to
own X sockets by creating the directory himself.

Well, here is how I would see the script (with a lot of comments to try
making things clear):

  # We move $SOCKET_DIR if:
  #   - it exists but is not a dir
  #   - is whatever but not owned by root
  #   - is a symlink
  if { [ -e $SOCKET_DIR ] && [ ! -d $SOCKET_DIR ]; } ||
     { [ -e $SOCKET_DIR ] && [ ! -O $SOCKET_DIR ]; } ||
       [ -h $SOCKET_DIR ]; then

    mv $SOCKET_DIR $SOCKET_DIR.$$
  fi


  # At this point, we could find:
  #  - Correct directory (ie. $SOCKET_DIR owned by root)
  #  - a symlink or whatever (raced by a malicious user)
  #
  # So before creating it, we need to check if:
  #  - does not exist or is not owned by root
  if [ ! -O $SOCKET_DIR ];
    # symlink, malicious files will give a failure here
    mkdir -m 1777 $SOCKET_DIR
  fi

What do you think about this?

To finish, I find the ways to create those two directories ($SOCKET_DIR
and $ICE_DIR) quite redundant.  A function called create_dir() could 
contain the code above and be launched from both set_up_socket_dir() and
set_up_ice_dir()?





Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 12:57:12 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 12:57:15 GMT) (full text, mbox, link).


Message #90 received at 661627@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: Tim <tim-debian@sentinelchicken.org>
Cc: Julien Cristau <jcristau@debian.org>, 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 13:27:19 +0100
[Message part 1 (text/plain, inline)]
Oops, I forgot "then" in my last post:

- if [ ! -O $SOCKET_DIR ];
+ if [ ! -O $SOCKET_DIR ]; then

I have attached a patch. Hope this helps.


[x11-common_avoid-racecond.patch (text/x-diff, attachment)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 13:30:48 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 13:30:53 GMT) (full text, mbox, link).


Message #95 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>
Cc: Tim <tim-debian@sentinelchicken.org>, 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 14:29:33 +0100
On Fri, Mar  2, 2012 at 12:44:23 +0100, vladz wrote:

> 
> Julien, thank you for putting me back in CC. ;)
> 
> On Thu, Mar 01, 2012 at 09:48:47PM +0100, Julien Cristau wrote:
> > On Thu, Mar  1, 2012 at 12:39:41 -0800, Tim wrote:
> > > > > Note that the "chown root:root $SOCKET_DIR" also seems redundant to me
> > > > > (if we didn't already own it, we would have bigger problems, right?).
> > > > > 
> > > > I guess it protects against some user doing mkdir /tmp/.X11-unix before
> > > > this runs (which probably means before the package is installed, so it's
> > > > not like this is a very likely race) and then owning the directory.
> > > 
> > > Oh, right, duh.  Well, the dir is created every time the box boots,
> > > since /tmp is cleared, so it is needed for sure.
> 
> I think the obsolete chown command should be removed (as said Tim), and
> also the chmod should by replaced by a single atomic operation (using 
> "mkdir -m").  Those two things will avoid usages of dangerous commands
> and then, reduce TOCTTOU risks.
> 
I'm not convinced the chown can be removed.  And 'mkdir -m 1777 foo' is
not any more atomic than 'mkdir foo && chmod 1777 foo'.  The problem is
that I'd still like to be able to chown and chmod /tmp/.X11-unix if it
already exists as a directory when the script runs.  I can do that in C
with something like this:

  ret = mkdir("/tmp/.X11-unix", 0700);
  if (ret == 0 || errno == EEXIST) {
    fd = open("/tmp/.X11-unix", O_RDONLY | O_NOFOLLOW);
    if (fd < 0)
      fail();
    fstat(fd, &st);
    if (!S_ISDIR(st.st_mode))
      fail();
    if (fchown(fd, 0, 0)) fail();
    if (fchmod(fd, 01777)) fail();
  } else {
    fail();
  }

but so far I haven't seen a way to do that in shell, because chmod(1)
doesn't have a --no-dereference option, and even if it did it doesn't
look like I could safely detect whether to exit with failure or success.

hmm, how about this:

mkdir -p /tmp/.X11-unix
chown -h root:root /tmp/.X11-unix
stat=$(LC_ALL=C stat -c '%u %g %F' /tmp/.X11-unix)
if [ "$stat" != '0 0 directory' ]; then
  exit 1
fi
chmod 1777 /tmp/.X11-unix

?

> To finish, I find the ways to create those two directories ($SOCKET_DIR
> and $ICE_DIR) quite redundant.  A function called create_dir() could 
> contain the code above and be launched from both set_up_socket_dir() and
> set_up_ice_dir()?
> 
Agreed.  Or drop those two functions and call set_up_dir "$SOCKET_DIR &&
set_up_dir $ICE_DIR" directly.

Thanks a lot to you both for the help so far...

Cheers,
Julien




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 15:42:03 GMT) (full text, mbox, link).


Acknowledgement sent to "Bernhard R. Link" <brlink@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 15:42:04 GMT) (full text, mbox, link).


Message #100 received at 661627@bugs.debian.org (full text, mbox, reply):

From: "Bernhard R. Link" <brlink@debian.org>
To: 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 16:40:30 +0100
* Bernhard R. Link <brlink@debian.org> [120302 15:56]:
> And I think if there is something else, it might make sense to
> try to move it away again and try to create it again.

How about the following:

# create a directory in /tmp.
# assumes /tmp has a sticky bit set (or is only writeable by root)
create_dir() {
  error=0
  while true ; do
    if [ $error -ne 0 ] ; then
           # an error means the file-system is readonly or an attacker
           # is doing evil things, distinguish by creating a temporary file.
           fn="$(mktemp /tmp/testwriteable.XXXXXXXXXX)" || return 1
           rm "$fn"
    fi
    mkdir -p -m 01777 "/tmp/$1" || { rm "/tmp/$1" || error=1 ; continue ; }
    case "$(LC_ALL=C stat -c '%u %g %a %F' "/tmp/$1")" in
      "0 0 1777 directory")
           # everything as it is supposed to be
           break
           ;;
      "0 0 "*" directory")
           # as it is owned by root, cannot be replaced with a symlink:
           chmod 01777 "/tmp/$1"
           break
           ;;
      *" directory")
           # if the chown succeeds, the next step can change it savely
           chown -h root:root "/tmp/$1" || error=1
           continue
           ;;
      *)
           # if it is not a directory, rm should be able to remove it
           # unless it vanished again or was replaced with a directory
           rm "/tmp/$1" || error=1
           continue
           ;;
    esac
  done
}




Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 18:34:06 GMT) (full text, mbox, link).


Message #103 received at 661627@bugs.debian.org (full text, mbox, reply):

From: "Bernhard R. Link" <brlink@debian.org>
To: debian-x@lists.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 15:39:35 +0100
* Julien Cristau <jcristau@debian.org> [120302 14:31]:
> > I think the obsolete chown command should be removed (as said Tim), and
> > also the chmod should by replaced by a single atomic operation (using 
> > "mkdir -m").  Those two things will avoid usages of dangerous commands
> > and then, reduce TOCTTOU risks.
> >
> I'm not convinced the chown can be removed.

> And 'mkdir -m 1777 foo' is not any more atomic than 'mkdir foo && chmod 1777 foo'.

To be more precise: it is more atomic but not in a way that makes a difference here.

> but so far I haven't seen a way to do that in shell, because chmod(1)
> doesn't have a --no-dereference option, and even if it did it doesn't
> look like I could safely detect whether to exit with failure or success.
>
> hmm, how about this:
>
> mkdir -p /tmp/.X11-unix
> chown -h root:root /tmp/.X11-unix
> stat=$(LC_ALL=C stat -c '%u %g %F' /tmp/.X11-unix)
> if [ "$stat" != '0 0 directory' ]; then
>   exit 1
> fi
> chmod 1777 /tmp/.X11-unix

If mkdir -p fails (it for example does if .X11-unix is a proper file
or a dangling symlink), one might refrain from issuing the following
commands.

Are there any kernel hardening patches that check ownership of symlinks?
If there are then changing the ownership of a /tmp/.X11-unix symlink
might actually introduce a problem similar to the one this was
originally trying to solve in those cases.

And I think if there is something else, it might make sense to
try to move it away again and try to create it again.

        Bernhard R. Link


-- 
To UNSUBSCRIBE, email to debian-x-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: http://lists.debian.org/20120302143935.GA2106@client.brlink.eu





Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 19:21:03 GMT) (full text, mbox, link).


Acknowledgement sent to vladz <vladz@devzero.fr>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 19:21:03 GMT) (full text, mbox, link).


Message #108 received at 661627@bugs.debian.org (full text, mbox, reply):

From: vladz <vladz@devzero.fr>
To: Julien Cristau <jcristau@debian.org>
Cc: Tim <tim-debian@sentinelchicken.org>, 661627@bugs.debian.org
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 20:20:11 +0100
On Fri, Mar 02, 2012 at 02:29:33PM +0100, Julien Cristau wrote:
> I'm not convinced the chown can be removed.  And 'mkdir -m 1777 foo' is
> not any more atomic than 'mkdir foo && chmod 1777 foo'.  

The command "mkdir -m" calls the mkdir() syscall, and its second
argument seems to be the mode.  

  $ man 2 mkdir
  [...]
  int mkdir(const char *pathname, mode_t mode);

Maybe I'm wrong, but this is what I call an atomic way to create and set
permissions (ie. two operations in a unique syscall).  For example:

  $ strace mkdir -m 222 /tmp/foo
  [...]
  mkdir("/tmp/foo", 0222)                 = 0

> The problem is
> that I'd still like to be able to chown and chmod /tmp/.X11-unix if it
> already exists as a directory when the script runs.  I can do that in C
> with something like this:
> 
>   ret = mkdir("/tmp/.X11-unix", 0700);
>   if (ret == 0 || errno == EEXIST) {
>     fd = open("/tmp/.X11-unix", O_RDONLY | O_NOFOLLOW);
>     if (fd < 0)
>       fail();
>     fstat(fd, &st);
>     if (!S_ISDIR(st.st_mode))
>       fail();
>     if (fchown(fd, 0, 0)) fail();
>     if (fchmod(fd, 01777)) fail();

Yes, fchown & fchmod use file descriptors, those functions are safer
than commands chmod & chown that directly use filenames...  This is why
I consider those commands dangerous.

> hmm, how about this:
>
>mkdir -p /tmp/.X11-unix
>chown -h root:root /tmp/.X11-unix
>stat=$(LC_ALL=C stat -c '%u %g %F' /tmp/.X11-unix)
>if [ "$stat" != '0 0 directory' ]; then
>  exit 1
>fi
>chmod 1777 /tmp/.X11-unix

This would work (even if it uses chmod), but wasn't the Bash approach (test
with "-O", "-G" and "-d") simpler than using "stat"?

> Agreed.  Or drop those two functions and call set_up_dir "$SOCKET_DIR &&
> set_up_dir $ICE_DIR" directly.

Agreed. :)





Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 19:33:06 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 19:33:06 GMT) (full text, mbox, link).


Message #113 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>, 661627@bugs.debian.org
Cc: Tim <tim-debian@sentinelchicken.org>
Subject: Re: Bug#661627: Avoid /tmp ?
Date: Fri, 2 Mar 2012 20:30:24 +0100
[Message part 1 (text/plain, inline)]
On Fri, Mar  2, 2012 at 20:20:11 +0100, vladz wrote:

> On Fri, Mar 02, 2012 at 02:29:33PM +0100, Julien Cristau wrote:
> > I'm not convinced the chown can be removed.  And 'mkdir -m 1777 foo' is
> > not any more atomic than 'mkdir foo && chmod 1777 foo'.  
> 
> The command "mkdir -m" calls the mkdir() syscall, and its second
> argument seems to be the mode.  
> 
>   $ man 2 mkdir
>   [...]
>   int mkdir(const char *pathname, mode_t mode);
> 
> Maybe I'm wrong, but this is what I call an atomic way to create and set
> permissions (ie. two operations in a unique syscall).  For example:
> 
>   $ strace mkdir -m 222 /tmp/foo
>   [...]
>   mkdir("/tmp/foo", 0222)                 = 0
> 
the second argument to mkdir obeys umask, so you end up doing

mkdir()
open()
fstat()
fchmod()

as far as I can tell.

> > hmm, how about this:
> >
> >mkdir -p /tmp/.X11-unix
> >chown -h root:root /tmp/.X11-unix
> >stat=$(LC_ALL=C stat -c '%u %g %F' /tmp/.X11-unix)
> >if [ "$stat" != '0 0 directory' ]; then
> >  exit 1
> >fi
> >chmod 1777 /tmp/.X11-unix
> 
> This would work (even if it uses chmod), but wasn't the Bash approach (test
> with "-O", "-G" and "-d") simpler than using "stat"?
> 
test -d follows symlinks...

the problem with stat(1) is it's on /usr so I'd have to make this script
depend on $remote_fs.  Which shouldn't be a problem, but is still a bit
annoying.

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Fri, 02 Mar 2012 21:24:09 GMT) (full text, mbox, link).


Acknowledgement sent to Julien Cristau <jcristau@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Fri, 02 Mar 2012 21:24:09 GMT) (full text, mbox, link).


Message #118 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: vladz <vladz@devzero.fr>, 661627@bugs.debian.org
Cc: Tim <tim-debian@sentinelchicken.org>, "Bernhard R. Link" <brlink@debian.org>
Subject: Re: Bug#661627: init script x11-common creates directories in insecure manners
Date: Fri, 2 Mar 2012 22:09:07 +0100
[Message part 1 (text/plain, inline)]
On Tue, Feb 28, 2012 at 18:31:02 +0100, vladz wrote:

> The init script "x11-common" creates directories "/tmp/.X11-unix" and
> "/tmp/.ICE-unix" in insecure manners.
> 
I've now pushed a change to
git://git.debian.org/git/pkg-xorg/debian/xorg.git that should hopefully
fix this.  The new version of the script is below, I intend to upload
tomorrow if nobody tells me it's still broken.

#!/bin/sh
# /etc/init.d/x11-common: set up the X server and ICE socket directories
### BEGIN INIT INFO
# Provides:          x11-common
# Required-Start:    $remote_fs
# Required-Stop:     $remote_fs
# Default-Start:     S
# Default-Stop:
### END INIT INFO

set -e

PATH=/usr/bin:/usr/sbin:/bin:/sbin
SOCKET_DIR=.X11-unix
ICE_DIR=.ICE-unix

. /lib/lsb/init-functions
if [ -f /etc/default/rcS ]; then
  . /etc/default/rcS
fi

do_restorecon () {
  # Restore file security context (SELinux).
  if which restorecon >/dev/null 2>&1; then
    restorecon "$1"
  fi
}

# create a directory in /tmp.
# assumes /tmp has a sticky bit set (or is only writeable by root)
set_up_dir () {
  DIR="/tmp/$1"

  if [ "$VERBOSE" != no ]; then
    log_progress_msg "$DIR"
  fi
  # if $DIR exists and isn't a directory, move it aside
  if [ -e $DIR ] && ! [ -d $DIR ] || [ -h $DIR ]; then
    mv "$DIR" "$(mktemp -d $DIR.XXXXXX)"
  fi

  error=0
  while :; do
    if [ $error -ne 0 ] ; then
      # an error means the file-system is readonly or an attacker
      # is doing evil things, distinguish by creating a temporary file,
      # but give up after a while.
      if [ $error -gt 5 ]; then
        log_failure_msg "failed to set up $DIR"
        return 1
      fi
      fn="$(mktemp /tmp/testwriteable.XXXXXXXXXX)" || return 1
      rm "$fn"
    fi
    mkdir -p -m 01777 "$DIR" || { rm "$DIR" || error=$((error + 1)) ; continue ; }
    case "$(LC_ALL=C stat -c '%u %g %a %F' "$DIR")" in
      "0 0 1777 directory")
        # everything as it is supposed to be
        break
        ;;
      "0 0 "*" directory")
        # as it is owned by root, cannot be replaced with a symlink:
        chmod 01777 "$DIR"
        break
        ;;
      *" directory")
        # if the chown succeeds, the next step can change it savely
        chown -h root:root "$DIR" || error=$((error + 1))
        continue
        ;;
      *)
        log_failure_msg "failed to set up $DIR"
        return 1
        ;;
    esac
  done

  return 0
}

do_status () {
    if [ -d "/tmp/$ICE_DIR" ] && [ -d "/tmp/$SOCKET_DIR" ]; then
      return 0
    else
      return 4
    fi
}

case "$1" in
  start)
    if [ "$VERBOSE" != no ]; then
      log_begin_msg "Setting up X socket directories..."
    fi
    set_up_dir "$SOCKET_DIR"
    set_up_dir "$ICE_DIR"
    if [ "$VERBOSE" != no ]; then
      log_end_msg 0
    fi
  ;;

  restart|reload|force-reload)
    /etc/init.d/x11-common start
  ;;

  stop)
   :
  ;;

  status)
    do_status
  ;;
  *)
    log_success_msg "Usage: /etc/init.d/x11-common {start|stop|status|restart|reload|force-reload}"
    exit 1
    ;;
esac

exit 0

# vim:set ai et sts=2 sw=2 tw=0:

Cheers,
Julien
[signature.asc (application/pgp-signature, inline)]

Reply sent to Julien Cristau <jcristau@debian.org>:
You have taken responsibility. (Sat, 03 Mar 2012 18:21:09 GMT) (full text, mbox, link).


Notification sent to vladz <vladz@devzero.fr>:
Bug acknowledged by developer. (Sat, 03 Mar 2012 18:21:09 GMT) (full text, mbox, link).


Message #123 received at 661627-close@bugs.debian.org (full text, mbox, reply):

From: Julien Cristau <jcristau@debian.org>
To: 661627-close@bugs.debian.org
Subject: Bug#661627: fixed in xorg 1:7.6+12
Date: Sat, 03 Mar 2012 18:19:21 +0000
Source: xorg
Source-Version: 1:7.6+12

We believe that the bug you reported is fixed in the latest version of
xorg, which is due to be installed in the Debian FTP archive:

x11-common_7.6+12_all.deb
  to main/x/xorg/x11-common_7.6+12_all.deb
xbase-clients_7.6+12_all.deb
  to main/x/xorg/xbase-clients_7.6+12_all.deb
xorg-dev_7.6+12_all.deb
  to main/x/xorg/xorg-dev_7.6+12_all.deb
xorg_7.6+12.dsc
  to main/x/xorg/xorg_7.6+12.dsc
xorg_7.6+12.tar.gz
  to main/x/xorg/xorg_7.6+12.tar.gz
xorg_7.6+12_amd64.deb
  to main/x/xorg/xorg_7.6+12_amd64.deb
xserver-xorg-input-all_7.6+12_amd64.deb
  to main/x/xorg/xserver-xorg-input-all_7.6+12_amd64.deb
xserver-xorg-video-all_7.6+12_amd64.deb
  to main/x/xorg/xserver-xorg-video-all_7.6+12_amd64.deb
xserver-xorg_7.6+12_amd64.deb
  to main/x/xorg/xserver-xorg_7.6+12_amd64.deb
xutils_7.6+12_all.deb
  to main/x/xorg/xutils_7.6+12_all.deb



A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 661627@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Julien Cristau <jcristau@debian.org> (supplier of updated xorg package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Format: 1.8
Date: Sat, 03 Mar 2012 18:54:30 +0100
Source: xorg
Binary: x11-common xserver-xorg xserver-xorg-video-all xserver-xorg-input-all xorg xorg-dev xbase-clients xutils
Architecture: source all amd64
Version: 1:7.6+12
Distribution: unstable
Urgency: high
Maintainer: Debian X Strike Force <debian-x@lists.debian.org>
Changed-By: Julien Cristau <jcristau@debian.org>
Description: 
 x11-common - X Window System (X.Org) infrastructure
 xbase-clients - miscellaneous X clients - metapackage
 xorg       - X.Org X Window System
 xorg-dev   - X.Org X Window System development libraries
 xserver-xorg - X.Org X server
 xserver-xorg-input-all - X.Org X server -- input driver metapackage
 xserver-xorg-video-all - X.Org X server -- output driver metapackage
 xutils     - X Window System utility programs metapackage
Closes: 661627
Changes: 
 xorg (1:7.6+12) unstable; urgency=high
 .
   * Fix unsafe manipulation of /tmp/.X11-unix and /tmp/.ICE-unix in the
     x11-common init script.  A malicious user could trick us into changing
     ownership/permissions of an arbitrary directory, and elevate their
     privileges (closes: #661627).  Reference: CVE-2012-1093.  Thanks to
     "vladz", Tim Morgan and Bernhard R. Link for their help getting this right
     (any remaining bugs are my own).
Checksums-Sha1: 
 c16d4bbe3abfa9eda5c9ebdc5d6920c785e0d323 1957 xorg_7.6+12.dsc
 50d7a6e2bc7026d876de63cec2ba10f0659eb587 922670 xorg_7.6+12.tar.gz
 d5296c059e6d101b063e6923226538e6ecbd30d5 282590 x11-common_7.6+12_all.deb
 cc0e742b5dc5ae1e0670b1b87928e74f22f9bab8 35180 xorg-dev_7.6+12_all.deb
 112ca719b6f1342bbbb19e6a0dd136b86005a3f8 35046 xbase-clients_7.6+12_all.deb
 c564b64d12b585936dd31db85fe5cb8d803d7fc5 34938 xutils_7.6+12_all.deb
 66323afa68848ffde1ad905a078ae9bd7a6602d1 111758 xserver-xorg_7.6+12_amd64.deb
 f94a3700317be4dd6a5c2d6995eb7885568bc421 35014 xserver-xorg-video-all_7.6+12_amd64.deb
 146bf564a8c1f31e7cae0c07e307ed9ee4f098aa 34886 xserver-xorg-input-all_7.6+12_amd64.deb
 2c8a3b15b9db3d8d0646dd3f64104d098f078bdd 35544 xorg_7.6+12_amd64.deb
Checksums-Sha256: 
 5a1a7f6f6f6dccb568f2d70034969fe0ed10ce1724bbee4f587ffa1ab58899a7 1957 xorg_7.6+12.dsc
 759fc337e04e054fbfd19e83b103814a2b8f17cf23b9f45b2143cc82349d3fcb 922670 xorg_7.6+12.tar.gz
 c43c595f43cdd364adfba155dd6b11069a8bfddd17b336dd75107125b2d49faa 282590 x11-common_7.6+12_all.deb
 87e5eb60ef591702e449f02ca442a7c12884ef0aa81ecd49ac9fbcc0e02c9ac5 35180 xorg-dev_7.6+12_all.deb
 e21b4fc5507e0f77baeb6faaad1cb060bfd363594bcfa6fe6f3200bfe708abe7 35046 xbase-clients_7.6+12_all.deb
 176c80426f547750293af1ddc79d062431608c0b07eb5e46b8acb6fda5569d91 34938 xutils_7.6+12_all.deb
 b9d0f15c1e430aa4da8967cc6b382e907364173ade0c4c42b79bf49de8a35fd1 111758 xserver-xorg_7.6+12_amd64.deb
 a08af50ee714fdc05888d213a069a502fbef1001eeb5f9ce52805413160c936c 35014 xserver-xorg-video-all_7.6+12_amd64.deb
 f8b3035213f7cbb4e59f122e29230dda526364804a3694001a17fc3e8e45f393 34886 xserver-xorg-input-all_7.6+12_amd64.deb
 f1cffd91c2b06f41142604309ac4ff0654d076486753197ff930d726c986e5e2 35544 xorg_7.6+12_amd64.deb
Files: 
 9c4a71c8e9a02f99a539b19847c99eab 1957 x11 optional xorg_7.6+12.dsc
 c34c36415287321393cee372de0b7ad4 922670 x11 optional xorg_7.6+12.tar.gz
 9108e934995a8903988c296d891e8d0c 282590 x11 optional x11-common_7.6+12_all.deb
 7c077c84369fe6128278b78d59bbde15 35180 x11 optional xorg-dev_7.6+12_all.deb
 a91712a123a38f985692dc9783c45d19 35046 x11 optional xbase-clients_7.6+12_all.deb
 8eb01663ea708e1c8a2722bcff523373 34938 x11 optional xutils_7.6+12_all.deb
 cad407b409bf329c85558c7689f5fb79 111758 x11 optional xserver-xorg_7.6+12_amd64.deb
 adb4ce79569ed9bbe403daff5ae4ca60 35014 x11 optional xserver-xorg-video-all_7.6+12_amd64.deb
 1372596f7c4c242518e78d967973327c 34886 x11 optional xserver-xorg-input-all_7.6+12_amd64.deb
 c149bbccf4f6b223c286d88222a35ec8 35544 x11 optional xorg_7.6+12_amd64.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBCAAGBQJPUlvCAAoJEDEBgAUJBeQMQtAP/2pHCd3BoAwM+mJhK+rgrb69
gCpY93L80sUCodTw1Q3GfkWleuz/UEL5HcrOabz64AmK3ICiXzq7gJ5gsfZDLPXe
NcYOs5DKKQZUNZDVS353OU6Fd441YiSxeC3SfSXItDrgp42YgY/f+4VvOZ10q9jw
iodf5/7/wmATgKv/i+VNI3SiynUGPOKCtFkxRp7In6ae8mtNSHdbybQyep//RKUH
G/Zw2BwzaBC0ElxVCAh82wd32PzsDyrRyKtcU7V6A67DSo6VwkbvXtxV8a1t3dRu
Y7A0C2l+KDAWl2qKfLYFxGUYGV0gTzkn3c4E9T02Zn6UJzl2WyRtRHF+5Teiox/R
CSqzwVGnZ3lfPGTB851VHsCHovAi08ewhWUH8pws/qPxTQ1CgN7E5vCj3g6O5Ngv
r9mIt1gJnMi2/a7k9FN+lWJJs7XszgFprvNxmhMEHu3Yjk/klwGhkhp6ZmIv35di
BNbsFoumB9ccJhzI/DHkBa5Y2+bsQ6JghpHSarNe3xodH/3vR9cZ1yJEvwFA/nRK
uNFZP72VthwvsE1FwSWq+li4i3LJtH2TPMzv7sEGxjQW1iDaWVgSvFKIceyQa5bQ
Z6FYorMC2+K3z5LA229s9zwQBxFkQOCNksomrBZ2j2wPta4khCu0W7X40qThDkqI
d+7tHFHQuBBpAlyrAPYy
=gIuL
-----END PGP SIGNATURE-----





Bug archived. Request was from Debbugs Internal Request <owner@bugs.debian.org> to internal_control@bugs.debian.org. (Tue, 03 Apr 2012 07:40:00 GMT) (full text, mbox, link).


Bug unarchived. Request was from jmw@debian.org to control@bugs.debian.org. (Sun, 08 Jul 2012 16:22:12 GMT) (full text, mbox, link).


Information forwarded to debian-bugs-dist@lists.debian.org, Debian X Strike Force <debian-x@lists.debian.org>:
Bug#661627; Package x11-common. (Mon, 09 Jul 2012 07:27:30 GMT) (full text, mbox, link).


Acknowledgement sent to Jonathan Wiltshire <jmw@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian X Strike Force <debian-x@lists.debian.org>. (Mon, 09 Jul 2012 07:27:30 GMT) (full text, mbox, link).


Message #132 received at 661627@bugs.debian.org (full text, mbox, reply):

From: Jonathan Wiltshire <jmw@debian.org>
To: 661627@bugs.debian.org
Subject: Re: init script x11-common creates directories in insecure manners
Date: Mon, 09 Jul 2012 07:23:40 -0000
Dear maintainer,

Recently you fixed one or more security problems and as a result you closed
this bug. These problems were not serious enough for a Debian Security
Advisory, so they are now on my radar for fixing in the following suites
through point releases:

squeeze (6.0.6) - use target "stable"

Please prepare a minimal-changes upload targetting each of these suites,
and submit a debdiff to the Release Team [0] for consideration. They will
offer additional guidance or instruct you to upload your package.

I will happily assist you at any stage if the patch is straightforward and
you need help. Please keep me in CC at all times so I can
track [1] the progress of this request.

For details of this process and the rationale, please see the original
announcement [2] and my blog post [3].

0: debian-release@lists.debian.org
1: http://prsc.debian.net/tracker/661627/
2: <201101232332.11736.thijs@debian.org>
3: http://deb.li/prsc

Thanks,

with his security hat on:
--
Jonathan Wiltshire                                      jmw@debian.org
Debian Developer                         http://people.debian.org/~jmw

4096R: 0xD3524C51 / 0A55 B7C5 1223 3942 86EC  74C3 5394 479D D352 4C51





Bug archived. Request was from Debbugs Internal Request <owner@bugs.debian.org> to internal_control@bugs.debian.org. (Tue, 07 Aug 2012 07:29:27 GMT) (full text, mbox, link).


Send a report that this bug log contains spam.


Debian bug tracking system administrator <owner@bugs.debian.org>. Last modified: Wed Jun 19 14:30:21 2019; Machine Name: beach

Debian Bug tracking system

Debbugs is free software and licensed under the terms of the GNU Public License version 2. The current version can be obtained from https://bugs.debian.org/debbugs-source/.

Copyright © 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson, 2005-2017 Don Armstrong, and many other contributors.