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
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
