Various bugfixes

daemon/qrexec-client.c

@@ -211,36 +211,46 @@
 
 
 /* See also qrexec-agent.c:wait_for_session_maybe() */
-static void wait_for_session_maybe(char *cmdline)
+static bool wait_for_session_maybe(char *cmdline)
 {
     struct qrexec_parsed_command *cmd;
     pid_t pid;
     int status;
+    bool rc = false;
 
     cmd = parse_qubes_rpc_command(cmdline, false);
     if (!cmd)
         goto out;
 
-    if (cmd->nogui)
+    if (cmd->nogui) {
+        rc = true;
         goto out;
+    }
 
-    if (!cmd->service_descriptor)
+    if (!cmd->service_descriptor) {
+        rc = true;
         goto out;
+    }
 
-    load_service_config_v2(cmd);
-    if (!cmd->wait_for_session)
+    if (load_service_config_v2(cmd) < 0)
         goto out;
+    if (!cmd->wait_for_session) {
+        rc = true;
+        goto out;
+    }
 
     pid = fork();
     switch (pid) {
         case 0:
             close(0);
             exec_wait_for_session(cmd->source_domain);
             PERROR("exec");
-            exit(1);
+            _exit(1);
         case -1:
             PERROR("fork");
             goto out;
+        default:
+            rc = true;
     }
 
     if (waitpid(local_pid, &status, 0) > 0) {
@@ -251,6 +261,7 @@
 
 out:
     destroy_qrexec_parsed_command(cmd);
+    return rc;
 }
 
 static int prepare_local_fds(char *cmdline, struct buffer *stdin_buffer)
@@ -617,8 +628,12 @@
 
         struct buffer stdin_buffer;
         buffer_init(&stdin_buffer);
-        wait_for_session_maybe(remote_cmdline);
-        prepare_ret = prepare_local_fds(remote_cmdline, &stdin_buffer);
+        if (!wait_for_session_maybe(remote_cmdline)) {
+            LOG(ERROR, "Cannot load service configuration, or forking process failed");
+            prepare_ret = -1;
+        } else {
+            prepare_ret = prepare_local_fds(remote_cmdline, &stdin_buffer);
+        }
         if (request_id) {
             void (*old_handler)(int);
 

daemon/qrexec-daemon.c

@@ -1262,6 +1262,21 @@ static bool validate_request_id(struct service_params *untrusted_params, const c
 
 #define ENSURE_NULL_TERMINATED(x) x[sizeof(x)-1] = 0
 
+static bool validate_service_name(char *untrusted_service_name)
+{
+    switch (untrusted_service_name[0]) {
+    case '\0':
+        LOG(ERROR, "Empty service name not allowed");
+        return false;
+    case '+':
+        LOG(ERROR, "Service name must not start with '+'");
+        return false;
+    default:
+        sanitize_name(untrusted_service_name, "+");
+        return true;
+    }
+}
+
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
 static
 #endif
@@ -1296,6 +1311,10 @@ void handle_message_from_agent(void)
                 send_service_refused(vchan, &untrusted_params.request_id);
                 return;
             }
+            if (!validate_service_name(untrusted_params.service_name)) {
+                send_service_refused(vchan, &untrusted_params.request_id);
+                return;
+            }
             params = untrusted_params;
             /* sanitize end */
 
@@ -1329,38 +1348,30 @@ void handle_message_from_agent(void)
             ENSURE_NULL_TERMINATED(untrusted_params3.target_domain);
             sanitize_name(untrusted_params3.target_domain, "@:");
             if (!validate_request_id(&untrusted_params3.request_id, "MSG_TRIGGER_SERVICE3"))
-                goto fail;
+                goto fail3;
             params3 = untrusted_params3;
             if (untrusted_service_name[service_name_len] != 0) {
                 LOG(ERROR, "Service name not NUL-terminated");
-                goto fail;
+                goto fail3;
             }
             nul_offset = strlen(untrusted_service_name);
             if (nul_offset != service_name_len) {
                 LOG(ERROR, "Service name contains NUL byte at offset %zu", nul_offset);
-                goto fail;
+                goto fail3;
             }
-            switch (untrusted_service_name[0]) {
-            case '\0':
-                LOG(ERROR, "Empty service name not allowed");
-                goto fail;
-            case '+':
-                LOG(ERROR, "Service name must not start with '+'");
-                goto fail;
-            default:
-                sanitize_name(untrusted_service_name, "+");
-                service_name = untrusted_service_name;
-                untrusted_service_name = NULL;
-                /* sanitize end */
+            if (!validate_service_name(untrusted_service_name))
+                goto fail3;
+            service_name = untrusted_service_name;
+            untrusted_service_name = NULL;
+            /* sanitize end */
 
-                handle_execute_service(remote_domain_id, remote_domain_name,
-                        params3.target_domain,
-                        service_name,
-                        &params3.request_id);
-                free(service_name);
-                return;
-            }
-fail:
+            handle_execute_service(remote_domain_id, remote_domain_name,
+                    params3.target_domain,
+                    service_name,
+                    &params3.request_id);
+            free(service_name);
+            return;
+fail3:
             send_service_refused(vchan, &untrusted_params3.request_id);
             free(untrusted_service_name);
             return;

lib/qubes-rpc-multiplexer

@@ -1,27 +1,32 @@
 #!/bin/sh -l
+# we don't use globbing, disable it
+set -f
 
-if [ -z "$QREXEC_SERVICE_PATH" ]; then
+if [ -z "${QREXEC_SERVICE_PATH+x}" ]; then
     QREXEC_SERVICE_PATH=/usr/local/etc/qubes-rpc:/etc/qubes-rpc
 fi
+tmpdir=${XDG_RUNTIME_DIR-/tmp}
 
 # write stderr to both calling party and local log; be very careful about
 # closing file descriptors here - if either stdout or stderr will not be closed
 # when service process does the same - service call will hang (waiting for EOF
 # on stdout/stderr)
-stderr_pipe=/tmp/qrexec-rpc-stderr.$$
-mkfifo $stderr_pipe
+stderr_pipe=$tmpdir/qrexec-rpc-stderr.$$
+mkfifo -- "$stderr_pipe"
 # tee can't write to file descriptor, nor /proc/self/fd/2 (EXIO on open)
-return_stderr_pipe=/tmp/qrexec-rpc-stderr-return.$$
-mkfifo $return_stderr_pipe
-{ cat <$return_stderr_pipe >&2 2>/dev/null; rm -f $return_stderr_pipe; } </dev/null >/dev/null &
-{ tee $return_stderr_pipe <$stderr_pipe |\
-       logger -t "$1-$2"; rm -f $stderr_pipe; } </dev/null >/dev/null 2>&1 &
-exec 2>$stderr_pipe
+return_stderr_pipe=$tmpdir/qrexec-rpc-stderr-return.$$
+mkfifo -- "$return_stderr_pipe"
+{ cat <"$return_stderr_pipe" >&2 2>/dev/null; rm -f -- "$return_stderr_pipe"; } </dev/null >/dev/null &
+{ tee -- "$return_stderr_pipe" <"$stderr_pipe" |
+       logger -t "$1-$2"; rm -f -- "$stderr_pipe"; } </dev/null >/dev/null 2>&1 &
+exec 2>"$stderr_pipe"
 
 if ! [ $# = 2 -o $# = 4 ] ; then
 	echo "$0: bad argument count, usage: $0 SERVICE-NAME REMOTE-DOMAIN-NAME [REQUESTED_TARGET_TYPE REQUESTED_TARGET]" >&2
 	exit 1
 fi
+# Avoid inheriting these from the environment
+unset QREXEC_REQUESTED_TARGET QREXEC_REQUESTED_TARGET_KEYWORD QREXEC_SERVICE_ARGUMENT
 export QREXEC_REQUESTED_TARGET_TYPE="$3"
 if [ "$QREXEC_REQUESTED_TARGET_TYPE" = "name" ]; then
     export QREXEC_REQUESTED_TARGET="$4"
@@ -34,6 +39,9 @@ export QREXEC_SERVICE_FULL_NAME="$1"
 SERVICE_WITHOUT_ARGUMENT="${1%%+*}"
 if [ "${QREXEC_SERVICE_FULL_NAME}" != "${SERVICE_WITHOUT_ARGUMENT}" ]; then
     export QREXEC_SERVICE_ARGUMENT="${QREXEC_SERVICE_FULL_NAME#*+}"
+else
+    # Search for qubes.Service+ if given qubes.Service
+    set -- "$1+" "$2"
 fi
 
 
@@ -51,7 +59,6 @@ for DIR in $QREXEC_SERVICE_PATH; do
 done
 IFS=$ifs
 
-# shellcheck disable=SC2086
-exec "$CFG_FILE" ${QREXEC_SERVICE_ARGUMENT}
+exec "$CFG_FILE" ${QREXEC_SERVICE_ARGUMENT:+"$QREXEC_SERVICE_ARGUMENT"}
 echo "$0: failed to execute handler for $1" >&2
 exit 1