Avoid clobbering environment variables loaded from /etc/profile.d

[ArrayBolt3]

qubes-session loads environment variables from
systemctl --user show-environment, so that variables set using systemd environment generators are present in the user session. Previously, when one of those generators provided an environment variable that was already set by a script under /etc/profile.d, the variable from the environment generator would clobber the variable from /etc/profile.d. This is backwards - variables from /etc/profile.d should take precedence over those defined in systemd environment generators.

To fix this, update the systemd user manager's environment with the contents of the session's environment immediately before updating the session's environment with the environment from the systemd user manager. This two-way sync means that all variables defined in both environment generators and profile scripts end up in the user's environment, while making variable definitions from the profile scripts take priority over variable definitions from environment generators.

Fixes: QubesOS/qubes-issues#10299

[ArrayBolt3]

Note that this results in a lot of new environment variables being loaded into systemd's user manager that weren't previously. I believe this is correct behavior, however it might change the behavior of other user services and qrexec services. Whatever testing needs to be done to make sure that doesn't break the world, should probably be done.

[marmarek]

This will now break some of the systemd-defined variables (including XDG_DATA_DIRS parts added there, so - flatpak). The problem with this approach is that /etc/profile.d scripts are usually handling already existing variables by appending value (that applies at least to PATH, but XDG_DATA_DIRS too). If you now override one with the other, you break /etc/profile.d expectation (it was meant to extend values, but now it will override them). What probably wants happening here, is loading systemd-defined variables earlier, before /etc/profile gets loaded. I'm not 100% sure where that actually should be. Maybe somewhere in https://github.com/QubesOS/qubes-gui-agent-linux/blob/main/gui-agent/qubes-gui-runuser.c (which is used in https://github.com/QubesOS/qubes-gui-agent-linux/blob/main/appvm-scripts/usrbin/qubes-run-xorg) ?

Note also that /etc/profile.d is meant for shell variables. I don't think any spec mandates loading it for non-shell apps. So, if you want a variable to be set for various GUI apps (not just shell running in a terminal emulator), you should probably use a different mechanism (likely /etc/environment.d). For example on native Xfce, if you launch an app from the menu, it won't see variables set via /etc/profile.d (you can check via /proc/PID/environ, as ofc opening shell will load /etc/profile.d; check for example LESSOPEN - this one seems to be set only in /etc/profile.d). /etc/profile.d works great for power users who open all applications from a terminal, which is likely why it's such a common habit...

See also QubesOS/qubes-issues#6408 which demanded /etc/environment.d to be respected. And also #151 for some more background.

[ArrayBolt3]

Maybe moving to environment.d in the future can be a goal, but right now to my awareness all of the system-wide environment variable manipulation in Whonix (and quite a bit of it done in Qubes it appears) is done in /etc/profile.d or /etc/X11/Xsession.d, and Whonix does a lot of environment variable manipulation. Changing everything to environment.d this late in the game might not be entirely out of the question, but it would definitely fall into the "last resort" category.

Loading things from /etc/environment.d first sounds like a good solution. qubes-gui-runuser sounds like kind of an awkward place to do it though since that requires lots of possibly messy string parsing in C. It seems like it would be easier (possibly) to do it in all scripts that eventually end up calling qubes-session. I don't know where all it is called though, and a string search in Github wasn't all that clear.

[ArrayBolt3]

I guess after further thought, maybe implementing this in qubes-gui-runser.c is better. Otherwise we risk this bug popping back up if someone creates a new script that calls qubes-session via qubes-gui-runuser and doesn't parse the environment variables from systemd. The environment variables are just going to be newline-separated key-value pairs, so whatever "parsing" is needed can't be that hard.

[marmarek]

I proposed qubes-gui-runuser, because that's what calls shell that will eventually call qubes-session, so it runs before shell startup scripts.
As for parsing, I don't think you really need it. You can simply ask systemd for the variables using proper API (dbus), no need for asking systemctl --user show-environment to format it into string just to have it split into variables back. It will add dbus dependency to that tool, but it's kinda there already via pam_systemd. Oh, maybe there is a simpler solution if there is some pam module that would load those variables?

[ArrayBolt3]

There's code in qubes-gui-runuser that can work without PAM, it seems to me like a bugfix should ideally work in both scenarios. Probably better to use dbus.

edit: There is pam_env, but it can only load explicitly provided environment variables, or variables from a file. We'd have to dump the environment variables from systemd into a file first, then load them with pam_env, to do this in PAM. That sounds less secure and more difficult to me.

[ArrayBolt3]

Well, turns out when https://dbus.freedesktop.org/doc/api/html/index.html says "If you use this low-level API directly, you're signing up for some pain", it is not joking! My latest commit is entirely untested, probably won't even compile, and doesn't handle the non-PAM case I wanted it to handle, but, for whatever it's worth, it's there. If this is an entirely unsuitable approach, I can give up on this, otherwise I'll finish polishing and testing this in the near future.

[marmarek]

Pain it is indeed... Aren't there any helpers for that?
And indeed it doesn't compile (missing build dep).

But also, I think it needs to be called only after pam_open_session - this is where session bus gets started. This may mean using pam_putenv might be not okay (to be checked)? If so, get_systemd_env_from_dbus probably should take already existing array (that was returned by pam_getenvlist) and extend it, instead of allocating new one.

[ArrayBolt3]

There are helpers for this, but they are mainly parts of larger application frameworks like (quoting from the API's main page) GLib, Qt, Python, Mono, Java, etc. D-Bus is also intended to be an event-driven RPC framework where things are managed by callback functions and a main loop, whereas what I'm doing is more like calling an HTTP endpoint and getting a response back. That isn't quite the normal use case for D-Bus AFAICT, thus I don't expect there will be a well-supported helper in general for this kind of thing.

Good point about pam_open_session, I'll move the call appropriately (and hopefully fully test this before the next push).

[ArrayBolt3]

It looks like the no-PAM code in qubes-gui-runuser is entirely broken - the execve line at the end references a variable env that doesn't even exist in the function or as a global. Not sure if it's worth trying to fix it now, I'll leave it broken and not add the systemd environment fixes. In the long run, should the code be repaired, or would it be better to just discard it?

[ArrayBolt3]

Alright, this time it builds locally, and it actually seems to work on my end, though I've not tested it nearly as much needed to consider it rigorously tested. I think at this point we can be fairly certain this solution can work, so some security review on the mess required to talk to systemd over D-Bus would be much appreciated.

Some TODOs that aren't already in the code:

• The error messages from the D-Bus interaction code should make their origin more clear, and it's possible the errors themselves could be clearer.
• The code needs to use "D-Bus" as the term for the message bus, not "DBus".
• There should be a comment somewhere documenting that one should never pass a char ** that contains any unsafe-to-free memory to get_systemd_env_from_dbus. The function reallocarray's the passed-in array numerous times, and can free elements within the array to replace them with elements copied from systemd.
• This whole mechanism should be tested in isolation - right now all I know is my environment looks like I'd expect (without things from /etc/profile.d being clobbered) when I boot with this, I don't yet know if getting the environment variables from systemd actually succeeded or not.

[ArrayBolt3]

After some further testing (in which I caught the above bugs and also a minor memory leak), it looks like the D-Bus interaction code itself works once those bugs are fixed, but for some reason in qubes-gui-runuser trying to even use D-Bus at all results in the error qubes-gui-runuser: Failed to initialize DBus, error name: 'org.freedesktop.DBus.Error.Spawn.ExecFailed', error contents: '/usr/bin/dbus-launch terminated abnormally without any error message'.

Edit: This turned out to be because DBUS_SESSION_BUS_ADDRESS wasn't properly set in the environment of qubes-gui-runuser, It needed to be taken from the environment returned by pam_getenvlist and putenv'd into the executable's real environment. Doing that fixed things.