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

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