Strip "QUBESRPC " prefix from service call commands

DemiMarie · DemiMarie · commit e0407d8f39bc · 2024-05-09T19:29:22.000-04:00
It carries no information, and various parts of the code must strip it.
Just omit it from the command entirely.  Whether a command is an RPC
command should be determined by the service descriptor being non-NULL.

Review with "git diff --ignore-space-change".
diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c
@@ -172,7 +172,8 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
             exit(1);
         }
         /* call QUBESRPC if requested */
-        exec_qubes_rpc_if_requested(prog, cmd, environ);
+        if (prog)
+            exec_qubes_rpc(prog, cmd, environ);
 
         /* otherwise exec shell */
         execl("/bin/sh", "sh", "-c", cmd, NULL);
@@ -279,7 +280,8 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
                 warn("chdir(%s)", pw->pw_dir);
 
             /* call QUBESRPC if requested */
-            exec_qubes_rpc_if_requested(prog, cmd, env);
+            if (prog)
+                exec_qubes_rpc(prog, cmd, env);
 
             /* otherwise exec shell */
             execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env);
@@ -317,7 +319,8 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
     exit(1);
 #else
     /* call QUBESRPC if requested */
-    exec_qubes_rpc_if_requested(prog, cmd, environ);
+    if (prog)
+        exec_qubes_rpc(prog, cmd, environ);
 
     /* otherwise exec shell */
     execl("/bin/su", "su", "-", user, "-c", cmd, NULL);
diff --git a/agent/qrexec-fork-server.c b/agent/qrexec-fork-server.c
@@ -45,7 +45,8 @@ void do_exec(const char *prog, const char *cmd, const char *user __attribute__((
     signal(SIGPIPE, SIG_DFL);
 
     /* call QUBESRPC if requested */
-    exec_qubes_rpc_if_requested(prog, cmd, environ);
+    if (prog != NULL)
+        exec_qubes_rpc(prog, cmd, environ);
 
     /* otherwise, pass it to shell */
     shell = getenv("SHELL");
diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c
@@ -66,10 +66,11 @@ static _Noreturn void do_exec(const char *prog,
                               const char *cmdline,
                               const char *username __attribute__((unused)))
 {
-    /* avoid calling RPC command through shell */
-    exec_qubes_rpc_if_requested(prog, cmdline, environ);
+    /* avoid calling RPC service through shell */
+    if (prog)
+        exec_qubes_rpc(prog, cmdline, environ);
 
-    /* if above haven't executed RPC command, pass it to shell */
+    /* if above haven't executed RPC service, pass it to shell */
     execl("/bin/bash", "bash", "-c", cmdline, NULL);
     PERROR("exec bash");
     exit(1);
diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c
@@ -1106,7 +1106,8 @@ static enum policy_response connect_daemon_socket(
 static _Noreturn void do_exec(const char *prog, const char *cmd, const char *username __attribute__((unused)))
 {
     /* avoid calling RPC command through shell */
-    exec_qubes_rpc_if_requested(prog, cmd, environ);
+    if (prog)
+        exec_qubes_rpc(prog, cmd, environ);
 
     /* if above haven't executed RPC command, pass it to shell */
     execl("/bin/bash", "bash", "-c", cmd, NULL);
diff --git a/libqrexec/exec.c b/libqrexec/exec.c
@@ -36,6 +36,7 @@
 #include <netdb.h>
 #include <unistd.h>
 #include <fcntl.h>
+
 #include "qrexec.h"
 #include "libqrexec-utils.h"
 #include "private.h"
@@ -54,110 +55,98 @@ static bool should_strip_env_var(const char *var)
     return strncmp(var + (sizeof "QREXEC" - 1), "_SERVICE_PATH=", sizeof "_SERVICE_PATH") != 0;
 }
 
-void exec_qubes_rpc_if_requested(const char *program, const char *cmd, char *const envp[]) {
+void exec_qubes_rpc(const char *program, const char *cmd, char *const envp[]) {
     /* avoid calling RPC service through shell */
-    if (program) {
-        assert(program);
-        char *prog_copy;
-        char *tok, *savetok;
-        const char *argv[6];
-        size_t i = 0;
+    char *prog_copy;
+    char *tok, *savetok;
+    const char *argv[6];
+    size_t i = 0;
 #define MAX_ADDED_ENV_VARS 5
-        size_t const extra_env_vars = MAX_ADDED_ENV_VARS;
-        size_t env_amount = extra_env_vars;
-
-        if (strncmp(cmd, RPC_REQUEST_COMMAND " ", RPC_REQUEST_COMMAND_LEN + 1) != 0) {
-            LOG(ERROR, "program != NULL, but '%s' does not start with '%s '",
-                cmd, RPC_REQUEST_COMMAND " ");
-            assert(!"Invalid command");
-            _exit(QREXEC_EXIT_PROBLEM);
-        }
-
-        for (char *const *env = envp; *env; ++env) {
-            // Set this 0 to 1 if adding new variable settings below,
-            // to ensure that MAX_ADDED_ENV_VARS is correct.
-            if (0 && should_strip_env_var(*env))
-                continue;
-            env_amount++;
-        }
+    size_t const extra_env_vars = MAX_ADDED_ENV_VARS;
+    size_t env_amount = extra_env_vars;
+
+    for (char *const *env = envp; *env; ++env) {
+        // Set this 0 to 1 if adding new variable settings below,
+        // to ensure that MAX_ADDED_ENV_VARS is correct.
+        if (0 && should_strip_env_var(*env))
+            continue;
+        env_amount++;
+    }
 #define EXTEND(...)                                             \
-        do {                                                    \
-            if (iterator >= env_amount)                         \
-                abort();                                        \
-            if (asprintf(&buf[iterator++], __VA_ARGS__) < 0)    \
-                goto bad_asprintf;                              \
-        } while (0)
+    do {                                                    \
+        if (iterator >= env_amount)                         \
+        abort();                                        \
+        if (asprintf(&buf[iterator++], __VA_ARGS__) < 0)    \
+        goto bad_asprintf;                              \
+    } while (0)
 #define EXTEND_RAW(arg)                 \
-        do {                            \
-            if (iterator >= env_amount) \
-                abort();                \
-            buf[iterator++] = (arg);    \
-        } while (0)
+    do {                            \
+        if (iterator >= env_amount) \
+        abort();                \
+        buf[iterator++] = (arg);    \
+    } while (0)
 
-        char **buf = calloc(env_amount + 1, sizeof(char *));
-        if (buf == NULL) {
-            LOG(ERROR, "calloc(%zu, %zu) failed: %m", env_amount, sizeof(char *));
-            _exit(QREXEC_EXIT_PROBLEM);
-        }
-        size_t iterator = 0;
-        for (char *const *env = envp; *env; ++env) {
-            if (!should_strip_env_var(*env)) {
-                EXTEND_RAW(*env);
-            }
+    char **buf = calloc(env_amount + 1, sizeof(char *));
+    if (buf == NULL) {
+        LOG(ERROR, "calloc(%zu, %zu) failed: %m", env_amount, sizeof(char *));
+        _exit(QREXEC_EXIT_PROBLEM);
+    }
+    size_t iterator = 0;
+    for (char *const *env = envp; *env; ++env) {
+        if (!should_strip_env_var(*env)) {
+            EXTEND_RAW(*env);
         }
+    }
 
-        prog_copy = strdup(cmd + RPC_REQUEST_COMMAND_LEN + 1);
-        if (!prog_copy) {
-            PERROR("strdup");
-            _exit(QREXEC_EXIT_PROBLEM);
-        }
+    prog_copy = strdup(cmd);
+    if (!prog_copy) {
+        PERROR("strdup");
+        _exit(QREXEC_EXIT_PROBLEM);
+    }
 
-        argv[i++] = (char *)program;
-        tok=strtok_r(prog_copy, " ", &savetok);
-        while (tok != NULL) {
-            if (i >= sizeof(argv)/sizeof(argv[0])-1) {
-                LOG(ERROR, "Too many arguments to %s", RPC_REQUEST_COMMAND);
-                _exit(QREXEC_EXIT_PROBLEM);
-            }
-            argv[i++] = tok;
-            tok = strtok_r(NULL, " ", &savetok);
-        }
-        argv[i] = NULL;
-        if (i == 5) {
-            EXTEND("QREXEC_REQUESTED_TARGET_TYPE=%s", argv[3]);
-            if (strcmp(argv[3], "name") == 0) {
-                EXTEND("QREXEC_REQUESTED_TARGET=%s", argv[4]);
-            } else if (strcmp(argv[3], "keyword") == 0) {
-                EXTEND("QREXEC_REQUESTED_TARGET_KEYWORD=%s", argv[4]);
-            } else {
-                // requested target type unknown, ignore
-            }
-        } else if (i == 3) {
-            EXTEND_RAW("QREXEC_REQUESTED_TARGET_TYPE=");
-        } else {
-            LOG(ERROR, "invalid number of arguments: %zu", i);
+    argv[i++] = (char *)program;
+    tok=strtok_r(prog_copy, " ", &savetok);
+    while (tok != NULL) {
+        if (i >= sizeof(argv)/sizeof(argv[0])-1) {
+            LOG(ERROR, "Too many arguments to %s", RPC_REQUEST_COMMAND);
             _exit(QREXEC_EXIT_PROBLEM);
         }
-        EXTEND("QREXEC_SERVICE_FULL_NAME=%s", argv[1]);
-        EXTEND("QREXEC_REMOTE_DOMAIN=%s", argv[2]);
-        const char *p = strchr(argv[1], '+');
-        argv[1] = NULL;
-        argv[2] = NULL;
-        if (p != NULL) {
-            EXTEND("QREXEC_SERVICE_ARGUMENT=%s", p + 1);
-            if (p[1])
-                argv[1] = p + 1;
+        argv[i++] = tok;
+        tok = strtok_r(NULL, " ", &savetok);
+    }
+    argv[i] = NULL;
+    if (i == 5) {
+        EXTEND("QREXEC_REQUESTED_TARGET_TYPE=%s", argv[3]);
+        if (strcmp(argv[3], "name") == 0) {
+            EXTEND("QREXEC_REQUESTED_TARGET=%s", argv[4]);
+        } else if (strcmp(argv[3], "keyword") == 0) {
+            EXTEND("QREXEC_REQUESTED_TARGET_KEYWORD=%s", argv[4]);
+        } else {
+            // requested target type unknown or not given, ignore
         }
-        assert(iterator <= env_amount);
-        buf[iterator] = NULL;
-        execve(argv[0], (char *const *)argv, buf);
-        _exit(errno == ENOENT ? QREXEC_EXIT_SERVICE_NOT_FOUND : QREXEC_EXIT_PROBLEM);
-bad_asprintf:
-        PERROR("asprintf");
-        _exit(QREXEC_EXIT_PROBLEM);
+    } else if (i == 3) {
+        EXTEND_RAW("QREXEC_REQUESTED_TARGET_TYPE=");
     } else {
-        assert(strncmp(cmd, RPC_REQUEST_COMMAND, RPC_REQUEST_COMMAND_LEN) != 0);
+        LOG(ERROR, "invalid number of arguments: %zu", i);
+        _exit(QREXEC_EXIT_PROBLEM);
     }
+    EXTEND("QREXEC_SERVICE_FULL_NAME=%s", argv[1]);
+    EXTEND("QREXEC_REMOTE_DOMAIN=%s", argv[2]);
+    const char *p = strchr(argv[1], '+');
+    argv[1] = NULL;
+    argv[2] = NULL;
+    if (p != NULL) {
+        EXTEND("QREXEC_SERVICE_ARGUMENT=%s", p + 1);
+        if (p[1])
+            argv[1] = p + 1;
+    }
+    assert(iterator <= env_amount);
+    buf[iterator] = NULL;
+    execve(argv[0], (char *const *)argv, buf);
+    _exit(errno == ENOENT ? QREXEC_EXIT_SERVICE_NOT_FOUND : QREXEC_EXIT_PROBLEM);
+bad_asprintf:
+    PERROR("asprintf");
+    _exit(QREXEC_EXIT_PROBLEM);
 }
 
 void fix_fds(int fdin, int fdout, int fderr)
@@ -493,7 +482,9 @@ struct qrexec_parsed_command *parse_qubes_rpc_command(
         }
 
         /* Parse service descriptor ("qubes.Service+arg") */
-        start = cmd->command + RPC_REQUEST_COMMAND_LEN + 1;
+        cmd->command += RPC_REQUEST_COMMAND_LEN + 1;
+
+        start = cmd->command;
         end = strchr(start, ' ');
         if (!end) {
             LOG(ERROR, "No space found after service descriptor");
@@ -608,7 +599,7 @@ int execute_parsed_qubes_rpc_command(
             return 0;
         }
         return do_fork_exec(buf.data, cmd->username, cmd->command,
-                           pid, stdin_fd, stdout_fd, stderr_fd);
+                            pid, stdin_fd, stdout_fd, stderr_fd);
     } else {
         // Legacy qrexec behavior: spawn shell directly
         return do_fork_exec(NULL, cmd->username, cmd->command,
@@ -732,7 +723,7 @@ int find_qrexec_service(
 
         if (cmd->send_service_descriptor) {
             /* send part after "QUBESRPC ", including trailing NUL */
-            const char *desc = cmd->command + RPC_REQUEST_COMMAND_LEN + 1;
+            const char *desc = cmd->command;
             buffer_append(stdin_buffer, desc, strlen(desc) + 1);
         }
 
@@ -792,7 +783,7 @@ int find_qrexec_service(
 
         if (cmd->send_service_descriptor) {
             /* send part after "QUBESRPC ", including trailing NUL */
-            const char *desc = cmd->command + RPC_REQUEST_COMMAND_LEN + 1;
+            const char *desc = cmd->command;
             buffer_append(stdin_buffer, desc, strlen(desc) + 1);
         }
 
diff --git a/libqrexec/libqrexec-utils.h b/libqrexec/libqrexec-utils.h
@@ -62,7 +62,8 @@ struct qrexec_parsed_command {
     /* Override to disable "wait for session" */
     bool nogui;
 
-    /* Command (the part after "user:") */
+    /* Command (the part after "user:").  If this is an RPC command
+     * then the "QUBESRPC " prefix is not included. */
     const char *command;
 
     /* The below parameters are NULL for legacy (non-"QUBESRPC") commands. */
@@ -145,7 +146,7 @@ void register_exec_func(do_exec_t *func);
  * returns without doing anything.
  */
 __attribute__((visibility("default")))
-void exec_qubes_rpc_if_requested(const char *program, const char *cmd, char *const envp[]);
+_Noreturn void exec_qubes_rpc(const char *program, const char *cmd, char *const envp[]);
 
 __attribute__((visibility("default")))
 int exec_wait_for_session(const char *source_domain);

Original file line number	Diff line number	Diff line change
`@@ -1106,7 +1106,8 @@ static enum policy_response connect_daemon_socket(`
`1106`	`1106`	`static _Noreturn void do_exec(const char prog, const char cmd, const char *username __attribute__((unused)))`
`1107`	`1107`	`{`
`1108`	`1108`	`/* avoid calling RPC command through shell */`
`1109`		`- exec_qubes_rpc_if_requested(prog, cmd, environ);`
	`1109`	`+ if (prog)`
	`1110`	`+ exec_qubes_rpc(prog, cmd, environ);`
`1110`	`1111`
`1111`	`1112`	`/* if above haven't executed RPC command, pass it to shell */`
`1112`	`1113`	`execl("/bin/bash", "bash", "-c", cmd, NULL);`