qubes-gui-runner: get environment from systemd user instance and pass it through to the session

qubes-session previously loaded environment variables from
`systemctl --user show-environment`, so that variables set using systemd
environment generators would be present in the user session. However,
doing this meant any environment variables defined or augmented by
scripts under /etc/profile.d could be clobbered by versions of those
same variables from the systemd user instance, resulting in application
misbehavior in some instances.

To fix this, load all environment variables from systemd's user instance
in qubes-gui-runner instead. This will augment the environment provided
by PAM with the environment provided by systemd, then the /etc/profile.d
scripts can augment the environment further. This should prevent any
variables from being incorrectly clobbered, and allow all mechanisms of
providing environment variables to the end-user's session to function
as intended.

Fixes: QubesOS/qubes-issues#10299

Author: ArrayBolt3

appvm-scripts/usrbin/qubes-session

@@ -27,17 +27,6 @@
 
 loginctl activate "$XDG_SESSION_ID"
 
-# Now import the environment from the systemd user session.
-# This is necessary to enable users to configure their
-# Qubes environment using the standard environment.d
-# facility.  Documentation for the facility is at:
-# https://www.freedesktop.org/software/systemd/man/environment.d.html
-set -a # export all variables
-env=$(systemctl --user show-environment) && eval "$env" || exit
-set +a
-unset env
-
-
 if qsvc guivm-gui-agent; then
     if [ -e "$HOME/.xinitrc" ]; then
         . "$HOME/.xinitrc"

debian/control

@@ -26,6 +26,7 @@ Build-Depends:
     qubesdb-dev,
     libltdl-dev,
     libunistring-dev,
+    libdbus-1-dev,
 Standards-Version: 4.4.0.1
 Homepage: http://www.qubes-os.org/
 #Vcs-Git: git://git.debian.org/collab-maint/qubes-gui-agent.git

gui-agent/Makefile

@@ -20,7 +20,8 @@
 #
 
 CC ?= gcc
-CFLAGS += -I../include/ `pkg-config --cflags vchan` -g -Wall -Wextra -Werror -fPIC \
+CFLAGS += -I../include/ `pkg-config --cflags vchan` \
+	  `pkg-config --cflags dbus-1` -g -Wall -Wextra -Werror -fPIC \
 	  -Wmissing-prototypes -Wstrict-prototypes -Wold-style-declaration \
 	  -Wold-style-definition
 OBJS = vmside.o txrx-vchan.o error.o list.o encoding.o
@@ -33,7 +34,7 @@ qubes-gui: $(OBJS)
 	$(CC) $(LDFLAGS) -pie -g -o qubes-gui $(OBJS) \
 		$(LIBS)
 qubes-gui-runuser: CFLAGS += -g -Wall -Wextra -Werror -pie -fPIC
-qubes-gui-runuser: LDLIBS += -lpam -lqubesdb
+qubes-gui-runuser: LDLIBS += -lpam -lqubesdb -ldbus-1
 qubes-gui-runuser: qubes-gui-runuser.c
 clean:
 	rm -f qubes-gui qubes-gui-runuser ./*.o ./*~

gui-agent/qubes-gui-runuser.c

@@ -31,13 +31,257 @@
 #include <grp.h>
 #include <pwd.h>
 #include <err.h>
+#include <stdbool.h>
 #include <qubesdb-client.h>
+#include <dbus/dbus.h>
 
 #ifdef HAVE_PAM
 #include <security/pam_appl.h>
 #endif
 
-pid_t child_pid = 0;
+pid_t fork_pid = 0;
+
+/* Note: augment_pam_env_with_systemd_env expects out_env_ref to be pointer to
+ * a NULL-terminated array of strings consisting of equals-sign-separated
+ * key-value pairs. All items in out_env_ref MUST be heap-allocated, as this
+ * function is liable to free() any item in the passed-in array in order to
+ * replace it with an item obtained from systemd's environment.
+ *
+ * Note also, this function talks with the systemd user instance, not the
+ * system instance (pid 1).
+ */
+static void augment_pam_env_with_systemd_env(char ***out_env_ref)
+{
+    DBusConnection *dbus_conn = NULL;
+    DBusError error_data = { 0 };
+    DBusMessage *env_request = NULL;
+    const char *systemd_manager_str = "org.freedesktop.systemd1.Manager";
+    const char *environment_str = "Environment";
+    dbus_bool_t ret = FALSE;
+    DBusMessage *env_reply = NULL;
+    int reply_type = 0;
+    DBusMessageIter reply_iter = { 0 };
+    DBusMessageIter reply_inner_iter = { 0 };
+    char *inner_iter_typesig = NULL;
+    DBusMessageIter reply_arr_iter = { 0 };
+    const char *env_val = NULL;
+    char **env_arr = NULL;
+    char **out_env_arr = NULL;
+    size_t env_arr_len = 0;
+    size_t out_env_arr_len = 0;
+    size_t out_env_idx = 0;
+    size_t env_idx = 0;
+    char *out_env_eq_ptr = NULL;
+    char *env_eq_ptr = NULL;
+    size_t out_env_pre_eq_len = 0;
+    size_t env_pre_eq_len = 0;
+    bool did_override_env_var = false;
+    int current_type = 0;
+
+    if (out_env_ref == NULL)
+        errx(1, "augment_pam_env_with_systemd_env: NULL out_env_ref argument is unsupported!\n");
+    out_env_arr = *out_env_ref;
+    if (out_env_arr == NULL)
+        errx(1, "augment_pam_env_with_systemd_env: NULL array in out_env_ref argument is unsupported!\n");
+    for (out_env_idx = 0; out_env_arr[out_env_idx] != NULL; out_env_idx++) {
+        out_env_arr_len++;
+    }
+    /* Increment 1 to include the NULL element at the end of the array. */
+    out_env_arr_len++;
+
+    /* Initialize D-Bus. */
+    dbus_error_init(&error_data);
+    dbus_conn = dbus_bus_get(DBUS_BUS_SESSION, &error_data);
+    if (dbus_conn == NULL) {
+        warnx("augment_pam_env_with_systemd_env: Failed to initialize D-Bus, error name: '%s', error contents: '%s'\n",
+              error_data.name,
+              error_data.message);
+        goto dbus_cleanup;
+    }
+
+    /* dbus_bus_get sets up our process to be killed if the D-Bus connection
+     * closes. We don't want that, turn that off.
+     */
+    dbus_connection_set_exit_on_disconnect(dbus_conn, FALSE);
+
+    /* Create a D-Bus method call message for getting the "Environment"
+     * property of org.freedesktop.systemd1.Manager.
+     */
+    env_request = dbus_message_new_method_call("org.freedesktop.systemd1",
+                                               "/org/freedesktop/systemd1",
+                                               "org.freedesktop.DBus.Properties",
+                                               "Get");
+    if (env_request == NULL) {
+        warnx("augment_pam_env_with_systemd_env: Failed to create D-Bus method call object!\n");
+        goto dbus_cleanup;
+    }
+
+    ret = dbus_message_append_args(env_request,
+                                   DBUS_TYPE_STRING, &systemd_manager_str,
+                                   DBUS_TYPE_STRING, &environment_str,
+                                   DBUS_TYPE_INVALID);
+    if (ret == FALSE) {
+        warnx("augment_pam_env_with_systemd_env: Failed to append arguments to D-Bus method call object!\n");
+        goto dbus_cleanup;
+    }
+
+    /* Send the method call to systemd, waiting a maximum of 500 milliseconds
+     * for a response.
+     */
+    env_reply = dbus_connection_send_with_reply_and_block(dbus_conn,
+                                                          env_request,
+                                                          500,
+                                                          &error_data);
+    if (env_reply == NULL) {
+        warnx("augment_pam_env_with_systemd_env: Failed to request environment data from systemd, error name: '%s', error contents: '%s'\n",
+              error_data.name,
+              error_data.message);
+        goto dbus_cleanup;
+    }
+
+    /* Ensure the reply is a method call return value. */
+    reply_type = dbus_message_get_type(env_reply);
+    if (reply_type != DBUS_MESSAGE_TYPE_METHOD_RETURN) {
+        warnx("augment_pam_env_with_systemd_env: Did not get method call return object from systemd!\n");
+        goto dbus_cleanup;
+    }
+
+    /* D-Bus property get methods return a Variant, which is not a basic type,
+     * thus we have to iterate through the method return value to get to the
+     * contents.
+     */
+    ret = dbus_message_iter_init(env_reply, &reply_iter);
+    if (ret == FALSE) {
+        warnx("augment_pam_env_with_systemd_env: systemd returned an empty method call return object!\n");
+        goto dbus_cleanup;
+    }
+
+    /* Make sure we actually got a Variant in reply. If so, recurse into it so
+     * we can look at its contents.
+     */
+    if (dbus_message_iter_get_arg_type(&reply_iter) != DBUS_TYPE_VARIANT) {
+        warnx("augment_pam_env_with_systemd_env: systemd did not return a variant object!\n");
+        goto dbus_cleanup;
+    }
+    dbus_message_iter_recurse(&reply_iter, &reply_inner_iter);
+
+    /* Ensure the returned Variant contains a string array. The type signature
+     * for this data type in D-Bus is "as". If we do have a string array,
+     * recurse into it so we can iterate through it.
+     */
+    inner_iter_typesig = dbus_message_iter_get_signature(&reply_inner_iter);
+    if (strcmp(inner_iter_typesig, "as") != 0) {
+        warnx("augment_pam_env_with_systemd_env: Variant object from systemd does not contain a string array!\n");
+        goto dbus_cleanup;
+    }
+    if (dbus_message_iter_get_arg_type(&reply_inner_iter)
+                != DBUS_TYPE_ARRAY) {
+        warnx("augment_pam_env_with_systemd_env: Variant object from systemd reported itself as a string array, but is not an array!\n");
+        goto dbus_cleanup;
+    }
+    dbus_message_iter_recurse(&reply_inner_iter, &reply_arr_iter);
+
+    /* Walk through the elements of the string array, appending them to our
+     * internal environment array "env_arr".
+     */
+    while ((current_type = dbus_message_iter_get_arg_type(&reply_arr_iter))
+                != DBUS_TYPE_INVALID) {
+        if (current_type != DBUS_TYPE_STRING) {
+            warnx("augment_pam_env_with_systemd_env: Non-string item found in string array!\n");
+            goto dbus_cleanup;
+        }
+        dbus_message_iter_get_basic(&reply_arr_iter, &env_val);
+        env_arr_len++;
+        env_arr = reallocarray(env_arr, env_arr_len, sizeof(char *));
+        if (env_arr == NULL)
+            err(1, "augment_pam_env_with_systemd_env: Failed to allocate memory for environment array");
+        env_arr[env_arr_len - 1] = strdup(env_val);
+        if (env_arr[env_arr_len - 1] == NULL)
+            err(1, "augment_pam_env_with_systemd_env: Failed to allocate memory for environment item");
+
+        dbus_message_iter_next(&reply_arr_iter);
+    }
+
+    /* Merge the environment from systemd with the environment from PAM.
+     * Prefer variables from systemd over variables from PAM.
+     */
+    for (env_idx = 0; env_idx < env_arr_len; env_idx++) {
+        env_eq_ptr = strstr(env_arr[env_idx], "=");
+        if (env_eq_ptr == NULL)
+            errx(1, "augment_pam_env_with_systemd_env: Environment variable without equals sign encountered in systemd environment!\n");
+        env_pre_eq_len = env_eq_ptr - env_arr[env_idx];
+        did_override_env_var = false;
+
+        for (out_env_idx = 0; out_env_arr[out_env_idx] != NULL;
+                    out_env_idx++) {
+            out_env_eq_ptr = strstr(out_env_arr[out_env_idx], "=");
+            if (out_env_eq_ptr == NULL)
+                errx(1, "augment_pam_env_with_systemd_env: Environment variable without equals sign encountered in PAM environment!\n");
+            out_env_pre_eq_len = out_env_eq_ptr - out_env_arr[out_env_idx];
+
+            if (out_env_pre_eq_len != env_pre_eq_len)
+                continue;
+
+            if (strncmp(out_env_arr[out_env_idx], env_arr[env_idx],
+                        env_pre_eq_len) == 0) {
+                /* According to `man pam_getenvlist`, "it is the
+                 * responsibility of the calling application to free() [the
+                 * memory allocated by pam_getenvlist()]". out_env_arr will be
+                 * a char ** created by pam_getenvlist(), thus we can safely
+                 * free items in out_env_arr.
+                 */
+                free(out_env_arr[out_env_idx]);
+
+                /* We intentionally are NOT copying the string here. Every
+                 * item in env_arr will eventually end up in out_env_arr, so
+                 * rather than copying strings from env_arr and then freeing
+                 * env_arr, we simply merge all of the pointers from env_arr
+                 * into out_env_arr, freeing anything in out_env_arr that is
+                 * overridden by something from systemd. This wastes no
+                 * memory, and is quite a bit more efficient.
+                 */
+                out_env_arr[out_env_idx] = env_arr[env_idx];
+
+                /* Signal to the outer loop that we don't need to append an
+                 * item to the environment list.
+                 */
+                did_override_env_var = true;
+                break;
+            }
+        }
+
+        if (!did_override_env_var) {
+            /* Append the variable to the list. */
+            out_env_arr_len++;
+            out_env_arr = reallocarray(out_env_arr, out_env_arr_len,
+                                       sizeof(char *));
+            if (out_env_arr == NULL)
+                err(1, "augment_pam_env_with_systemd_env: Failed to allocate memory for environment item");
+
+            out_env_arr[out_env_arr_len - 1] = NULL;
+            /* See above for rationale behind using assignment rather than
+             * copying here.
+             */
+            out_env_arr[out_env_arr_len - 2] = env_arr[env_idx];
+        }
+    }
+
+dbus_cleanup:
+    /* Don't free the elements of env_arr, they have been placed into
+     * out_env_arr.
+     */
+    if (env_arr != NULL)
+        free(env_arr);
+    if (inner_iter_typesig != NULL)
+        dbus_free(inner_iter_typesig);
+    if (env_reply != NULL)
+        dbus_message_unref(env_reply);
+    if (env_request != NULL)
+        dbus_message_unref(env_request);
+    if (dbus_conn != NULL)
+        dbus_connection_unref(dbus_conn);
+    *out_env_ref = out_env_arr;
+}
 
 #ifdef HAVE_PAM
 static int pam_conv_callback(int num_msg, const struct pam_message **msg,
@@ -79,8 +323,8 @@ static pid_t do_execute(char *user, char *path, char **argv)
     int retval=0, status;
     char **env;
     char env_buf[256];
+    size_t env_idx;
     pam_handle_t *pamh=NULL;
-    pid_t pid;
 
     if (!user)
         goto error;
@@ -223,9 +467,9 @@ static pid_t do_execute(char *user, char *path, char **argv)
     if (retval != PAM_SUCCESS)
         goto error;
 
-    pid = fork();
+    fork_pid = fork();
 
-    switch (pid) {
+    switch (fork_pid) {
         case -1:
             perror("fork xorg");
             goto error;
@@ -241,6 +485,24 @@ static pid_t do_execute(char *user, char *path, char **argv)
             /* This is a copy but don't care to free as we exec later anyway.  */
             env = pam_getenvlist (pamh);
 
+            /* Get the DBUS_SESSION_BUS_ADDRESS from the PAM environment and
+             * place it into our own environment, so that we can talk to
+             * systemd via D-Bus to get environment variables from it.
+             */
+            for (env_idx = 0; env[env_idx] != NULL; env_idx++) {
+                if (strncmp(env[env_idx], "DBUS_SESSION_BUS_ADDRESS=",
+                            strlen("DBUS_SESSION_BUS_ADDRESS=")) == 0) {
+                    putenv(strdup(env[env_idx]));
+                    break;
+                }
+            }
+
+            /* Try to augment the environment list from PAM with the
+             * environment from the systemd user instance for the current
+             * user.
+             */
+            augment_pam_env_with_systemd_env(&env);
+
             /* try to enter home dir, but don't abort if it fails */
             retval = chdir(pw->pw_dir);
             if (retval == -1)
@@ -252,21 +514,19 @@ static pid_t do_execute(char *user, char *path, char **argv)
         default:;
     }
 
-    child_pid = pid;
-
     for (;;) {
-        pid_t wait_pid = waitpid(pid, &status, 0);
+        pid_t wait_pid = waitpid(fork_pid, &status, 0);
         if (wait_pid == (pid_t)-1) {
             if (errno == EINTR)
                 continue;
             perror("waitpid");
             goto error;
         }
-        if (wait_pid == pid)
+        if (wait_pid == fork_pid)
             break;
     }
 
-    child_pid = 0;
+    fork_pid = 0;
 
     retval = pam_close_session(pamh, 0);
     retval = pam_setcred(pamh, PAM_DELETE_CRED | PAM_SILENT);
@@ -332,8 +592,10 @@ static pid_t do_execute(char *user, char *path, char **argv)
 #endif
 
 static void propagate_signal(int signal) {
-    if (child_pid)
-        kill(child_pid, signal);
+    if (!fork_pid) {
+        exit(0);
+    }
+    kill(fork_pid, signal);
 }
 
 static void usage(char *argv0) {

rpm_spec/gui-agent.spec.in

@@ -76,6 +76,7 @@ BuildRequires:	qubes-db-devel
 BuildRequires:	xen-devel
 BuildRequires:	systemd-rpm-macros
 BuildRequires:	libunistring-devel
+BuildRequires:  dbus-devel
 %if 0%{?is_opensuse}
 # for directory ownership
 BuildRequires:  xinit