Bug 84040 - weston-launch is setuid, so it should handle the environment in a paranoid way
Summary: weston-launch is setuid, so it should handle the environment in a paranoid way
Status: RESOLVED MOVED
Alias: None
Product: Wayland
Classification: Unclassified
Component: weston (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Wayland bug list
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-18 11:55 UTC by Simon McVittie
Modified: 2018-06-08 23:53 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2014-09-18 11:55:26 UTC
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.
Comment 1 GitLab Migration User 2018-06-08 23:53:14 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/wayland/weston/issues/55.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.