Bug#661627: init script x11-common creates directories in insecure manners

[vladz]

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




--
To UNSUBSCRIBE, email to debian-bugs-...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listm...@lists.debian.org

[Julien Cristau]

This script is set -e AFAICT, which means it already does care about the
mkdir return code.

Cheers,
Julien

[vladz]

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

[vladz]

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).

[Julien Cristau]

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

[vladz]

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,

[Julien Cristau]

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

[Julien Cristau]

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

[Julien Cristau]

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