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
Attachment:
signature.asc
Description: Digital signature