[PATCH RFC] Xorg: Add a suid root wrapper

[Hans de Goede]

Hi,

On 03/06/2014 06:40 AM, Alan Coopersmith wrote:
> On 03/ 5/14 07:51 AM, Hans de Goede wrote:
>> This commit adds a little suid root wrapper, which is a bit weird, first we
>> strip the suid-root bit of the Xorg binary, and then we add a wrapper ?
> 
> Have you looked at Debian's Xwrapper and considered adopting it, or at least
> some of its functionality?

I was not aware of Debian's Xwrapper, thanks for pointing it out to me.

> (The overall plan seems reasonable, but I think they've already thought through
>  some details you're currently missing.)

So looking at Debian's Xwrapper I see several things in there:

1) It has a config file to determine which users are allowed to run the
xserver through the wrapper, with 3 possible settings: a) root only
b) users on the local console only c) everyone

Having a config file seems like a good idea, I was a bit worried about
my wrapper thinking it would be ok to drop root rights when it would
not be on hybrid gpu laptop configs. So it would be good to have an
override option for this in a config file

2) If configured to do so it will only allow local console users to
start X through the wrapper, this to seems a useful thing to have

3) It installs the wrapper as /usr/bin/X rather then /usr/bin/Xorg,
and keeps the real server as /usr/bin/Xorg I think this is a bad idea
since some users (ie gdm) directly execute /usr/bin/Xorg

4) It checks to see if the real xserver it will execute is not
a symlink to the wrapper, and it does so in a rather naive way
(using hardcoded paths for the possible wrapper install location)
since both the wrapper and the Xserver should be in directories which
can only be written by root, and an absolute path is used I don't see
this as a necessary thing to do. If an attacker can replace the
real Xserver binary with a symlink to the wrapper, he can do much
worse things then replace it with a symlink back to the wrapper.

This is likely to check for some configuration error caused by 3,
all in all I don't see this as useful.

5) It does a check if the real Xserver is executable by the
real uid, this is mostly yet another broken config check but it
cannot hurt, so I'll add this to my wrapper too.

6) It does some checks on /tmp/.X11, if we want these we should
do them in the server itself and not in the wrapper IMHO.

Unfortunately the Xwrapper code is GPL, so not suitable for us,
still there are some good ideas in there, so I'll extend my
wrapper to use them.

I'll keep the config-file location and format compatible so that
hopefully Debian can simply switch over to my wrapper in the future.

So TL;DR: Yes there are some good ideas in there, I'll add those
to the next revision of my wrapper.

Regards,

Hans