Search for qubes.Service+ if call for qubes.Service is made

Previously, qubes.Service+ was searched for if the call was from a VM,
but not if the call was from dom0.  Furthermore, a call from dom0 with
no argument would skip the check for a too-long service name.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089

lib/qubes-rpc-multiplexer

@@ -32,17 +32,20 @@ elif [ "$QREXEC_REQUESTED_TARGET_TYPE" = "keyword" ]; then
 fi
 # else: requested target type unknown or not given, ignore
 export QREXEC_REMOTE_DOMAIN="$2"
-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#*+}"
+if [ "$1" != "${SERVICE_WITHOUT_ARGUMENT}" ]; then
+    export QREXEC_SERVICE_ARGUMENT="${1#*+}"
+    export QREXEC_SERVICE_FULL_NAME="$1"
+else
+    export QREXEC_SERVICE_FULL_NAME="$1+"
+    export QREXEC_SERVICE_ARGUMENT=
 fi
 
 
 ifs=$IFS
 IFS=:
 for DIR in $QREXEC_SERVICE_PATH; do
-    CFG_FILE="$DIR/$1"
+    CFG_FILE="$DIR/$QREXEC_SERVICE_FULL_NAME"
     if [ -s "$CFG_FILE" ]; then
         break
     fi
@@ -53,7 +56,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

libqrexec/exec.c

@@ -282,7 +282,7 @@ static int load_service_config_raw(struct qrexec_parsed_command *cmd,
 
     int ret = find_file(config_path, cmd->service_descriptor,
                         config_full_path, sizeof(config_full_path), NULL);
-    if (ret == -1 && strcmp(cmd->service_descriptor, cmd->service_name) != 0)
+    if (ret == -1 && cmd->service_descriptor != cmd->service_name)
         ret = find_file(config_path, cmd->service_name,
                         config_full_path, sizeof(config_full_path), NULL);
     if (ret < 0)
@@ -361,53 +361,61 @@ struct qrexec_parsed_command *parse_qubes_rpc_command(
             goto err;
         }
 
-        if (end - start > MAX_SERVICE_NAME_LEN) {
-            LOG(ERROR, "Command too long (length %zu)",
-                end - start);
+        if (end <= start) {
+            LOG(ERROR, "Service descriptor is empty (too many spaces after QUBESRPC?)");
             goto err;
         }
 
-        cmd->service_descriptor = strndup(start, end - start);
-        if (!cmd->service_descriptor) {
-            PERROR("strndup");
+        size_t const descriptor_len = (size_t)(end - start);
+        if (descriptor_len > MAX_SERVICE_NAME_LEN) {
+            LOG(ERROR, "Command too long (length %zu)", descriptor_len);
             goto err;
         }
 
         /* Parse service name ("qubes.Service") */
 
-        const char *plus = memchr(start, '+', end - start);
-        if (plus) {
-            if (plus - start == 0) {
-                LOG(ERROR, "Service name empty");
-                goto err;
-            }
-            if (plus - start > NAME_MAX) {
-                LOG(ERROR, "Service name too long to execute (length %zu)",
-                    plus - start);
-                goto err;
-            }
-            cmd->service_name = strndup(start, plus - start);
-            if (!cmd->service_name) {
-                PERROR("strndup");
-                goto err;
-            }
-        } else {
-            cmd->service_name = strndup(start, end - start);
-            if (!cmd->service_name) {
-                PERROR("strdup");
-                goto err;
-            }
+        const char *const plus = memchr(start, '+', descriptor_len);
+        size_t const name_len = plus != NULL ? (size_t)(plus - start) : descriptor_len;
+        if (name_len > NAME_MAX) {
+            LOG(ERROR, "Service name too long to execute (length %zu)", name_len);
+            goto err;
+        }
+        if (name_len < 1) {
+            LOG(ERROR, "Service name empty");
+            goto err;
+        }
+        cmd->service_name = malloc(name_len + 1);
+        if (!cmd->service_name) {
+            PERROR("malloc");
+            goto err;
+        }
+        memcpy(cmd->service_name, start, name_len);
+        cmd->service_name[name_len] = '\0';
+
+        /* If there is no service argument, add a trailing "+" to the descriptor */
+        size_t const descriptor_buffer_size = descriptor_len + 1 + (plus == NULL);
+        cmd->service_descriptor = malloc(descriptor_buffer_size);
+        if (!cmd->service_descriptor) {
+            PERROR("malloc");
+            goto err;
         }
+        memcpy(cmd->service_descriptor, start, descriptor_len);
+        if (plus == NULL)
+            cmd->service_descriptor[descriptor_len] = '+';
+        cmd->service_descriptor[descriptor_buffer_size - 1] = '\0';
 
         /* Parse source domain */
 
         start = end + 1; /* after the space */
         end = strchrnul(start, ' ');
-        cmd->source_domain = strndup(start, end - start);
+        size_t const source_domain_len = (size_t)(end - start);
+        cmd->source_domain = malloc(source_domain_len + 1);
         if (!cmd->source_domain) {
             PERROR("strndup");
             goto err;
         }
+        memcpy(cmd->source_domain, start, source_domain_len);
+        cmd->source_domain[source_domain_len] = '\0';
     }
 
     return cmd;
@@ -480,7 +488,7 @@ static int execute_qrexec_service(
     int ret = find_file(qrexec_service_path, cmd->service_descriptor,
                         service_full_path, sizeof(service_full_path),
                         &statbuf);
-    if (ret == -1 && strcmp(cmd->service_descriptor, cmd->service_name) != 0)
+    if (ret == -1)
         ret = find_file(qrexec_service_path, cmd->service_name,
                         service_full_path, sizeof(service_full_path),
                         &statbuf);

qrexec/tests/socket/agent.py

@@ -584,7 +584,6 @@ def test_exec_broken_specific_service(self):
         )
         self.check_dom0(dom0)
 
-    @unittest.expectedFailure
     def test_exec_null_argument_finds_service_for_empty_argument(self):
         self.make_executable_service(
             "local-rpc",
@@ -616,6 +615,42 @@ def test_exec_null_argument_finds_service_for_empty_argument(self):
         )
         self.check_dom0(dom0)
 
+    def test_socket_null_argument_finds_service_for_empty_argument(self):
+        good_socket_path = os.path.join(
+            self.tempdir, "rpc", "qubes.SocketService+"
+        )
+        bad_socket_path = os.path.join(
+            self.tempdir, "rpc", "qubes.SocketService"
+        )
+        good_server = qrexec.socket_server(good_socket_path)
+        self.addCleanup(good_server.close)
+        bad_server = qrexec.socket_server(bad_socket_path)
+        self.addCleanup(bad_server.close)
+
+        target, dom0 = self.execute_qubesrpc("qubes.SocketService", "domX")
+
+        good_server.accept()
+
+        message = b"stdin data"
+        target.send_message(qrexec.MSG_DATA_STDIN, message)
+        target.send_message(qrexec.MSG_DATA_STDIN, b"")
+        expected = b"qubes.SocketService domX\0" + message
+        self.assertEqual(good_server.recvall(len(expected)), expected)
+
+        good_server.sendall(b"stdout data")
+        good_server.close()
+        messages = target.recv_all_messages()
+        # No stderr
+        self.assertListEqual(
+            util.sort_messages(messages),
+            [
+                (qrexec.MSG_DATA_STDOUT, b"stdout data"),
+                (qrexec.MSG_DATA_STDOUT, b""),
+                (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"),
+            ],
+        )
+        self.check_dom0(dom0)
+
     def test_connect_socket_no_metadata(self):
         socket_path = os.path.join(
             self.tempdir, "rpc", "qubes.SocketService+arg2"