weston-launch is setuid, so it should handle the environment in a paranoid way
Submitted by Simon McVittie
Assigned to Wayland bug list
Description
Similar to Bug #83849, weston-launch runs with elevated privileges (it is setuid), so it needs to be careful not to trust its environment. It is linked to arbitrary libraries (via libpam if nothing else), and should not assume that those libraries are all designed to be setuid-safe - most libraries aren't.
(See, e.g., Bug #52202 in libdbus, which was not designed to be setuid-safe, and had that bolted on as an afterthought when it became clear that people were using it in an unsupported way anyway.)
It is possible that weston-launch is actually completely OK - it does do a clearenv() before invoking PAM, and the rest of its code seems to be just libc and libsystemd.
However, it would be more obviously correct (the best kind of correctness for security-sensitive code) if it behaved more like this pseudocode:
original_environ = deep-copy of environ
clearenv()
foreach (whitelist of known-safe variables, e.g. TERM):
if (variable is in original_environ and its value is safe):
copy it to new environment
... do stuff with privileges ...
if (on code path where we drop privileges):
fork() or whatever
if (in child process):
drop privileges
(optionally) put original_environ back
exec(thing that must run as original user)When I say "its value is safe" I mean a check specific to that variable: the more strict its consumers are, the more lenient you can be. For instance, pkexec does this:
SHELL: must be in /etc/shells XAUTHORITY: must not contain % or .. LANG, LINGUAS, LANGUAGE, LC_*, TERM, COLORTERM: must not contain /, % or ..
which seems reasonable.