Use a separate program to exec qrexec services

This makes executing a program via the shell consistent with doing so
_not_ via the shell: both handle the environment the same way, and both
produce the same error codes if the program is bad.  This also
significantly simplifiex exec_qubes_rpc2(), which doesn't need to handle
creating a shell command anymore.  It is assumed that the user's startup
files do not modify the positional parameters outside of a function.
Doing so would be extremely buggy and break the R4.2 multiplexer.  The
new code does use spaces in shell script arguments, but if the user's
startup files break when that happens, the user has other problems.

This is an ABI break for libqrexec, but that's okay in an unstable
release.  In the future, libqrexec should really be a static library.

Fixes: 8728002 (Stop using qubes-rpc-multiplexer for service calls)

.gitignore

@@ -8,6 +8,7 @@ build/
 /agent/qrexec-fork-server
 /agent/qrexec-client-vm
 /agent/qrexec-agent
+/agent/qrexec-exec-program
 /daemon/qrexec-client
 /daemon/qrexec-daemon
 /agent/qrexec-client-vm.1.gz

agent/Makefile

@@ -19,7 +19,7 @@ LDLIBS += -lqrexec-utils $(shell pkg-config --libs $(VCHAN_PKG)) $(if $(HAVE_PAM
 remove_generated := \
 	rm -f -- *.o *~ qrexec-agent qrexec-client-vm *.o.dep *.gcda *.gcno
 
-all: qrexec-agent qrexec-client-vm qrexec-fork-server qrexec-client-vm.1.gz
+all: qrexec-agent qrexec-client-vm qrexec-fork-server qrexec-client-vm.1.gz qrexec-exec-program
 .PHONY: all clean install .PHONY
 qrexec-agent: qrexec-agent.o qrexec-agent-data.o
 qrexec-fork-server: qrexec-fork-server.o qrexec-agent-data.o
@@ -35,7 +35,7 @@ endif
 install: all
 	install -d $(DESTDIR)/etc/qubes-rpc $(DESTDIR)/usr/lib/qubes \
 		$(DESTDIR)/usr/bin $(DESTDIR)/usr/share/man/man1
-	install qrexec-agent $(DESTDIR)/usr/lib/qubes
+	install qrexec-agent qrexec-exec-program $(DESTDIR)/usr/lib/qubes
 	install qrexec-client-vm $(DESTDIR)/usr/bin
 	install -d $(DESTDIR)/usr/share/man/man1
 	install qrexec-client-vm.1.gz $(DESTDIR)/usr/share/man/man1

agent/qrexec-agent.c

@@ -45,6 +45,7 @@
 #include <libvchan.h>
 #include "libqrexec-utils.h"
 #include "qrexec-agent.h"
+#define QREXEC_EXEC_PROGRAM_PATH "/usr/lib/qubes/qrexec-exec-program"
 
 struct connection_info {
     int pid; /* pid of child process handling the data */
@@ -53,6 +54,9 @@ struct connection_info {
     int connect_port;
 };
 
+const char *service_path;
+const char *exec_program = QREXEC_EXEC_PROGRAM_PATH;
+
 /* structure describing a single request waiting for qubes.WaitForSession to
  * finish */
 struct waiting_request {
@@ -124,8 +128,15 @@ static struct pam_conv conv = {
 #endif
 
 _Noreturn static void really_exec(const char *prog, const struct passwd *pw,
-                                  char **env, const char *cmd)
+                                  char **env, const char *cmd, pid_t agent_pid)
 {
+    /* 1 means init, which is completely wrong. */
+    if (agent_pid < 2) {
+        LOG(ERROR, "Bad agent_pid value (did parent process die?): %ju",
+            (uintmax_t)agent_pid);
+        _exit(QREXEC_EXIT_PROBLEM);
+    }
+
     /* child */
     setsid();
 
@@ -137,7 +148,16 @@ _Noreturn static void really_exec(const char *prog, const struct passwd *pw,
     /* call QUBESRPC if requested */
     if (prog) {
         /* Set up environment variables for a login shell. */
-        exec_qubes_rpc2(prog, cmd, env, true);
+        char *pid_str;
+        if (asprintf(&pid_str, "%ju", (uintmax_t)agent_pid) < 1) {
+            PERROR("asprintf");
+            _exit(QREXEC_EXIT_PROBLEM);
+        }
+        const char *buf[] = {"sh", "-lc", "exec \"$@\"", "sh", exec_program, pid_str, prog, cmd, service_path, NULL};
+        /* TODO: use the user's shell not /bin/sh */
+        execve("/bin/sh", (char * const *)buf, env);
+        PERROR("execve /bin/sh");
+        _exit(QREXEC_EXIT_PROBLEM);
     }
     /* otherwise exec shell */
     char *shell_dup = strdup(pw->pw_shell);
@@ -215,6 +235,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
     char env_buf[64];
 #endif
     sigset_t sigmask;
+    pid_t ppid = getppid();
 
     sigemptyset(&sigmask);
     sigprocmask(SIG_SETMASK, &sigmask, NULL);
@@ -249,7 +270,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
             case -1:
                 goto error;
             case 0:
-                really_exec(prog, pw, environ, cmd);
+                really_exec(prog, pw, environ, cmd, ppid);
             default:
                 /* parent */
                 close_std(null_fd);
@@ -303,14 +324,6 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
     if (retval != PAM_SUCCESS)
         goto error;
 
-    /* provide this variable to child process */
-    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "QREXEC_AGENT_PID=%d", getppid()) >= sizeof(env_buf)) {
-        retval = PAM_ABORT;
-        goto error;
-    }
-    retval = pam_putenv(pamh, env_buf);
-    if (retval != PAM_SUCCESS)
-        goto error;
     if ((unsigned)snprintf(env_buf, sizeof(env_buf), "HOME=%s", pw->pw_dir) >= sizeof(env_buf)) {
         retval = PAM_ABORT;
         goto error;
@@ -362,7 +375,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
             /* This is a copy but don't care to free as we exec later anyway.  */
             env = pam_getenvlist (pamh);
 
-            really_exec(prog, pw, env, cmd);
+            really_exec(prog, pw, env, cmd, ppid);
         default:
             /* parent */
             close_std(null_fd);
@@ -923,9 +936,9 @@ static struct option longopts[] = {
     { "agent-socket", required_argument, 0, 'a' },
     { "fork-server-socket", required_argument, 0, 's' },
     { "no-fork-server", no_argument, 0, 'S' },
+    { "exec-program", required_argument, 0, 'p' },
     { NULL, 0, 0, 0 },
 };
-
 static _Noreturn void usage(const char *argv0)
 {
     fprintf(stderr, "usage: %s [options]\n", argv0);
@@ -937,13 +950,16 @@ static _Noreturn void usage(const char *argv0)
             QREXEC_FORK_SERVER_SOCKET);
     fprintf(stderr, "    (set empty to disable, use %%s as username)\n");
     fprintf(stderr, "  --no-fork-server - don't try to connect to fork server\n");
+    fprintf(stderr, "  --exec-program-path=PATH - where to find qrexec-exec-program, for tests, default: %s\n",
+            QREXEC_EXEC_PROGRAM_PATH);
     exit(2);
 }
 
 int main(int argc, char **argv)
 {
     sigset_t selectmask;
 
+    service_path = getenv("QREXEC_SERVICE_PATH");
     setup_logging("qrexec-agent");
 
     int opt;
@@ -961,6 +977,11 @@ int main(int argc, char **argv)
             case 'S':
                 fork_server_path = NULL;
                 break;
+            case 'p':
+                exec_program = optarg;
+                if (exec_program[0] != '/')
+                    errx(2, "%s is not an absolute path", exec_program);
+                break;
             case 'h':
             case '?':
                 usage(argv[0]);

agent/qrexec-exec-program.c

@@ -0,0 +1,23 @@
+#include <stdlib.h>
+#include <err.h>
+#include <libqrexec-utils.h>
+int main(int argc, char **argv)
+{
+    extern char **environ;
+    if (argc == 5) {
+        if (setenv("QREXEC_SERVICE_PATH", argv[4], 1)) {
+            err(QREXEC_EXIT_PROBLEM, "setenv");
+        }
+    } else if (argc == 4) {
+        if (unsetenv("QREXEC_SERVICE_PATH")) {
+            err(QREXEC_EXIT_PROBLEM, "setenv");
+        }
+    } else {
+        errx(QREXEC_EXIT_PROBLEM, "usage: qrexec-run AGENT_PID PROGRAM COMMAND [SERVICE_PATH]");
+    }
+    if (setenv("QREXEC_AGENT_PID", argv[1], 1)) {
+        err(QREXEC_EXIT_PROBLEM, "setenv");
+    }
+    exec_qubes_rpc2(argv[2], argv[3], environ);
+    /* does not return */
+}

agent/qrexec-fork-server.c

@@ -47,7 +47,7 @@ void do_exec(const char *prog, const char *cmd, const char *user __attribute__((
     /* call QUBESRPC if requested */
     if (prog != NULL) {
         /* Already in login session. */
-        exec_qubes_rpc2(prog, cmd, environ, false);
+        exec_qubes_rpc2(prog, cmd, environ);
     }
 
     /* otherwise, pass it to shell */

daemon/qrexec-client.c

@@ -69,7 +69,7 @@ static _Noreturn void do_exec(const char *prog,
     /* avoid calling RPC service through shell */
     if (prog) {
         /* qrexec-client is always in a login session. */
-        exec_qubes_rpc2(prog, cmdline, environ, false);
+        exec_qubes_rpc2(prog, cmdline, environ);
     }
 
     /* if above haven't executed RPC service, pass it to shell */
@@ -328,7 +328,7 @@ int main(int argc, char **argv)
                 assert(command->username == NULL);
                 assert(command->command);
                 /* qrexec-client is always in a login session. */
-                exec_qubes_rpc2(buf.data, command->command, environ, false);
+                exec_qubes_rpc2(buf.data, command->command, environ);
                 /* not reached */
             default:
                 assert(false);

daemon/qrexec-daemon.c

@@ -1136,7 +1136,7 @@ static _Noreturn void do_exec(const char *prog, const char *cmd, const char *use
     /* avoid calling RPC command through shell */
     if (prog) {
         /* qrexec-daemon is always in a login session already */
-        exec_qubes_rpc2(prog, cmd, environ, false);
+        exec_qubes_rpc2(prog, cmd, environ);
     }
 
     /* if above haven't executed RPC command, pass it to shell */

debian/control

@@ -19,7 +19,7 @@ Homepage: https://www.qubes-os.org
 Package: qubes-core-qrexec
 Architecture: any
 Depends:
- libqrexec-utils4 (= ${binary:Version}),
+ libqrexec-utils5 (= ${binary:Version}),
  python3-qrexec,
  ${shlibs:Depends},
  ${misc:Depends}
@@ -36,7 +36,7 @@ Description: Qubes qrexec agent
  Agent part of Qubes RPC system. A daemon responsible for starting processes as
  requested by dom0 or other VMs, according to dom0-enforced policy.
 
-Package: libqrexec-utils4
+Package: libqrexec-utils5
 Architecture: any
 Depends: ${shlibs:Depends}, ${misc:Depends}
 Breaks: qubes-utils (<< 3.1.4)
@@ -47,7 +47,7 @@ Description: Library of common functions of qrexec agent and daemon
 Package: libqrexec-utils-dev
 Architecture: any
 Section: libdevel
-Depends: libqrexec-utils4 (= ${binary:Version}), ${misc:Depends}
+Depends: libqrexec-utils5 (= ${binary:Version}), ${misc:Depends}
 Breaks: qubes-utils (<< 3.1.4)
 Replaces: qubes-utils (<< 3.1.4)
 Description: Development headers for libqrexec-utils

debian/libqrexec-utils5.install

@@ -0,0 +1 @@
+usr/lib/libqrexec-utils.so.5*

debian/libqrexec-utils5.shlibs

@@ -0,0 +1 @@
+libqrexec-utils 5 libqrexec-utils5 (>4.3.3)

debian/qubes-core-qrexec.install

@@ -18,6 +18,7 @@ usr/bin/qubes-policy-admin
 usr/bin/qubes-policy-editor
 usr/bin/qubes-policy-lint
 usr/lib/qubes/qrexec-agent
+usr/lib/qubes/qrexec-exec-program
 usr/lib/qubes/qrexec-client-vm
 usr/lib/qubes/qrexec_client_vm
 usr/lib/qubes/qubes-rpc-multiplexer

libqrexec/Makefile

@@ -10,7 +10,7 @@ CFLAGS += -I. -I../libqrexec -g -O2 -Wall -Wextra -Werror \
 
 LDFLAGS += -pie -Wl,-z,relro,-z,now -shared
 
-SO_VER=4
+SO_VER=5
 VCHANLIBS := $(shell pkg-config --libs vchan)
 LIBDIR ?= /usr/lib
 INCLUDEDIR ?= /usr/include