Libraries have traditionally been responsible for avoiding bogus use of
environment variables themselves in a single place, instead of requiring
every caller to do so.   For instance, we have checks in libX11 to not
trust variables that set paths to code to be dlopened if called from setuid
processes, libc does the same for NLSPATH, and the runtime loader itself
does so for LD_LIBRARY_PATH and friends.

Dbus is the first library I'm aware of the X server using that requires us
to do this on it's behalf, so we've never had a need for this before.

(In reply to comment #1)
> Libraries have traditionally been responsible for avoiding bogus use of
> environment variables themselves in a single place, instead of requiring
> every caller to do so.

(libdbus maintainer here; speaking only for myself, not for the other dbus maintainers.)

That can't work particularly well in general: a library has no idea how the process that links it was invoked, or whether it's acting as a trust boundary (which setuid things are, but 99% of libdbus users aren't). To put it another way, there's a limit to how much we can avoid bogus use of environment variables, because we don't know whether your environment variables are bogus. When they're not, not using them would be an ABI break - I suppose you could argue that that was a design flaw in how the D-Bus session bus address is communicated, but the time to argue that was several years before I got involved.

The best we can do at the moment is to check whether euid != ruid, which is what libdbus currently does. That sort of works, until someone calls setresuid() or similar, either to make the euid "real" (keep the higher privileges) or to drop privileges (keep the lower privileges - but existing fds, etc. that give more privilege remain). In principle we should be using glibc's secure_getenv() instead, to have the same special handling for executables with filesystem capabilities, but we had to revert that because it broke gnome-keyring - it seems we can't win here.

setuid executables create a trust boundary: the euid is more privileged than the parent process, but is operating in a somewhat precarious situation because its inherited process environment came from the parent, so it has to cope with a malicious parent process giving it crafted environment variables, or fds already open to a chosen file, or bizarre choices of rlimit, or whatever. I would argue that because the setuid executable is creating the trust boundary, it's the setuid executable that is responsible for cleaning up the inherited environment to the extent that non-trivial library code can be run safely.

I'm aware that historical practice is that executables do whatever they do, and libraries (which may or may not have been designed to be a trust boundary) try to be as safe as possible in whatever situation they find themselves used, without knowing which facets of their execution environment are trustworthy. I'm not sure that that's a particularly sustainable model.

There seems to be a trend for recently-written setuid executables (e.g. pkexec) to clear out their environment with a whitelist on startup, and I think that's a good direction. If the executable happens to only use libraries that also distrust their environment, great, they weren't using the variables that got cleared anyway; if not, vulnerability avoided, also good.

(In reply to comment #2)
> I would argue that because the setuid executable is
> creating the trust boundary, it's the setuid executable that is responsible
> for cleaning up the inherited environment to the extent that non-trivial
> library code can be run safely.

One corollary of that is that I think distributors should never make a binary setuid unless its upstream maintainer specifically intended it to be safe for setuid. I'm aware that X is in an unusual situation here because it's expected to deal with several inconsistent sets of historical practices: Red Hat have historically made X itself setuid; Debian run it via a setuid wrapper (which really ought to clear the environment but I don't think it does); and as far as I'm aware, X only actually needs to be setuid if it's going to be run by startx/xinit, whereas display managers run it with privileges and a clean(ish) environment in any case.

To be clear - I didn't close the bug as WONTFIX or INVALID, just explained
why the xserver doesn't currently do this.   Changing it to do this would
mostly be an issue of finding someone who cares with the necessary development
skills, and that's a tough problem with the low participation rates in xserver
development these days.

> The best we can do at the moment is to check whether euid != ruid, which is
> what libdbus currently does.

Sorry, since I don't normally work on Linux, I forgot that it didn't have an
equivalent of the BSD/Solaris issetugid() system call that we use for this in
libX11 & xorg.

(In reply to comment #3)
> Red Hat have historically made X itself setuid; Debian run it via
> a setuid wrapper (which really ought to clear the environment but I don't
> think it does);

There's now a setuid wrapper for all distros added in the Xorg 1.16 release:
   http://patchwork.freedesktop.org/patch/21863/
as part of our longer-term effort to get X to need root privileges less often,
and yes, the Xwrapper.config file seems like the place to add the list of
environment variables to whitelist.

(In reply to comment #5)
> (In reply to comment #3)
> > Red Hat have historically made X itself setuid; Debian run it via
> > a setuid wrapper (which really ought to clear the environment but I don't
> > think it does);
> 
> There's now a setuid wrapper for all distros added in the Xorg 1.16 release:
>    http://patchwork.freedesktop.org/patch/21863/
> as part of our longer-term effort to get X to need root privileges less
> often,
> and yes, the Xwrapper.config file seems like the place to add the list of
> environment variables to whitelist.

I agree with Alan that having this added to the shared setuid wrapper, and allowing to whitelist environment variables through Xwrapper.config sounds like a good idea.

I would certainly welcome and review such a patch.

p.s.

Laurent, has Debian switched to the new xserver provided wrapper ? And if not why not, and can we fix the wrapper to make it work for Debian ?

(In reply to comment #6)
> Laurent, has Debian switched to the new xserver provided wrapper ? And if
> not why not, and can we fix the wrapper to make it work for Debian ?

Not yet, mostly because we/I haven't got around to doing/testing it yet.

Hello,

Do we have a list somewhere of the environment variables that should be preserved by default?

Julien has added this patch[0] in debian, but the xserver is trying to write the logs in /var/log even if it runs as !root user. I've found that HOME and XDG_DATA_HOME should at least be passed to the Xorg executable.

An idea of any other one?

Cheers,

[0]https://anonscm.debian.org/cgit/pkg-xorg/xserver/xorg-server.git/tree/debian/patches/xorg-wrapper-envp.diff?h=debian-experimental

This should be fixed by 1d4aa672424d8b1629fda11400b88607b5066965