From 2fae62f615496ee1dccb6e4251a6f7392deb1e98 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 9 May 2024 19:07:45 -0400 Subject: [PATCH 01/13] Add additional tests for running services in dom0 This checks that the RPC multiplexer only allows calls with 2 or 4 arguments and sets environment variables $QREXEC_REQUESTED_TARGET_TYPE, $QREXEC_REQUESTED_TARGET, and $QREXEC_REQUESTED_TARGET_KEYWORD properly. Since it only affects the tests, it can safely be backported to R4.2. --- qrexec/tests/socket/agent.py | 1 + qrexec/tests/socket/daemon.py | 99 ++++++++++++++++++++++++++++------- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index b17812e5..b3a221d8 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -1339,6 +1339,7 @@ def assertStdoutMessages( msg_type, msg_body = target.recv_message() self.assertEqual(msg_type, ty) l = len(msg_body) + self.assertGreater(l, 0) self.assertEqual(msg_body, expected_stdout[bytes_recvd:l]) bytes_recvd += l self.assertEqual(bytes_recvd, expected) diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index d561e740..c7e3a1e9 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -1000,36 +1000,73 @@ def test_exec_service_with_invalid_config_5(self): def test_exec_service_with_invalid_config_6(self): self.exec_service_with_invalid_config(None) - def _test_run_dom0_service_exec(self, nogui): + def assertExpectedStdout( + self, target, expected_stdout: bytes, exit_code: int = 0 + ): + bytes_recvd, expected = 0, len(expected_stdout) + while bytes_recvd < expected: + msg_type, msg_body = target.recv_message() + self.assertEqual(msg_type, qrexec.MSG_DATA_STDOUT) + l = len(msg_body) + self.assertGreater(l, 0) + new_bytes_received = bytes_recvd + l + self.assertEqual( + msg_body, expected_stdout[bytes_recvd:new_bytes_received] + ) + bytes_recvd = new_bytes_received + self.assertEqual(bytes_recvd, expected) + self.assertListEqual( + util.sort_messages(target.recv_all_messages()), + [ + (qrexec.MSG_DATA_STDOUT, b""), + (qrexec.MSG_DATA_EXIT_CODE, struct.pack(" Date: Wed, 10 Apr 2024 18:02:45 -0400 Subject: [PATCH 02/13] Test that stderr from qrexec-agent is properly received No functional change. --- qrexec/tests/socket/agent.py | 67 +++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index b3a221d8..c71c3c43 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -58,25 +58,49 @@ def check_dom0(self, dom0): ), ) - def assertExpectedStdout( - self, target, expected_stdout: bytes, *, exit_code=0 - ): + def assertExpectedStdoutStderr( + self, + target, + expected_stdout: bytes, + expected_stderr: bytes, + *, + exit_code: int = 0, + ) -> None: messages = util.sort_messages(target.recv_all_messages()) self.assertListEqual( - messages[-3:], + messages[-2:], [ - (qrexec.MSG_DATA_STDOUT, b""), (qrexec.MSG_DATA_STDERR, b""), (qrexec.MSG_DATA_EXIT_CODE, struct.pack("> " + shlex.quote(fifo)).encode("ascii", "strict") @@ -219,7 +243,7 @@ def test_just_exec(self): self.assertEqual(f.read(), b"a\n") self.check_dom0(dom0) - def test_just_exec_rpc(self): + def test_003_just_exec_rpc(self): fifo = os.path.join(self.tempdir, "new_file") os.mkfifo(fifo, mode=0o600) util.make_executable_service( @@ -227,7 +251,8 @@ def test_just_exec_rpc(self): "rpc", "qubes.Service", rf"""#!/bin/bash -eu -printf %s\\n "$QREXEC_SERVICE_FULL_NAME" >> {shlex.quote(fifo)} +printf %s "$QREXEC_SERVICE_FULL_NAME" >> {shlex.quote(fifo)} +exit 1 """, ) cmd = b"QUBESRPC qubes.Service+ domX" @@ -238,9 +263,8 @@ def test_just_exec_rpc(self): (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), ], ) - with open(fifo, "rb") as f: - self.assertEqual(f.read(), b"qubes.Service+\n") + self.assertEqual(f.read(), b"qubes.Service+") self.check_dom0(dom0) def test_just_exec_rpc_not_found(self): @@ -400,11 +424,14 @@ def test_exec_service(self): """\ #!/bin/sh echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN" +printf 'something on stderr' >&2 """, ) target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") target.send_message(qrexec.MSG_DATA_STDIN, b"") - self.assertExpectedStdout(target, b"arg: arg, remote domain: domX\n") + self.assertExpectedStdoutStderr( + target, b"arg: arg, remote domain: domX\n", b"something on stderr" + ) self.check_dom0(dom0) def test_exec_service_keyword(self): @@ -695,7 +722,7 @@ def test_exec_null_argument_finds_service_for_empty_argument(self): "rpc", "qubes.Service", """\ -#!/bin/sh +#!/bin/sh -- echo "general service" """, ) From 47f444a2f3e7f7e94ffb974fe716161deaef310b Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 Jan 2025 00:38:48 -0500 Subject: [PATCH 03/13] Fix spurious newline in error message No other change. --- daemon/qrexec-client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index 92d4fd45..c14118ed 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -134,7 +134,7 @@ static void parse_connect(char *str, char **request_id, if (token == NULL) goto bad_c_param; if ((size_t)(token - str) >= sizeof(struct service_params)) - errx(1, "Invalid -c parameter (request_id too long, max %zu)\n", + errx(1, "Invalid -c parameter (request_id too long, max %zu)", sizeof(struct service_params)-1); *token = 0; *request_id = str; From 950d3835ec1cd2ab36e7ca3901ddd10ea064adc4 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 Jan 2025 00:40:58 -0500 Subject: [PATCH 04/13] Add type annotation to Python code No other functional change. --- qrexec/client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qrexec/client.py b/qrexec/client.py index f2d97d8e..32783328 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -18,6 +18,7 @@ import pathlib import subprocess import asyncio +from typing import Optional from .utils import prepare_subprocess_kwds @@ -33,7 +34,7 @@ VERSION = "vm" -def call(dest, rpcname, arg=None, *, input=None): +def call(dest: str, rpcname: str, arg: Optional[str] = None, *, input=None): """Invoke qrexec call The `input` parameter should be either :py:class:`str` or :py:class:`bytes` From a4506140e79075901e4e98f6908a43d03d0d17e9 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 Jan 2025 00:41:23 -0500 Subject: [PATCH 05/13] Python: Assert that there is no space in command arguments This would result in malformed commands being generated. --- qrexec/client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qrexec/client.py b/qrexec/client.py index 32783328..88fab528 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -100,7 +100,9 @@ async def call_async(dest, rpcname, arg=None, *, input=None): def make_command(dest, rpcname, arg): assert "+" not in rpcname + assert " " not in rpcname if arg is not None: + assert " " not in arg rpcname = f"{rpcname}+{arg}" if VERSION == "dom0" and dest == "dom0": From 20e39b16f90db65fa88beccb1104325427b27524 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 Jan 2025 00:42:57 -0500 Subject: [PATCH 06/13] Python: Add "--" to qrexec-client-vm call This avoids option injection if an invalid VM name starting with '-' is passed. --- qrexec/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qrexec/client.py b/qrexec/client.py index 88fab528..f83b491d 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -118,7 +118,7 @@ def make_command(dest, rpcname, arg): f"DEFAULT:QUBESRPC {rpcname} dom0", ] if VERSION == "vm": - return [QREXEC_CLIENT_VM, dest, rpcname] + return [QREXEC_CLIENT_VM, "--", dest, rpcname] assert VERSION is None raise NotImplementedError("qrexec not available") From d52e8d4fd3c0e86b1a1cc32caf864e1f6b546499 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 Jan 2025 17:21:39 -0500 Subject: [PATCH 07/13] Call abort() if there is a qrexec buffer problem These should never happen unless out of memory. --- libqrexec/buffer.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libqrexec/buffer.c b/libqrexec/buffer.c index 1c49a07e..217d94dd 100644 --- a/libqrexec/buffer.c +++ b/libqrexec/buffer.c @@ -35,12 +35,12 @@ static char *limited_malloc(int len) (total_mem > BUFFER_LIMIT) || (len <= 0)) { LOG(ERROR, "attempt to allocate >BUFFER_LIMIT"); - exit(1); + abort(); } ret = malloc((size_t)len); if (!ret) { PERROR("malloc"); - exit(1); + abort(); } return ret; } @@ -83,11 +83,11 @@ void buffer_append(struct buffer *b, const char *data, int len) assert(data != NULL && "NULL data"); if (b->buflen < 0 || b->buflen > BUFFER_LIMIT) { LOG(ERROR, "buffer_append buflen %d", len); - exit(1); + abort(); } if (len < 0 || len > BUFFER_LIMIT) { LOG(ERROR, "buffer_append %d", len); - exit(1); + abort(); } if (len == 0) return; @@ -108,7 +108,7 @@ void buffer_remove(struct buffer *b, int len) char *qdata = NULL; if (len < 0 || len > b->buflen) { LOG(ERROR, "buffer_remove %d/%d", len, b->buflen); - exit(1); + abort(); } newsize = b->buflen - len; if (newsize > 0) { From 9ea7ea2111f95acddff9343ad26707e058a371e9 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 19 Jan 2025 22:18:38 -0500 Subject: [PATCH 08/13] Test that opening /proc/self/fd/2 for writing works USB forwarding requires this, but there were no unit tests for it, so a buggy patch that broke it was not noticed until openQA tested USB forwarding. --- qrexec/tests/socket/agent.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index c71c3c43..5d21d22f 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -425,6 +425,28 @@ def test_exec_service(self): #!/bin/sh echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN" printf 'something on stderr' >&2 +""", + ) + target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") + target.send_message(qrexec.MSG_DATA_STDIN, b"") + self.assertExpectedStdoutStderr( + target, b"arg: arg, remote domain: domX\n", b"something on stderr" + ) + self.check_dom0(dom0) + + def test_exec_service_stderr_redirection(self): + """ + USB forwarding expects to open /proc/PID/fd/2 for writing. + Check that this works. + """ + util.make_executable_service( + self.tempdir, + "rpc", + "qubes.Service", + """\ +#!/bin/sh +echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN" +printf 'something on stderr' > /proc/self/fd/2 """, ) target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") From 87460969141755458f712ffa7335e919796a7bf4 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 Jan 2025 00:41:23 -0500 Subject: [PATCH 09/13] Python: Assert that there is no space in command arguments This would result in malformed commands being generated. --- qrexec/client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qrexec/client.py b/qrexec/client.py index f83b491d..dca84294 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -104,6 +104,8 @@ def make_command(dest, rpcname, arg): if arg is not None: assert " " not in arg rpcname = f"{rpcname}+{arg}" + else: + rpcname += "+" if VERSION == "dom0" and dest == "dom0": # Invoke qubes-rpc-multiplexer directly. This will work for non-socket From 694bd93edd194529b76e1896ed41d081502e3d8e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 10 Apr 2024 18:02:45 -0400 Subject: [PATCH 10/13] Stop using qubes-rpc-multiplexer for service calls Instead, directly execute the command from C. All variables with names beginning with QREXEC are stripped from the environment, except for QREXEC_SERVICE_PATH. This is a change in behavior compared to the current code. This is a backwards-incompatible change to exec_qubes_rpc_if_requested(), which now takes an extra argument. Therefore, it cannot be backported to R4.2. It also requires changing the SELinux policy so that the labels on /etc/qubes-rpc/ and /usr/local/etc/qubes-rpc/ (and their contents) are correct. qubes-rpc-multiplexer is still present because it has legacy uses in Python code and for compatibility. Fixes: QubesOS/qubes-issues#9062 --- agent/qrexec-agent-data.c | 33 +++++- agent/qrexec-agent.c | 15 ++- agent/qrexec-agent.h | 2 +- agent/qrexec-client-vm.c | 2 +- agent/qrexec-fork-server.c | 6 +- daemon/qrexec-client.c | 13 ++- daemon/qrexec-daemon.c | 12 +- libqrexec/Makefile | 2 +- libqrexec/exec.c | 205 ++++++++++++++++++++++++++-------- libqrexec/libqrexec-utils.h | 37 ++++-- libqrexec/open_logger.c | 38 +++++++ libqrexec/process_io.c | 5 +- libqrexec/qrexec.h | 1 - libqrexec/remote.c | 5 +- libqrexec/remote.h | 5 +- qrexec/tests/socket/agent.py | 21 +--- qrexec/tests/socket/daemon.py | 7 +- selinux/qubes-core-qrexec.fc | 4 +- 18 files changed, 305 insertions(+), 108 deletions(-) create mode 100644 libqrexec/open_logger.c diff --git a/agent/qrexec-agent-data.c b/agent/qrexec-agent-data.c index 8e760936..08fb71ff 100644 --- a/agent/qrexec-agent-data.c +++ b/agent/qrexec-agent-data.c @@ -138,22 +138,29 @@ int handle_handshake(libvchan_t *ctrl) static int handle_just_exec(struct qrexec_parsed_command *cmd) { - int fdn, pid; + int fdn, pid, log_fd; if (cmd == NULL) return QREXEC_EXIT_PROBLEM; + char file_path[QUBES_SOCKADDR_UN_MAX_PATH_LEN]; + struct buffer buf = { .data = file_path, .buflen = (int)sizeof(file_path) }; + struct buffer stdin_buffer; + buffer_init(&stdin_buffer); if (cmd->service_descriptor) { int socket_fd; struct buffer stdin_buffer; buffer_init(&stdin_buffer); - int status = find_qrexec_service(cmd, &socket_fd, &stdin_buffer); + int status = find_qrexec_service(cmd, &socket_fd, &stdin_buffer, &buf); if (status == -1) return QREXEC_EXIT_SERVICE_NOT_FOUND; if (status != 0) return QREXEC_EXIT_PROBLEM; if (socket_fd != -1) - return write_all(socket_fd, stdin_buffer.data, stdin_buffer.buflen) ? 0 : QREXEC_EXIT_PROBLEM; + return write_all(socket_fd, stdin_buffer.data, stdin_buffer.buflen) ? + 0 : QREXEC_EXIT_PROBLEM; + } else { + buf.data = NULL; } switch (pid = fork()) { case -1: @@ -161,11 +168,22 @@ static int handle_just_exec(struct qrexec_parsed_command *cmd) return QREXEC_EXIT_PROBLEM; case 0: fdn = open("/dev/null", O_RDWR); - fix_fds(fdn, fdn, fdn); - do_exec(cmd->command, cmd->username); + if (fdn < 0) { + LOG(ERROR, "open /dev/null failed"); + _exit(QREXEC_EXIT_PROBLEM); + } + int other_pid; + log_fd = cmd->service_descriptor ? open_logger(cmd, &other_pid) : fdn; + if (log_fd < 0) + _exit(QREXEC_EXIT_PROBLEM); + fix_fds(fdn, fdn, log_fd); + do_exec(buf.data, cmd->command, cmd->username); default:; } - LOG(INFO, "executed (nowait): %s (pid %d)", cmd->command, pid); + if (buf.data) + LOG(INFO, "executed (nowait): %s %s (pid %d)", buf.data, cmd->command, pid); + else + LOG(INFO, "executed (nowait): %s (pid %d)", cmd->command, pid); return 0; } @@ -261,6 +279,7 @@ static int handle_new_process_common( return 0; } + int logger_pid = 0; req.vchan = data_vchan; req.stdin_buf = &stdin_buf; @@ -280,6 +299,8 @@ static int handle_new_process_common( req.prefix_data.data = NULL; req.prefix_data.len = 0; + if (cmd->service_descriptor != NULL) + req.logger_fd = open_logger(cmd, &logger_pid); exit_code = qrexec_process_io(&req, cmd); diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index b4f41212..16887660 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -137,7 +137,7 @@ static struct pam_conv conv = { * If dom0 sends overly long cmd, it will probably crash qrexec-agent (unless * process can allocate up to 4GB on both stack and heap), sorry. */ -_Noreturn void do_exec(const char *cmd, const char *user) +_Noreturn void do_exec(const char *prog, const char *cmd, const char *user) { #ifdef HAVE_PAM int retval, status; @@ -172,7 +172,8 @@ _Noreturn void do_exec(const char *cmd, const char *user) exit(1); } /* call QUBESRPC if requested */ - exec_qubes_rpc_if_requested(cmd, environ); + /* no point in creating a login shell for test environments */ + exec_qubes_rpc_if_requested2(prog, cmd, environ, false); /* otherwise exec shell */ execl("/bin/sh", "sh", "-c", cmd, NULL); @@ -278,8 +279,9 @@ _Noreturn void do_exec(const char *cmd, const char *user) if (retval == -1) warn("chdir(%s)", pw->pw_dir); - /* call QUBESRPC if requested */ - exec_qubes_rpc_if_requested(cmd, env); + /* Call QUBESRPC if requested, using a login shell to set up + * environment variables. */ + exec_qubes_rpc_if_requested2(prog, cmd, env, true); /* otherwise exec shell */ execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env); @@ -316,8 +318,9 @@ _Noreturn void do_exec(const char *cmd, const char *user) pam_end(pamh, PAM_ABORT); exit(1); #else - /* call QUBESRPC if requested */ - exec_qubes_rpc_if_requested(cmd, environ); + /* Call QUBESRPC if requested, using a login shell to set up + * environment variables. */ + exec_qubes_rpc_if_requested2(prog, cmd, environ, true); /* otherwise exec shell */ execl("/bin/su", "su", "-", user, "-c", cmd, NULL); diff --git a/agent/qrexec-agent.h b/agent/qrexec-agent.h index 83ed3f43..f85064ed 100644 --- a/agent/qrexec-agent.h +++ b/agent/qrexec-agent.h @@ -29,7 +29,7 @@ int handle_handshake(libvchan_t *ctrl); void handle_vchan_error(const char *op); -_Noreturn void do_exec(const char *cmd, const char *user); +_Noreturn void do_exec(const char *prog, const char *cmd, const char *user); /* call before fork() for service handling process (either end) */ void prepare_child_env(void); diff --git a/agent/qrexec-client-vm.c b/agent/qrexec-client-vm.c index 0f03bb8d..e933448b 100644 --- a/agent/qrexec-client-vm.c +++ b/agent/qrexec-client-vm.c @@ -43,7 +43,7 @@ _Noreturn void handle_vchan_error(const char *op) exit(1); } -_Noreturn void do_exec(const char *cmd __attribute__((unused)), char const* user __attribute__((__unused__))) { +_Noreturn void do_exec(const char *prog __attribute__((unused)), const char *cmd __attribute__((unused)), char const* user __attribute__((__unused__))) { LOG(ERROR, "BUG: do_exec function shouldn't be called!"); abort(); } diff --git a/agent/qrexec-fork-server.c b/agent/qrexec-fork-server.c index 9edba38e..d1a78d6f 100644 --- a/agent/qrexec-fork-server.c +++ b/agent/qrexec-fork-server.c @@ -37,15 +37,15 @@ extern char **environ; const bool qrexec_is_fork_server = true; -void do_exec(const char *cmd, const char *user __attribute__((unused))) +void do_exec(const char *prog, const char *cmd, const char *user __attribute__((unused))) { char *shell; signal(SIGCHLD, SIG_DFL); signal(SIGPIPE, SIG_DFL); - /* call QUBESRPC if requested */ - exec_qubes_rpc_if_requested(cmd, environ); + /* Call QUBESRPC if requested. This code already runs in a login session. */ + exec_qubes_rpc_if_requested2(prog, cmd, environ, false); /* otherwise, pass it to shell */ shell = getenv("SHELL"); diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index c14118ed..56b44d65 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -62,13 +62,16 @@ static void set_remote_domain(const char *src_domain_name) { } /* called from do_fork_exec */ -static _Noreturn void do_exec(const char *prog, const char *username __attribute__((unused))) +static _Noreturn void do_exec(const char *prog, + const char *cmdline, + const char *username __attribute__((unused))) { - /* avoid calling qubes-rpc-multiplexer through shell */ - exec_qubes_rpc_if_requested(prog, environ); + /* Avoid calling RPC command through shell. + * Qrexec-client is always in a login session. */ + exec_qubes_rpc_if_requested2(prog, cmdline, environ, false); - /* if above haven't executed qubes-rpc-multiplexer, pass it to shell */ - execl("/bin/bash", "bash", "-c", prog, NULL); + /* if above haven't executed RPC command, 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 index 3d20849e..fac3e209 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -1131,14 +1131,16 @@ static enum policy_response connect_daemon_socket( } /* called from do_fork_exec */ -static _Noreturn void do_exec(const char *prog, const char *username __attribute__((unused))) +static _Noreturn void do_exec(const char *prog, const char *cmd, const char *username __attribute__((unused))) { - /* avoid calling qubes-rpc-multiplexer through shell */ - exec_qubes_rpc_if_requested(prog, environ); + /* Avoid calling RPC command through shell. + * Qrexec-daemon is always in a login session already. */ + exec_qubes_rpc_if_requested2(prog, cmd, environ, true); - /* if above haven't executed qubes-rpc-multiplexer, pass it to shell */ - execl("/bin/bash", "bash", "-c", prog, NULL); + /* if above haven't executed RPC command, pass it to shell */ + execl("/bin/bash", "bash", "-c", cmd, NULL); PERROR("exec bash"); + /* treat ENOENT as "problem" because bash should always exist */ _exit(QREXEC_EXIT_PROBLEM); } diff --git a/libqrexec/Makefile b/libqrexec/Makefile index 3838f6a6..bd9bcb8c 100644 --- a/libqrexec/Makefile +++ b/libqrexec/Makefile @@ -22,7 +22,7 @@ endif all: libqrexec-utils.so -libqrexec-utils.so.$(SO_VER): unix-server.o ioall.o buffer.o exec.o txrx-vchan.o write-stdin.o replace.o remote.o process_io.o log.o toml.o vchan_timeout.o +libqrexec-utils.so.$(SO_VER): unix-server.o ioall.o buffer.o exec.o txrx-vchan.o write-stdin.o replace.o remote.o process_io.o log.o toml.o open_logger.o vchan_timeout.o $(CC) $(LDFLAGS) -Wl,-soname,$@ -o $@ $^ $(VCHANLIBS) libqrexec-utils.so: libqrexec-utils.so.$(SO_VER) diff --git a/libqrexec/exec.c b/libqrexec/exec.c index 1c6196c2..ff836e86 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -47,43 +47,146 @@ void register_exec_func(do_exec_t *func) { exec_func = func; } -void exec_qubes_rpc_if_requested(const char *prog, char *const envp[]) { - /* avoid calling qubes-rpc-multiplexer through shell */ - if (strncmp(prog, RPC_REQUEST_COMMAND, RPC_REQUEST_COMMAND_LEN) == 0) { +#define STR_LITERAL_SIZE(a) (sizeof("" a) - 1) +#define STARTS_WITH_LITERAL(a, b) (strncmp(a, b, STR_LITERAL_SIZE(b)) == 0) + +static bool should_strip_env_var(const char *var) +{ + if (!STARTS_WITH_LITERAL(var, "QREXEC")) + return false; + const char *rest = var + STR_LITERAL_SIZE("QREXEC"); + return !STARTS_WITH_LITERAL(rest, "_SERVICE_PATH=") && + !STARTS_WITH_LITERAL(rest, "_AGENT_PID="); +} + +void exec_qubes_rpc_if_requested2(const char *program, const char *cmd, char *const envp[], + bool use_shell) { + /* avoid calling RPC service through shell */ + if (program) { + assert(program); char *prog_copy; char *tok, *savetok; - char *argv[16]; // right now 6 are used, but allow future extensions - size_t i = 1; + const char *argv[7]; + 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); + } - if (prog[RPC_REQUEST_COMMAND_LEN] != ' ') { - LOG(ERROR, "\"" RPC_REQUEST_COMMAND "\" not followed by space"); - _exit(126); + 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) +#define EXTEND_RAW(arg) \ + 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); + } } - prog_copy = strdup(prog + RPC_REQUEST_COMMAND_LEN + 1); + prog_copy = strdup(cmd + RPC_REQUEST_COMMAND_LEN + 1); if (!prog_copy) { PERROR("strdup"); _exit(QREXEC_EXIT_PROBLEM); } + argv[i++] = program; tok=strtok_r(prog_copy, " ", &savetok); while (tok != NULL) { if (i >= sizeof(argv)/sizeof(argv[0])-1) { - LOG(ERROR, "To many arguments to %s", RPC_REQUEST_COMMAND); + LOG(ERROR, "Too many arguments to %s", RPC_REQUEST_COMMAND); _exit(QREXEC_EXIT_PROBLEM); } argv[i++] = tok; - tok=strtok_r(NULL, " ", &savetok); + tok = strtok_r(NULL, " ", &savetok); } argv[i] = NULL; - - argv[0] = getenv("QREXEC_MULTIPLEXER_PATH"); - if (!argv[0]) - argv[0] = QUBES_RPC_MULTIPLEXER_PATH; - execve(argv[0], argv, envp); - bool noent = errno == ENOENT; - PERROR("exec qubes-rpc-multiplexer"); - _exit(noent ? QREXEC_EXIT_SERVICE_NOT_FOUND : QREXEC_EXIT_PROBLEM); + 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 + } + } else if (i == 3) { + EXTEND_RAW("QREXEC_REQUESTED_TARGET_TYPE="); + } else { + 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; + if (p != NULL) { + EXTEND("QREXEC_SERVICE_ARGUMENT=%s", p + 1); + if (p[1] != '\0') { + argv[1] = p + 1; + argv[2] = NULL; + } + } + assert(iterator <= env_amount); + buf[iterator] = NULL; + if (program[0] != '/') { + LOG(ERROR, "Program to execute not absolute path"); + _exit(QREXEC_EXIT_PROBLEM); + } + if (use_shell) { + /* Move the command to after 'sh -lc "exec \"\$@\"" sh ' */ + argv[4] = argv[0]; + argv[5] = argv[1]; + argv[6] = NULL; + /* Now fill in the shell invocation itelf */ + argv[0] = "sh"; + argv[1] = "-lc"; + /* Do not use "-lcexec \"$@\"": -c does not take an argument + * itself, and instead causes the first non-option argument + * to be interpreted as a command. */ + argv[2] = "exec \"$@\""; + argv[3] = "sh"; + execve("/bin/sh", (char *const *)argv, buf); + /* /bin/sh should always exist */ + _exit(QREXEC_EXIT_PROBLEM); + } else { + execve(program, (char *const *)argv, buf); + _exit(errno == ENOENT ? QREXEC_EXIT_SERVICE_NOT_FOUND : QREXEC_EXIT_PROBLEM); + } +bad_asprintf: + PERROR("asprintf"); + _exit(QREXEC_EXIT_PROBLEM); + } else { + assert(strncmp(cmd, RPC_REQUEST_COMMAND, RPC_REQUEST_COMMAND_LEN) != 0); } } @@ -94,7 +197,9 @@ void fix_fds(int fdin, int fdout, int fderr) fdin, fdout, fderr); _exit(125); } - if (dup2(fdin, 0) < 0 || dup2(fdout, 1) < 0 || dup2(fderr, 2) < 0) { + if ((fdin != 0 && dup2(fdin, 0) < 0) || + (fdout != 1 && dup2(fdout, 1) < 0) || + (fderr != 2 && dup2(fderr, 2) < 0)) { PERROR("dup2"); abort(); } @@ -112,7 +217,8 @@ void fix_fds(int fdin, int fdout, int fderr) close(i); } -static int do_fork_exec(const char *user, +static int do_fork_exec(const char *prog, + const char *user, const char *cmdline, int *pid, int *stdin_fd, @@ -124,8 +230,12 @@ static int do_fork_exec(const char *user, #define SOCK_CLOEXEC 0 #endif if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, inpipe) || - socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, outpipe) || - (stderr_fd && socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, errpipe))) { + socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, outpipe)) { + PERROR("socketpair"); + /* FD leaks do not matter, we exit soon anyway */ + return -2; + } + if (stderr_fd != NULL && pipe2(errpipe, O_CLOEXEC) != 0) { PERROR("socketpair"); /* FD leaks do not matter, we exit soon anyway */ return -2; @@ -144,7 +254,7 @@ static int do_fork_exec(const char *user, fix_fds(inpipe[0], outpipe[1], 2); if (exec_func != NULL) - exec_func(cmdline, user); + exec_func(prog, cmdline, user); abort(); default: retval = 0; @@ -450,7 +560,7 @@ struct qrexec_parsed_command *parse_qubes_rpc_command( goto err; } - size_t const descriptor_len = (size_t)(end - start); + size_t const descriptor_len = cmd->service_descriptor_len = (size_t)(end - start); if (descriptor_len > MAX_SERVICE_NAME_LEN) { LOG(ERROR, "Command too long (length %zu)", descriptor_len); goto err; @@ -538,7 +648,9 @@ int execute_parsed_qubes_rpc_command( int *stdout_fd, int *stderr_fd, struct buffer *stdin_buffer) { if (cmd->service_descriptor) { // Proper Qubes RPC call - int find_res = find_qrexec_service(cmd, stdin_fd, stdin_buffer); + char file_path[QUBES_SOCKADDR_UN_MAX_PATH_LEN]; + struct buffer buf = { .data = file_path, .buflen = (int)sizeof(file_path) }; + int find_res = find_qrexec_service(cmd, stdin_fd, stdin_buffer, &buf); if (find_res != 0) { assert(find_res < 0); return find_res; @@ -550,12 +662,12 @@ int execute_parsed_qubes_rpc_command( *pid = 0; return 0; } - return do_fork_exec(cmd->username, cmd->command, + return do_fork_exec(buf.data, cmd->username, cmd->command, pid, stdin_fd, stdout_fd, stderr_fd); } else { // Legacy qrexec behavior: spawn shell directly - return do_fork_exec(cmd->username, cmd->command, - pid, stdin_fd, stdout_fd, stderr_fd); + return do_fork_exec(NULL, cmd->username, cmd->command, + pid, stdin_fd, stdout_fd, stderr_fd); } } static bool validate_port(const char *port) { @@ -632,11 +744,11 @@ static int qubes_tcp_connect(const char *host, const char *port) int find_qrexec_service( struct qrexec_parsed_command *cmd, - int *socket_fd, struct buffer *stdin_buffer) { + int *socket_fd, struct buffer *stdin_buffer, + struct buffer *path_buffer) { assert(cmd->service_descriptor); + assert(path_buffer->buflen > NAME_MAX); - char file_path[QUBES_SOCKADDR_UN_MAX_PATH_LEN]; - struct buffer path_buffer = { .data = file_path, .buflen = (int)sizeof(file_path) }; const char *qrexec_service_path = getenv("QREXEC_SERVICE_PATH"); if (!qrexec_service_path) qrexec_service_path = QREXEC_SERVICE_PATH; @@ -645,11 +757,11 @@ int find_qrexec_service( struct stat statbuf; int ret = find_file(qrexec_service_path, cmd->service_descriptor, - path_buffer.data, (size_t)path_buffer.buflen, + path_buffer->data, (size_t)path_buffer->buflen, &statbuf); if (ret == -1) ret = find_file(qrexec_service_path, cmd->service_name, - path_buffer.data, (size_t)path_buffer.buflen, + path_buffer->data, (size_t)path_buffer->buflen, &statbuf); if (ret < 0) { if (ret == -1) @@ -662,11 +774,11 @@ int find_qrexec_service( if (S_ISSOCK(statbuf.st_mode)) { /* Socket-based service. */ int s; - if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { + if ((s = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0)) == -1) { PERROR("socket"); return -2; } - if (qubes_connect(s, path_buffer.data, strlen(path_buffer.data))) { + if (qubes_connect(s, path_buffer->data, strlen(path_buffer->data))) { PERROR("qubes_connect"); close(s); return -2; @@ -682,9 +794,9 @@ int find_qrexec_service( return 0; } else if (S_ISLNK(statbuf.st_mode)) { /* TCP-based service */ - assert(path_buffer.buflen >= (int)sizeof("/dev/tcp") - 1); - assert(memcmp(path_buffer.data, "/dev/tcp", sizeof("/dev/tcp") - 1) == 0); - char *address = path_buffer.data + (sizeof("/dev/tcp") - 1); + assert(path_buffer->buflen >= (int)sizeof("/dev/tcp") - 1); + assert(memcmp(path_buffer->data, "/dev/tcp", sizeof("/dev/tcp") - 1) == 0); + char *address = path_buffer->data + (sizeof("/dev/tcp") - 1); char *host = NULL, *port = NULL; if (*address == '/') { host = address + 1; @@ -699,7 +811,7 @@ int find_qrexec_service( if (port == NULL) { if (cmd->arg == NULL || *cmd->arg == '\0') { LOG(ERROR, "No or empty argument provided, cannot connect to %s", - path_buffer.data); + path_buffer->data); return -2; } if (host == NULL) { @@ -726,7 +838,8 @@ int find_qrexec_service( } } else { if (cmd->arg != NULL && *cmd->arg != '\0') { - LOG(ERROR, "Unexpected argument %s to service %s", cmd->arg, path_buffer.data); + LOG(ERROR, "Unexpected argument %s to service %s", cmd->arg, + path_buffer->data); return -2; } } @@ -744,30 +857,30 @@ int find_qrexec_service( return 0; } - if (euidaccess(path_buffer.data, X_OK) == 0) { + if (euidaccess(path_buffer->data, X_OK) == 0) { /* Executable-based service. */ if (!cmd->send_service_descriptor) { LOG(WARNING, "Warning: ignoring skip-service-descriptor=true " "for execute executable service %s", - path_buffer.data); + path_buffer->data); } if (cmd->exit_on_stdout_eof) { LOG(WARNING, "Warning: ignoring exit-on-service-eof=true " "for executable service %s", - path_buffer.data); + path_buffer->data); cmd->exit_on_stdout_eof = false; } if (cmd->exit_on_stdin_eof) { LOG(WARNING, "Warning: ignoring exit-on-client-eof=true " "for executable service %s", - path_buffer.data); + path_buffer->data); cmd->exit_on_stdin_eof = false; } return 0; } LOG(ERROR, "Unknown service type (not executable, not a socket): %.*s", - path_buffer.buflen, path_buffer.data); + path_buffer->buflen, path_buffer->data); return -2; } diff --git a/libqrexec/libqrexec-utils.h b/libqrexec/libqrexec-utils.h index 18d9b141..2390cc2c 100644 --- a/libqrexec/libqrexec-utils.h +++ b/libqrexec/libqrexec-utils.h @@ -104,6 +104,8 @@ struct qrexec_parsed_command { /* Pointer to the argument, or NULL if there is no argument. * Same buffer as "service_descriptor". */ char *arg; + /* length of the command */ + size_t service_descriptor_len; }; /* Parse a command, return NULL on failure. Uses cmd->cmdline @@ -137,16 +139,28 @@ int load_service_config(struct qrexec_parsed_command *cmd_name, __attribute__((visibility("default"))) int load_service_config_v2(struct qrexec_parsed_command *cmd_name); -typedef void (do_exec_t)(const char *cmdline, const char *user); +typedef void (do_exec_t)(const char *program, const char *cmd, const char *user); __attribute__((visibility("default"))) void register_exec_func(do_exec_t *func); -/* - * exec() qubes-rpc-multiplexer if *prog* starts with magic "QUBESRPC" keyword, - * do not return in that case; pass *envp* to execve() as en environment - * otherwise, return false without any action + +/** + * \param program Full path to program to execute or NULL. + * \param cmd RPC command, including "QUBESRPC " prefix, if *program* is not NULL. + * Otherwise *program* must be NULL and *cmd* must not start with "QUBESRPC". + * \param envp Environment passed to execve(), ignored if *program* is NULL. + * \param use_shell If true, use a login shell to spawn the program. + * + * If *program* is not NULL, execute it as an RPC service or call _exit() on failure. + * *cmd* is used to set the argument (if any) and "QREXEC_*" environment variables. + * Environment variables in *envp* that start with "QREXEC" are ignored, except for + * "QREXEC_SERVICE_PATH" and "QREXEC_AGENT_PID", which are inherited. If *program* + * is not NULL and *cmd* does not start with "QUBESRPC ", or if *program* is NULL + * and *cmd* starts with "QUBESRPC", fails an assertion. If *program* is NULL and + * *cmd* does not start with "QUBESRPC", returns without doing anything. */ __attribute__((visibility("default"))) -void exec_qubes_rpc_if_requested(const char *prog, char *const envp[]); +void exec_qubes_rpc_if_requested2(const char *program, const char *cmd, char *const envp[], + bool use_shell); /* Execute `qubes.WaitForSession` service, do not return on success, return -1 * (maybe setting errno) on failure. */ @@ -194,13 +208,17 @@ int execute_parsed_qubes_rpc_command( * the socket, or -1 for executable services. * @param stdin_buffer This buffer will need to be prepended to the child process’s * stdin. + * @param path_buffer This buffer (NUL-terminated) holds the service's path. On + * entry it must be at least NAME_MAX bytes. It will not be freed or reallocated. + * Its contents should be ignored if stdout_fd is not -1. * @return 0 if the implementation is found (and, for sockets, connected to) * successfully, -1 if not found, -2 if problem. */ __attribute__((visibility("default"))) int find_qrexec_service( struct qrexec_parsed_command *cmd, - int *socket_fd, struct buffer *stdin_buffer); + int *socket_fd, struct buffer *stdin_buffer, + struct buffer *path_buffer); /** Suggested buffer size for the path buffer of find_qrexec_service. */ #define QUBES_SOCKADDR_UN_MAX_PATH_LEN 1024 @@ -288,7 +306,7 @@ struct process_io_request { * *local* process. For a *remote* process, stdin_fd is the standard * *output*, stdout_fd is the standard *input*, and stderr_fd must be -1. */ // stderr_fd can be -1 - int stdin_fd, stdout_fd, stderr_fd; + int stdin_fd, stdout_fd, stderr_fd, logger_fd; // 0 if no child process pid_t local_pid; @@ -318,6 +336,9 @@ struct process_io_request { volatile sig_atomic_t *sigusr1; struct prefix_data prefix_data; }; +/** Open an FD to a logger */ +__attribute__((visibility("default"))) +int open_logger(struct qrexec_parsed_command *command, int *pid); /* * Pass IO between vchan and local FDs. See the comments for diff --git a/libqrexec/open_logger.c b/libqrexec/open_logger.c new file mode 100644 index 00000000..a1d378b4 --- /dev/null +++ b/libqrexec/open_logger.c @@ -0,0 +1,38 @@ +#include +#include +#include +#include +#include + +int open_logger(struct qrexec_parsed_command *command, int *pid) +{ + int pipes[2]; + if (pipe2(pipes, O_CLOEXEC)) { + LOG(ERROR, "Cannot create status pipe"); + return -1; + } + char *buf[] = { + "logger", + "-t", + NULL, + NULL, + }; + if (asprintf(buf + 2, "%.*s-%s", (int)command->service_descriptor_len, + command->service_descriptor, command->source_domain) < 0) { + LOG(ERROR, "asprintf() failed"); + return -1; + } + switch ((*pid = fork())) { + case -1: + LOG(ERROR, "Cannot fork logger process"); + return -1; + case 0: + fix_fds(pipes[0], 1, 2); + execvp("logger", buf); + _exit(126); + default: + free(buf[2]); + close(pipes[0]); + return pipes[1]; + } +} diff --git a/libqrexec/process_io.c b/libqrexec/process_io.c index b0524876..4b16baf6 100644 --- a/libqrexec/process_io.c +++ b/libqrexec/process_io.c @@ -96,6 +96,7 @@ int qrexec_process_io(const struct process_io_request *req, int stdin_fd = req->stdin_fd; int stdout_fd = req->stdout_fd; int stderr_fd = req->stderr_fd; + int dup_fd = req->logger_fd; struct buffer *stdin_buf = req->stdin_buf; bool const is_service = req->is_service; @@ -344,7 +345,7 @@ int qrexec_process_io(const struct process_io_request *req, if (prefix.len > 0 || (stdout_fd >= 0 && fds[FD_STDOUT].revents)) { switch (handle_input_v2( vchan, stdout_fd, stdout_msg_type, - &prefix, &remote_buffer)) { + &prefix, &remote_buffer, -1)) { case REMOTE_ERROR: if (!is_service && remote_status == -1) { /* Even if sending fails, still try to read remaining @@ -365,7 +366,7 @@ int qrexec_process_io(const struct process_io_request *req, if (stderr_fd >= 0 && fds[FD_STDERR].revents) { switch (handle_input_v2( vchan, stderr_fd, MSG_DATA_STDERR, - &empty, &remote_buffer)) { + &empty, &remote_buffer, dup_fd)) { case REMOTE_ERROR: handle_vchan_error("send(handle_input stderr)"); break; diff --git a/libqrexec/qrexec.h b/libqrexec/qrexec.h index a8fc06b3..ec0d44a2 100644 --- a/libqrexec/qrexec.h +++ b/libqrexec/qrexec.h @@ -163,7 +163,6 @@ enum { #define QREXEC_AGENT_TRIGGER_PATH "/var/run/qubes/qrexec-agent" #define QREXEC_AGENT_FDPASS_PATH "/var/run/qubes/qrexec-agent-fdpass" #define MEMINFO_WRITER_PIDFILE "/var/run/meminfo-writer.pid" -#define QUBES_RPC_MULTIPLEXER_PATH "/usr/lib/qubes/qubes-rpc-multiplexer" #define QREXEC_DAEMON_SOCKET_DIR "/var/run/qubes" #define QREXEC_POLICY_PROGRAM "/usr/bin/qrexec-policy-exec" #define QREXEC_SERVICE_PATH "/usr/local/etc/qubes-rpc:/etc/qubes-rpc" diff --git a/libqrexec/remote.c b/libqrexec/remote.c index 314bb8af..b831e7a6 100644 --- a/libqrexec/remote.c +++ b/libqrexec/remote.c @@ -150,7 +150,8 @@ int handle_remote_data_v2( int handle_input_v2( libvchan_t *vchan, int fd, int msg_type, struct prefix_data *prefix_data, - const struct buffer *buffer) + const struct buffer *buffer, + int dup_fd) { const size_t max_len = (size_t)buffer->buflen; char *buf = buffer->data; @@ -175,6 +176,8 @@ int handle_input_v2( prefix_data->len -= len; } else { len = read(fd, buf, len); + if (dup_fd != -1 && len > 0) + write_all(dup_fd, buf, (size_t)len); /* If the other side of the socket is a process that is already dead, * read from such socket could fail with ECONNRESET instead of * just 0. */ diff --git a/libqrexec/remote.h b/libqrexec/remote.h index 7d19f7a4..b6a42397 100644 --- a/libqrexec/remote.h +++ b/libqrexec/remote.h @@ -73,9 +73,12 @@ int handle_remote_data_v2( * initialized, and will _not_ be anything meaningful on return. The * buffer pointer and length will not be freed or reallocated, though. * In Rust terms: this is an &mut [MaybeUninit]. + * + * If out_fd is not -1, all data written is duplicated to it. */ int handle_input_v2( libvchan_t *vchan, int fd, int msg_type, struct prefix_data *prefix_data, - const struct buffer *buffer); + const struct buffer *buffer, + int out_fd); #pragma GCC visibility pop diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 5d21d22f..c2ec546c 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -113,7 +113,7 @@ def setUp(self): os.mkdir(os.path.join(self.tempdir, "rpc-config")) self.addCleanup(shutil.rmtree, self.tempdir) - def start_agent(self, fail_exec=False): + def start_agent(self): if self.agent is not None: return env = os.environ.copy() @@ -130,11 +130,6 @@ def start_agent(self, fail_exec=False): ] ) env["QUBES_RPC_CONFIG_PATH"] = os.path.join(self.tempdir, "rpc-config") - env["QREXEC_MULTIPLEXER_PATH"] = ( - "/dev/null" - if fail_exec - else os.path.join(ROOT_PATH, "lib", "qubes-rpc-multiplexer") - ) cmd = [ os.path.join(ROOT_PATH, "agent", "qrexec-agent"), "--no-fork-server", @@ -377,10 +372,8 @@ def trigger_service(self, dom0, client, target_domain_name, service_name): @unittest.skipIf(os.environ.get("SKIP_SOCKET_TESTS"), "socket tests not set up") class TestAgentExecQubesRpc(TestAgentBase): - def execute_qubesrpc( - self, service: str, src_domain_name: str, fail_exec: bool = False - ): - self.start_agent(fail_exec) + def execute_qubesrpc(self, service: str, src_domain_name: str): + self.start_agent() dom0 = self.connect_dom0() @@ -596,14 +589,12 @@ def test_exec_service_not_found(self): """ self._test_exec_service_fail(qrexec.QREXEC_EXIT_SERVICE_NOT_FOUND) - def test_exec_service_bad_multiplexer(self): + def test_exec_service_bad_service(self): """ - RPC multiplexer is junk + Service to execute is junk (-ENOEXEC from execve()) """ util.make_executable_service(self.tempdir, "rpc", "qubes.Service", "") - target, dom0 = self.execute_qubesrpc( - "qubes.Service+arg", "domX", fail_exec=True - ) + target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") target.send_message(qrexec.MSG_DATA_STDIN, b"") messages = util.sort_messages(target.recv_all_messages()) self.assertEqual(messages[0], (qrexec.MSG_DATA_STDOUT, b"")) diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index c7e3a1e9..4da11d5b 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -652,9 +652,6 @@ def start_client(self, args): os.path.join(self.tempdir, "rpc"), ] ) - env["QREXEC_MULTIPLEXER_PATH"] = os.path.join( - ROOT_PATH, "lib", "qubes-rpc-multiplexer" - ) env["QUBES_RPC_CONFIG_PATH"] = os.path.join(self.tempdir, "rpc-config") cmd = [ os.path.join(ROOT_PATH, "daemon", "qrexec-client"), @@ -1109,12 +1106,12 @@ def _test_run_dom0_service_failed( def test_run_dom0_service_3_args(self): self._test_run_dom0_service_failed( - exit_status=1, cmd="QUBESRPC qubes.Service+arg src_domain name" + cmd="QUBESRPC qubes.Service+arg src_domain name" ) def test_run_dom0_service_5_args(self): self._test_run_dom0_service_failed( - exit_status=1, cmd="QUBESRPC qubes.Service+arg src_domain name a b" + cmd="QUBESRPC qubes.Service+arg src_domain name a b" ) def test_run_dom0_service_not_found(self): diff --git a/selinux/qubes-core-qrexec.fc b/selinux/qubes-core-qrexec.fc index 2ffe77d3..d995881a 100644 --- a/selinux/qubes-core-qrexec.fc +++ b/selinux/qubes-core-qrexec.fc @@ -1 +1,3 @@ -/usr/lib/qubes/qrexec-agent -- gen_context(system_u:object_r:qubes_qrexec_agent_exec_t,s0) +/usr/lib/qubes/qrexec-agent -- gen_context(system_u:object_r:qubes_qrexec_agent_exec_t,s0) +/etc/qubes-rpc(/.*)? gen_context(system_u:object_r:bin_t,s0) +/usr/local/etc/qubes-rpc(/.*)? -- gen_context(system_u:object_r:bin_t,s0) From 1047b699e18ab2a59d3dd3afec7d4d31a109bc47 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 16 Apr 2024 19:47:44 -0400 Subject: [PATCH 11/13] Strip "QUBESRPC " prefix from service call commands 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". --- agent/qrexec-agent.c | 24 ++-- agent/qrexec-fork-server.c | 7 +- daemon/qrexec-client.c | 10 +- daemon/qrexec-daemon.c | 8 +- libqrexec/exec.c | 231 +++++++++++++++++------------------- libqrexec/libqrexec-utils.h | 21 ++-- 6 files changed, 150 insertions(+), 151 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index 16887660..1d87132b 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -172,8 +172,10 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) exit(1); } /* call QUBESRPC if requested */ - /* no point in creating a login shell for test environments */ - exec_qubes_rpc_if_requested2(prog, cmd, environ, false); + if (prog) { + /* no point in creating a login shell for test environmens */ + exec_qubes_rpc2(prog, cmd, environ, false); + } /* otherwise exec shell */ execl("/bin/sh", "sh", "-c", cmd, NULL); @@ -279,10 +281,11 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) if (retval == -1) warn("chdir(%s)", pw->pw_dir); - /* Call QUBESRPC if requested, using a login shell to set up - * environment variables. */ - exec_qubes_rpc_if_requested2(prog, cmd, env, true); - + /* call QUBESRPC if requested */ + if (prog) { + /* Set up environment variables for a login shell. */ + exec_qubes_rpc2(prog, cmd, env, true); + } /* otherwise exec shell */ execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env); _exit(QREXEC_EXIT_PROBLEM); @@ -318,10 +321,11 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) pam_end(pamh, PAM_ABORT); exit(1); #else - /* Call QUBESRPC if requested, using a login shell to set up - * environment variables. */ - exec_qubes_rpc_if_requested2(prog, cmd, environ, true); - + /* call QUBESRPC if requested */ + if (prog) { + /* Set up environment variables for a login session. */ + exec_qubes_rpc2(prog, cmd, environ, true); + } /* otherwise exec shell */ execl("/bin/su", "su", "-", user, "-c", cmd, NULL); PERROR("execl"); diff --git a/agent/qrexec-fork-server.c b/agent/qrexec-fork-server.c index d1a78d6f..34bc1fa4 100644 --- a/agent/qrexec-fork-server.c +++ b/agent/qrexec-fork-server.c @@ -44,8 +44,11 @@ void do_exec(const char *prog, const char *cmd, const char *user __attribute__(( signal(SIGCHLD, SIG_DFL); signal(SIGPIPE, SIG_DFL); - /* Call QUBESRPC if requested. This code already runs in a login session. */ - exec_qubes_rpc_if_requested2(prog, cmd, environ, false); + /* call QUBESRPC if requested */ + if (prog != NULL) { + /* Already in login session. */ + exec_qubes_rpc2(prog, cmd, environ, false); + } /* otherwise, pass it to shell */ shell = getenv("SHELL"); diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index 56b44d65..b29f4386 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -66,11 +66,13 @@ static _Noreturn void do_exec(const char *prog, const char *cmdline, const char *username __attribute__((unused))) { - /* Avoid calling RPC command through shell. - * Qrexec-client is always in a login session. */ - exec_qubes_rpc_if_requested2(prog, cmdline, environ, false); + /* avoid calling RPC service through shell */ + if (prog) { + /* qrexec-client is always in a login session. */ + exec_qubes_rpc2(prog, cmdline, environ, false); + } - /* 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 index fac3e209..9578fdec 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -1133,9 +1133,11 @@ static enum policy_response connect_daemon_socket( /* called from do_fork_exec */ static _Noreturn void do_exec(const char *prog, const char *cmd, const char *username __attribute__((unused))) { - /* Avoid calling RPC command through shell. - * Qrexec-daemon is always in a login session already. */ - exec_qubes_rpc_if_requested2(prog, cmd, environ, true); + /* avoid calling RPC command through shell */ + if (prog) { + /* qrexec-daemon is always in a login session already */ + exec_qubes_rpc2(prog, cmd, environ, false); + } /* 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 index ff836e86..01ba4b5b 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -36,6 +36,7 @@ #include #include #include + #include "qrexec.h" #include "libqrexec-utils.h" #include "private.h" @@ -59,135 +60,123 @@ static bool should_strip_env_var(const char *var) !STARTS_WITH_LITERAL(rest, "_AGENT_PID="); } -void exec_qubes_rpc_if_requested2(const char *program, const char *cmd, char *const envp[], - bool use_shell) { +void exec_qubes_rpc2(const char *program, const char *cmd, char *const envp[], + bool use_shell) { /* avoid calling RPC service through shell */ - if (program) { - assert(program); - char *prog_copy; - char *tok, *savetok; - const char *argv[7]; - size_t i = 0; + char *prog_copy; + char *tok, *savetok; + const char *argv[7]; + 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); + 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) +#define EXTEND_RAW(arg) \ + 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); } + } - 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) -#define EXTEND_RAW(arg) \ - 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); - } - } + prog_copy = strdup(cmd); + if (!prog_copy) { + PERROR("strdup"); + _exit(QREXEC_EXIT_PROBLEM); + } - prog_copy = strdup(cmd + RPC_REQUEST_COMMAND_LEN + 1); - if (!prog_copy) { - PERROR("strdup"); + argv[i++] = 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++] = 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 or not given, ignore - } - } else if (i == 3) { - EXTEND_RAW("QREXEC_REQUESTED_TARGET_TYPE="); + 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 { - 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; - if (p != NULL) { - EXTEND("QREXEC_SERVICE_ARGUMENT=%s", p + 1); - if (p[1] != '\0') { - argv[1] = p + 1; - argv[2] = NULL; - } + // requested target type unknown or not given, ignore } - assert(iterator <= env_amount); - buf[iterator] = NULL; - if (program[0] != '/') { - LOG(ERROR, "Program to execute not absolute path"); - _exit(QREXEC_EXIT_PROBLEM); - } - if (use_shell) { - /* Move the command to after 'sh -lc "exec \"\$@\"" sh ' */ - argv[4] = argv[0]; - argv[5] = argv[1]; - argv[6] = NULL; - /* Now fill in the shell invocation itelf */ - argv[0] = "sh"; - argv[1] = "-lc"; - /* Do not use "-lcexec \"$@\"": -c does not take an argument - * itself, and instead causes the first non-option argument - * to be interpreted as a command. */ - argv[2] = "exec \"$@\""; - argv[3] = "sh"; - execve("/bin/sh", (char *const *)argv, buf); - /* /bin/sh should always exist */ - _exit(QREXEC_EXIT_PROBLEM); - } else { - execve(program, (char *const *)argv, buf); - _exit(errno == ENOENT ? QREXEC_EXIT_SERVICE_NOT_FOUND : QREXEC_EXIT_PROBLEM); + } else if (i == 3) { + EXTEND_RAW("QREXEC_REQUESTED_TARGET_TYPE="); + } else { + 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; + if (p != NULL) { + EXTEND("QREXEC_SERVICE_ARGUMENT=%s", p + 1); + if (p[1] != '\0') { + argv[1] = p + 1; + argv[2] = NULL; } -bad_asprintf: - PERROR("asprintf"); + } + assert(iterator <= env_amount); + buf[iterator] = NULL; + if (program[0] != '/') { + LOG(ERROR, "Program to execute not absolute path"); + _exit(QREXEC_EXIT_PROBLEM); + } + if (use_shell) { + /* Move the command to after 'sh -lc "exec \"\$@\"" sh ' */ + argv[4] = argv[0]; + argv[5] = argv[1]; + argv[6] = NULL; + /* Now fill in the shell invocation itelf */ + argv[0] = "sh"; + argv[1] = "-lc"; + /* Do not use "-lcexec \"$@\"": -c does not take an argument + * itself, and instead causes the first non-option argument + * to be interpreted as a command. */ + argv[2] = "exec \"$@\""; + argv[3] = "sh"; + execve("/bin/sh", (char *const *)argv, buf); + /* /bin/sh should always exist */ _exit(QREXEC_EXIT_PROBLEM); } else { - assert(strncmp(cmd, RPC_REQUEST_COMMAND, RPC_REQUEST_COMMAND_LEN) != 0); + execve(program, (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) @@ -548,7 +537,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"); @@ -663,7 +654,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, @@ -786,7 +777,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); } @@ -846,7 +837,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 index 2390cc2c..6009b589 100644 --- a/libqrexec/libqrexec-utils.h +++ b/libqrexec/libqrexec-utils.h @@ -66,7 +66,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. */ @@ -144,23 +145,19 @@ __attribute__((visibility("default"))) void register_exec_func(do_exec_t *func); /** - * \param program Full path to program to execute or NULL. - * \param cmd RPC command, including "QUBESRPC " prefix, if *program* is not NULL. - * Otherwise *program* must be NULL and *cmd* must not start with "QUBESRPC". - * \param envp Environment passed to execve(), ignored if *program* is NULL. + * \param program Full path to program to execute. + * \param cmd RPC command, excluding "QUBESRPC " prefix. + * \param envp Environment passed to execve(). * \param use_shell If true, use a login shell to spawn the program. * - * If *program* is not NULL, execute it as an RPC service or call _exit() on failure. + * Execute *program* as an RPC service or call _exit() on failure. * *cmd* is used to set the argument (if any) and "QREXEC_*" environment variables. * Environment variables in *envp* that start with "QREXEC" are ignored, except for - * "QREXEC_SERVICE_PATH" and "QREXEC_AGENT_PID", which are inherited. If *program* - * is not NULL and *cmd* does not start with "QUBESRPC ", or if *program* is NULL - * and *cmd* starts with "QUBESRPC", fails an assertion. If *program* is NULL and - * *cmd* does not start with "QUBESRPC", returns without doing anything. + * "QREXEC_SERVICE_PATH" and "QREXEC_AGENT_PID", which are inherited. */ __attribute__((visibility("default"))) -void exec_qubes_rpc_if_requested2(const char *program, const char *cmd, char *const envp[], - bool use_shell); +_Noreturn void exec_qubes_rpc2(const char *program, const char *cmd, char *const envp[], + bool use_shell); /* Execute `qubes.WaitForSession` service, do not return on success, return -1 * (maybe setting errno) on failure. */ From a1b29937a77297d4cd329f68ac808a2a0bbe912d Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 10 Apr 2024 20:05:24 -0400 Subject: [PATCH 12/13] libqrexec SONAME bump The previous two changes were ABI breaks. --- debian/control | 6 +++--- debian/libqrexec-utils2.install | 1 - debian/libqrexec-utils2.shlibs | 1 - debian/libqrexec-utils4.install | 1 + debian/libqrexec-utils4.shlibs | 1 + libqrexec/Makefile | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) delete mode 100644 debian/libqrexec-utils2.install delete mode 100644 debian/libqrexec-utils2.shlibs create mode 100644 debian/libqrexec-utils4.install create mode 100644 debian/libqrexec-utils4.shlibs diff --git a/debian/control b/debian/control index 58669b3b..bc12ba57 100644 --- a/debian/control +++ b/debian/control @@ -19,7 +19,7 @@ Homepage: https://www.qubes-os.org Package: qubes-core-qrexec Architecture: any Depends: - libqrexec-utils2 (= ${binary:Version}), + libqrexec-utils4 (= ${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-utils2 +Package: libqrexec-utils4 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-utils2 (= ${binary:Version}), ${misc:Depends} +Depends: libqrexec-utils4 (= ${binary:Version}), ${misc:Depends} Breaks: qubes-utils (<< 3.1.4) Replaces: qubes-utils (<< 3.1.4) Description: Development headers for libqrexec-utils diff --git a/debian/libqrexec-utils2.install b/debian/libqrexec-utils2.install deleted file mode 100644 index 5f64a9a0..00000000 --- a/debian/libqrexec-utils2.install +++ /dev/null @@ -1 +0,0 @@ -usr/lib/libqrexec-utils.so.3* diff --git a/debian/libqrexec-utils2.shlibs b/debian/libqrexec-utils2.shlibs deleted file mode 100644 index f6a013f3..00000000 --- a/debian/libqrexec-utils2.shlibs +++ /dev/null @@ -1 +0,0 @@ -libqrexec-utils 3 libqrexec-utils2 (>= 4.1.0) diff --git a/debian/libqrexec-utils4.install b/debian/libqrexec-utils4.install new file mode 100644 index 00000000..3b2f712c --- /dev/null +++ b/debian/libqrexec-utils4.install @@ -0,0 +1 @@ +usr/lib/libqrexec-utils.so.4* diff --git a/debian/libqrexec-utils4.shlibs b/debian/libqrexec-utils4.shlibs new file mode 100644 index 00000000..68a934d8 --- /dev/null +++ b/debian/libqrexec-utils4.shlibs @@ -0,0 +1 @@ +libqrexec-utils 4 libqrexec-utils4 (>=4.2.18) diff --git a/libqrexec/Makefile b/libqrexec/Makefile index bd9bcb8c..6d95c040 100644 --- a/libqrexec/Makefile +++ b/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=3 +SO_VER=4 VCHANLIBS := $(shell pkg-config --libs vchan) LIBDIR ?= /usr/lib INCLUDEDIR ?= /usr/include From 51460e6fb843e565f22b8eef1fa43d84133217c3 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 17 Jan 2025 15:56:14 -0500 Subject: [PATCH 13/13] Avoid qubes-rpc-multiplexer for dom0 -> dom0 calls Instead, just use qrexec-client, as with any other service call. --- daemon/qrexec-client.c | 85 ++++++++++++++++++++++++++++------- daemon/qrexec-daemon-common.c | 2 +- daemon/qrexec-daemon-common.h | 4 ++ libqrexec/exec.c | 16 ++++++- qrexec/client.py | 6 --- qrexec/tests/socket/daemon.py | 61 ++++++++++++++++++++++++- 6 files changed, 150 insertions(+), 24 deletions(-) diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index b29f4386..c55ce098 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -201,6 +201,10 @@ int main(int argc, char **argv) usage(argv[0], 1); } parse_connect(optarg, &request_id, &src_domain_name, &src_domain_id); + if (target_refers_to_dom0(src_domain_name) || src_domain_id == 0) { + warnx("ERROR: -c cannot be used for requests to dom0"); + usage(argv[0], 1); + } break; case 't': replace_chars_stdout = true; @@ -261,22 +265,73 @@ int main(int argc, char **argv) } if (target_refers_to_dom0(domname)) { - if (request_id == NULL) { - fprintf(stderr, "ERROR: when target domain is 'dom0', -c must be specified\n"); - usage(argv[0], 1); - } - strncpy(svc_params.ident, request_id, sizeof(svc_params.ident) - 1); - svc_params.ident[sizeof(svc_params.ident) - 1] = '\0'; - if (src_domain_name == NULL) { - LOG(ERROR, "internal error: src_domain_name should not be NULL here"); - abort(); + if (request_id != NULL) { + if (request_id[0] == '\0') { + warnx("ERROR: request ID cannot be empty"); + usage(argv[0], 1); + } + strncpy(svc_params.ident, request_id, sizeof(svc_params.ident) - 1); + svc_params.ident[sizeof(svc_params.ident) - 1] = '\0'; + if (src_domain_name == NULL) { + LOG(ERROR, "internal error: src_domain_name should not be NULL here"); + abort(); + } + rc = run_qrexec_to_dom0(&svc_params, + src_domain_id, + src_domain_name, + remote_cmdline, + connection_timeout, + exit_with_code); + } else { + /* dom0 -> dom0 fake service call */ + assert(src_domain_id == 0); + /* + * Normally calls to dom0 omit the username, but in this case + * that would require the caller to pass the user if and only if the target is _not_ + * dom0, and that's annoying. In the past, qrexec-client was called by qrexec-daemon + * which got it right, but now the main caller of qrexec-client is Python scripts + * that don't have access to the C target_refers_to_dom0() function. + * Therefore, parse the username and fail if it is not "DEFAULT". + */ +#define DEFAULT_USER "DEFAULT" + if (strncmp(remote_cmdline, DEFAULT_USER ":", sizeof(DEFAULT_USER)) != 0) { + errx(QREXEC_EXIT_PROBLEM, "dom0 -> dom0 commands must be prefixed with " DEFAULT_USER ":"); + } + remote_cmdline += sizeof(DEFAULT_USER); + struct qrexec_parsed_command *command = parse_qubes_rpc_command(remote_cmdline, false); + int prepare_ret; + char file_path[QUBES_SOCKADDR_UN_MAX_PATH_LEN]; + struct buffer buf = { .data = file_path, .buflen = (int)sizeof(file_path) }; + if (command == NULL) { + prepare_ret = -2; + } else if (command->service_descriptor == NULL) { + LOG(ERROR, "For dom0 -> dom0 commands, only proper qrexec calls are allowed"); + prepare_ret = -2; + } else if (!wait_for_session_maybe(command)) { + LOG(ERROR, "Cannot load service configuration, or forking process failed"); + prepare_ret = -2; + } else { + prepare_ret = find_qrexec_service(command, NULL, NULL, &buf); + } + switch (prepare_ret) { + case -2: + rc = QREXEC_EXIT_PROBLEM; + break; + case -1: + rc = QREXEC_EXIT_SERVICE_NOT_FOUND; + break; + case 0: + assert(command->username == NULL); + assert(command->command); + /* qrexec-client is always in a login session. */ + exec_qubes_rpc2(buf.data, command->command, environ, false); + /* not reached */ + default: + assert(false); + rc = QREXEC_EXIT_PROBLEM; + break; + } } - rc = run_qrexec_to_dom0(&svc_params, - src_domain_id, - src_domain_name, - remote_cmdline, - connection_timeout, - exit_with_code); } else { if (request_id) { bool const use_uuid = strncmp(domname, "uuid:", 5) == 0; diff --git a/daemon/qrexec-daemon-common.c b/daemon/qrexec-daemon-common.c index b465a54d..7bce924e 100644 --- a/daemon/qrexec-daemon-common.c +++ b/daemon/qrexec-daemon-common.c @@ -364,7 +364,7 @@ static void sigchld_handler(int x __attribute__((__unused__))) } /* See also qrexec-agent.c:wait_for_session_maybe() */ -static bool wait_for_session_maybe(struct qrexec_parsed_command *cmd) +bool wait_for_session_maybe(struct qrexec_parsed_command *cmd) { pid_t pid; int status; diff --git a/daemon/qrexec-daemon-common.h b/daemon/qrexec-daemon-common.h index 02b36397..335888d2 100644 --- a/daemon/qrexec-daemon-common.h +++ b/daemon/qrexec-daemon-common.h @@ -147,3 +147,7 @@ bool qrexec_execute_vm(const char *target, bool autostart, int remote_domain_id, extern int local_stdin_fd; __attribute__((warn_unused_result)) bool target_refers_to_dom0(const char *target); + +/** Wait for a session if needed. */ +__attribute__((warn_unused_result)) +bool wait_for_session_maybe(struct qrexec_parsed_command *cmd); diff --git a/libqrexec/exec.c b/libqrexec/exec.c index 01ba4b5b..f0fb8ee2 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -62,6 +62,7 @@ static bool should_strip_env_var(const char *var) void exec_qubes_rpc2(const char *program, const char *cmd, char *const envp[], bool use_shell) { + assert(cmd); /* avoid calling RPC service through shell */ char *prog_copy; char *tok, *savetok; @@ -743,7 +744,9 @@ int find_qrexec_service( const char *qrexec_service_path = getenv("QREXEC_SERVICE_PATH"); if (!qrexec_service_path) qrexec_service_path = QREXEC_SERVICE_PATH; - *socket_fd = -1; + if (socket_fd != NULL) { + *socket_fd = -1; + } struct stat statbuf; @@ -764,6 +767,9 @@ int find_qrexec_service( if (S_ISSOCK(statbuf.st_mode)) { /* Socket-based service. */ + if (socket_fd == NULL) { + goto bad_socket_service; + } int s; if ((s = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0)) == -1) { PERROR("socket"); @@ -784,6 +790,9 @@ int find_qrexec_service( *socket_fd = s; return 0; } else if (S_ISLNK(statbuf.st_mode)) { + if (socket_fd == NULL) { + goto bad_socket_service; + } /* TCP-based service */ assert(path_buffer->buflen >= (int)sizeof("/dev/tcp") - 1); assert(memcmp(path_buffer->data, "/dev/tcp", sizeof("/dev/tcp") - 1) == 0); @@ -873,6 +882,11 @@ int find_qrexec_service( LOG(ERROR, "Unknown service type (not executable, not a socket): %.*s", path_buffer->buflen, path_buffer->data); return -2; +bad_socket_service: + LOG(ERROR, "Service %.*s is a socket or /dev/tcp symlink, but this is not " + "supported for dom0 -> dom0 calls", + path_buffer->buflen, path_buffer->data); + return -2; } int exec_wait_for_session(const char *source_domain) { diff --git a/qrexec/client.py b/qrexec/client.py index dca84294..33ea8044 100644 --- a/qrexec/client.py +++ b/qrexec/client.py @@ -24,7 +24,6 @@ QREXEC_CLIENT_DOM0 = "/usr/bin/qrexec-client" QREXEC_CLIENT_VM = "/usr/bin/qrexec-client-vm" -RPC_MULTIPLEXER = "/usr/lib/qubes/qubes-rpc-multiplexer" VERSION = None @@ -107,11 +106,6 @@ def make_command(dest, rpcname, arg): else: rpcname += "+" - if VERSION == "dom0" and dest == "dom0": - # Invoke qubes-rpc-multiplexer directly. This will work for non-socket - # services only. - return [RPC_MULTIPLEXER, rpcname, "dom0"] - if VERSION == "dom0": return [ QREXEC_CLIENT_DOM0, diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index 4da11d5b..eb1e1d62 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -641,7 +641,7 @@ def setUp(self): def make_executable_service(self, *args): util.make_executable_service(self.tempdir, *args) - def start_client(self, args): + def start_client(self, args, stderr=None): env = os.environ.copy() env["LD_LIBRARY_PATH"] = os.path.join(ROOT_PATH, "libqrexec") env["VCHAN_DOMAIN"] = "0" @@ -664,6 +664,7 @@ def start_client(self, args): env=env, stdin=subprocess.PIPE, stdout=subprocess.PIPE, + stderr=stderr, ) self.addCleanup(self.stop_client) @@ -1020,6 +1021,64 @@ def assertExpectedStdout( ], ) + def _test_run_service_in_dom0(self, requested_target): + util.make_executable_service( + self.tempdir, + "rpc", + "qubes.Service", + """\ +#!/bin/sh -eu +read input +echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN, input: $input, service path: $QREXEC_SERVICE_PATH" +case $QREXEC_REQUESTED_TARGET_TYPE in +(name) echo "requested target name: $QREXEC_REQUESTED_TARGET";; +(keyword) echo "requested target keyword: $QREXEC_REQUESTED_TARGET_KEYWORD";; +esac +""", + ) + requested_target_type = "name" + if requested_target.startswith("@"): + requested_target = requested_target[1:] + requested_target_type = "keyword" + + cmd = f"DEFAULT:QUBESRPC qubes.Service+arg src_domain {requested_target_type} {requested_target}" + self.start_client(["-d", "dom0", cmd], stderr=subprocess.PIPE) + stdout, stderr = self.client.communicate(b"stdin data\n") + self.client.wait() + self.assertFalse(stderr, stderr) + self.assertEqual(stderr, b"") + self.assertEqual(self.client.returncode, 0) + service_path = ( + ":".join( + [ + os.path.join(self.tempdir, "local-rpc"), + os.path.join(self.tempdir, "rpc"), + ] + ) + ).encode("ascii", "strict") + self.assertEqual( + stdout, + b"arg: arg, remote domain: src_domain, " + b"input: stdin data, service path: %s\n" + b"requested target %s: %s\n" + % ( + service_path, + requested_target_type.encode("ascii", "strict"), + requested_target.encode("ascii", "strict"), + ), + ) + + def test_dom0_to_dom0(self): + self._test_run_service_in_dom0("@adminvm") + + def test_dom0_to_dom0_v1(self): + self._test_run_service_in_dom0("dom0") + + def test_dom0_to_dom0_v2(self): + self._test_run_service_in_dom0( + "uuid:00000000-0000-0000-0000-000000000000" + ) + def _test_run_dom0_service_exec(self, nogui, requested_target="src_domain"): util.make_executable_service( self.tempdir,