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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
