From f74981f3cb20a223a9059935937a70fa5b6801f8 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 4 Apr 2024 17:11:16 -0400 Subject: [PATCH 01/15] Add test for broken symbolic links as services It currently fails (QubesOS/qubes-issues#9089). --- qrexec/tests/socket/agent.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 07cef783..cf36a63e 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -525,6 +525,31 @@ def test_exec_service_with_arg(self): ) self.check_dom0(dom0) + @unittest.expectedFailure + def test_exec_broken_specific_service(self): + os.symlink("/dev/null/invalid", + os.path.join(self.tempdir, "rpc", "qubes.Service+arg")) + self.make_executable_service( + "rpc", + "qubes.Service", + """\ +#!/bin/sh +echo "general service" +""", + ) + target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") + target.send_message(qrexec.MSG_DATA_STDIN, b"") + messages = target.recv_all_messages() + self.assertListEqual( + util.sort_messages(messages), + [ + (qrexec.MSG_DATA_STDOUT, b""), + (qrexec.MSG_DATA_STDERR, b""), + (qrexec.MSG_DATA_EXIT_CODE, b"\177\0\0\0"), + ], + ) + self.check_dom0(dom0) + def test_connect_socket_no_metadata(self): socket_path = os.path.join( self.tempdir, "rpc", "qubes.SocketService+arg2" From d5d71842fbd79c3673667965b4a1b3984594288b Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 3 Apr 2024 20:24:54 -0400 Subject: [PATCH 02/15] find_file(): Check for broken symlinks and I/O errors Previously these were ignored. Treat them as errors instead and terminate the search. Fixes: QubesOS/qubes-issues#9089 --- libqrexec/exec.c | 28 +++++++++++++++++++++++----- qrexec/tests/socket/agent.py | 1 - 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/libqrexec/exec.c b/libqrexec/exec.c index 0825f49d..103ecad5 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -212,7 +212,9 @@ static int execute_qrexec_service( /* Find a file in the ':'-delimited list of paths given in path_list. - Returns 0 on success, -1 on failure. + Returns 0 on success, -1 if the file is definitely absent in all of the + paths, and -2 on error (broken symlink, I/O error, path too long, out + of memory, etc). On success, fills buffer and statbuf (unless statbuf is NULL). */ static int find_file( @@ -225,6 +227,10 @@ static int find_file( struct stat dummy_buf; const char *path_start = path_list; size_t name_length = strlen(name); + int res; + + if (name_length > NAME_MAX) + return -1; /* cannot possibly exist */ if (!statbuf) statbuf = &dummy_buf; @@ -236,15 +242,27 @@ static int find_file( if (path_length + name_length + 1 >= buffer_size) { LOG(ERROR, "find_qrexec_service_file: buffer too small for file path"); - return -1; + return -2; } memcpy(buffer, path_start, path_length); buffer[path_length] = '/'; strcpy(buffer + path_length + 1, name); //LOG(INFO, "stat(%s)", buffer); - if (stat(buffer, statbuf) == 0) + res = lstat(buffer, statbuf); + if (res == 0 && S_ISLNK(statbuf->st_mode)) { + /* check if the symlink is valid */ + res = stat(buffer, statbuf); + assert(res == -1 || !S_ISLNK(statbuf->st_mode)); + } + if (res == 0) { return 0; + } else { + assert(res == -1); + if (errno != ENOENT) { + return -2; + } + } path_start = path_end; while (*path_start == ':') @@ -264,7 +282,7 @@ static int load_service_config_raw(struct qrexec_parsed_command *cmd, int ret = find_file(config_path, cmd->service_descriptor, config_full_path, sizeof(config_full_path), NULL); - if (ret < 0 && strcmp(cmd->service_descriptor, cmd->service_name) != 0) + if (ret == -1 && strcmp(cmd->service_descriptor, cmd->service_name) != 0) ret = find_file(config_path, cmd->service_name, config_full_path, sizeof(config_full_path), NULL); if (ret < 0) @@ -462,7 +480,7 @@ static int execute_qrexec_service( int ret = find_file(qrexec_service_path, cmd->service_descriptor, service_full_path, sizeof(service_full_path), &statbuf); - if (ret < 0 && strcmp(cmd->service_descriptor, cmd->service_name) != 0) + if (ret == -1 && strcmp(cmd->service_descriptor, cmd->service_name) != 0) ret = find_file(qrexec_service_path, cmd->service_name, service_full_path, sizeof(service_full_path), &statbuf); diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index cf36a63e..29ffbce2 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -525,7 +525,6 @@ def test_exec_service_with_arg(self): ) self.check_dom0(dom0) - @unittest.expectedFailure def test_exec_broken_specific_service(self): os.symlink("/dev/null/invalid", os.path.join(self.tempdir, "rpc", "qubes.Service+arg")) From 7f784417622e57590094341117bf8554b20a1b13 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 4 Apr 2024 19:38:03 -0400 Subject: [PATCH 03/15] Add test for unsetting QREXEC_* variables QREXEC_SERVICE_ARGUMENT, QREXEC_REQUESTED_TARGET, and QREXEC_REQUESTED_TARGET_KEYWORD should not be in the environment of the child process unless explicitly set by the qrexec call. The current code fails to unset them. --- qrexec/tests/socket/agent.py | 36 ++++++++++++++++++++++++++++++++++++ run-tests | 3 +-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 29ffbce2..587dc038 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -58,6 +58,9 @@ def start_agent(self): env["LD_LIBRARY_PATH"] = os.path.join(ROOT_PATH, "libqrexec") env["VCHAN_DOMAIN"] = str(self.domain) env["VCHAN_SOCKET_DIR"] = self.tempdir + env["QREXEC_SERVICE_ARGUMENT"] = "%did_not_get_unset" + env["QREXEC_REQUESTED_TARGET_KEYWORD"] = "%did_not_get_unset" + env["QREXEC_REQUESTED_TARGET"] = "%did_not_get_unset" env["QREXEC_SERVICE_PATH"] = ":".join( [ os.path.join(self.tempdir, "local-rpc"), @@ -341,6 +344,39 @@ def test_exec_service(self): ], ) + @unittest.expectedFailure + def test_exec_service_keyword(self): + util.make_executable_service( + self.tempdir, + "rpc", + "qubes.Service", + """\ +#!/bin/sh -e +printf %s\\\\n "arg: ${1+bad}, remote domain: $QREXEC_REMOTE_DOMAIN" \ +"target name: ${QREXEC_REQUESTED_TARGET-NONAME}" \ +"target keyword: ${QREXEC_REQUESTED_TARGET_KEYWORD-NOKEYWORD}" \ +${QREXEC_REQUESTED_TARGET_TYPE+"target type: '${QREXEC_REQUESTED_TARGET_TYPE}'"} \ +${QREXEC_SERVICE_ARGUMENT+"call argument: '${QREXEC_SERVICE_ARGUMENT}'"} +""", + ) + target, dom0 = self.execute_qubesrpc("qubes.Service", "domX") + target.send_message(qrexec.MSG_DATA_STDIN, b"") + messages = target.recv_all_messages() + self.assertListEqual( + util.sort_messages(messages), + [ + (qrexec.MSG_DATA_STDOUT, b"""arg: , remote domain: domX +target name: NONAME +target keyword: NOKEYWORD +target type: '' +"""), + (qrexec.MSG_DATA_STDOUT, b""), + (qrexec.MSG_DATA_STDERR, b""), + (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), + ], + ) + self.check_dom0(dom0) + def test_exec_service_with_config(self): util.make_executable_service( self.tempdir, diff --git a/run-tests b/run-tests index 6b339cba..33e2cffc 100755 --- a/run-tests +++ b/run-tests @@ -25,8 +25,7 @@ else export SKIP_SOCKET_TESTS=1 fi -set -x - if [[ "$#" = 0 ]]; then set -- -v qrexec/tests; fi +set -x python3 -m coverage run -m pytest -o 'python_files=*.py' "$@" From c0e7f6a55e7e58d589a39ed41a171a9f4f26b586 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 4 Apr 2024 19:38:03 -0400 Subject: [PATCH 04/15] Explicitly unset QREXEC_ variables QREXEC_SERVICE_ARGUMENT, QREXEC_REQUESTED_TARGET and QREXEC_REQUESTED_TARGET_KEYWORD should not be in the environment of the child process unless explicitly set by the qrexec call. Explicitly unset them. Also avoid relying on QREXEC_SERVICE_ARGUMENT not containing glob characters or characters in $IFS. Commands sent from a VM cannot have them due to the sanitization in qrexec-daemon, but commands sent from dom0 could. Fixes: QubesOS/qubes-issues#9091 --- lib/qubes-rpc-multiplexer | 5 +++-- qrexec/tests/socket/agent.py | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/qubes-rpc-multiplexer b/lib/qubes-rpc-multiplexer index ae1122f0..0eecec24 100755 --- a/lib/qubes-rpc-multiplexer +++ b/lib/qubes-rpc-multiplexer @@ -22,6 +22,8 @@ if ! [ $# = 2 -o $# = 4 ] ; then echo "$0: bad argument count, usage: $0 SERVICE-NAME REMOTE-DOMAIN-NAME [REQUESTED_TARGET_TYPE REQUESTED_TARGET]" >&2 exit 1 fi +# Avoid inheriting these from the environment +unset QREXEC_REQUESTED_TARGET QREXEC_REQUESTED_TARGET_KEYWORD QREXEC_SERVICE_ARGUMENT export QREXEC_REQUESTED_TARGET_TYPE="$3" if [ "$QREXEC_REQUESTED_TARGET_TYPE" = "name" ]; then export QREXEC_REQUESTED_TARGET="$4" @@ -51,7 +53,6 @@ for DIR in $QREXEC_SERVICE_PATH; do done IFS=$ifs -# shellcheck disable=SC2086 -exec "$CFG_FILE" ${QREXEC_SERVICE_ARGUMENT} +exec "$CFG_FILE" ${QREXEC_SERVICE_ARGUMENT:+"$QREXEC_SERVICE_ARGUMENT"} echo "$0: failed to execute handler for $1" >&2 exit 1 diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 587dc038..46815936 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -344,7 +344,6 @@ def test_exec_service(self): ], ) - @unittest.expectedFailure def test_exec_service_keyword(self): util.make_executable_service( self.tempdir, From b16e0a54f24e2368004ed0d1c3df48646163bea1 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 4 Apr 2024 17:42:56 -0400 Subject: [PATCH 05/15] Add test for missing service arguments The test currently fails due to QubesOS/qubes-issues#9090. The test for socket services will be added along with the fix, as without the fix it hangs. --- qrexec/tests/socket/agent.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 46815936..0f93c5ee 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -26,6 +26,7 @@ import struct import getpass import itertools +import asyncio import psutil import pytest @@ -584,6 +585,38 @@ def test_exec_broken_specific_service(self): ) self.check_dom0(dom0) + @unittest.expectedFailure + def test_exec_null_argument_finds_service_for_empty_argument(self): + self.make_executable_service( + "local-rpc", + "qubes.Service+", + """\ +#!/bin/sh -- +echo "specific service: $QREXEC_SERVICE_FULL_NAME" +""", + ) + self.make_executable_service( + "rpc", + "qubes.Service", + """\ +#!/bin/sh +echo "general service" +""", + ) + target, dom0 = self.execute_qubesrpc("qubes.Service", "domX") + target.send_message(qrexec.MSG_DATA_STDIN, b"") + messages = target.recv_all_messages() + self.assertListEqual( + util.sort_messages(messages), + [ + (qrexec.MSG_DATA_STDOUT, b"specific service: qubes.Service\n"), + (qrexec.MSG_DATA_STDOUT, b""), + (qrexec.MSG_DATA_STDERR, b""), + (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), + ], + ) + self.check_dom0(dom0) + def test_connect_socket_no_metadata(self): socket_path = os.path.join( self.tempdir, "rpc", "qubes.SocketService+arg2" From 2a302d88cce0d7e19577a5f930a0c1f3dea50bf4 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 3 Apr 2024 21:50:38 -0400 Subject: [PATCH 06/15] Search for qubes.Service+ if call for qubes.Service is made Previously, qubes.Service+ was searched for if the call was from a VM, but not if the call was from dom0. Furthermore, a call from dom0 with no argument would skip the check for a too-long service name. In the future, service arguments should be required and not passing one should be treated as an error. That's for R5.0, though: there is too much code in dom0 that depends on the current behavior, and Mirage also depends on receiving a call with no argument at all. QREXEC_SERVICE_FULL_NAME (for executable services) and the call metadata (for socket services) still include the actual command, even if the command does not include "+". Therefore, code that needs to differentiate between "no argument" and "empty argument" can do so for calls from dom0. Fixes: QubesOS/qubes-issues#9090 --- lib/qubes-rpc-multiplexer | 3 ++ libqrexec/exec.c | 83 +++++++++++++++++++----------------- qrexec/tests/socket/agent.py | 37 +++++++++++++++- 3 files changed, 84 insertions(+), 39 deletions(-) diff --git a/lib/qubes-rpc-multiplexer b/lib/qubes-rpc-multiplexer index 0eecec24..eea0ca30 100755 --- a/lib/qubes-rpc-multiplexer +++ b/lib/qubes-rpc-multiplexer @@ -36,6 +36,9 @@ export QREXEC_SERVICE_FULL_NAME="$1" SERVICE_WITHOUT_ARGUMENT="${1%%+*}" if [ "${QREXEC_SERVICE_FULL_NAME}" != "${SERVICE_WITHOUT_ARGUMENT}" ]; then export QREXEC_SERVICE_ARGUMENT="${QREXEC_SERVICE_FULL_NAME#*+}" +else + # Search for qubes.Service+ if given qubes.Service + set -- "$1+" "$2" fi diff --git a/libqrexec/exec.c b/libqrexec/exec.c index 103ecad5..df65a68e 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -282,7 +282,7 @@ static int load_service_config_raw(struct qrexec_parsed_command *cmd, int ret = find_file(config_path, cmd->service_descriptor, config_full_path, sizeof(config_full_path), NULL); - if (ret == -1 && strcmp(cmd->service_descriptor, cmd->service_name) != 0) + if (ret == -1) ret = find_file(config_path, cmd->service_name, config_full_path, sizeof(config_full_path), NULL); if (ret < 0) @@ -311,6 +311,21 @@ int load_service_config(struct qrexec_parsed_command *cmd, return rc; } +/* Duplicates a buffer and adds a NUL terminator. + * Same as strndup(), except that it logs on failure (with PERROR()) + * and always copies exactly "len" bytes, even if some of them are NUL + * bytes. This guarantees that the output buffer is of the expected + * length and saves an unneeded call to strnlen(). */ +static char* memdupnul(const char *ptr, size_t len) { + char *buf = malloc(len + 1); + if (buf == NULL) { + PERROR("malloc"); + return NULL; + } + memcpy(buf, ptr, len); + buf[len] = '\0'; + return buf; +} struct qrexec_parsed_command *parse_qubes_rpc_command( const char *cmdline, bool strip_username) { @@ -332,11 +347,9 @@ struct qrexec_parsed_command *parse_qubes_rpc_command( LOG(ERROR, "Bad command from dom0 (%s): no colon", cmdline); goto err; } - cmd->username = strndup(cmdline, (size_t)(colon - cmdline)); - if (!cmd->username) { - PERROR("strndup"); + cmd->username = memdupnul(cmdline, (size_t)(colon - cmdline)); + if (!cmd->username) goto err; - } cmd->command = colon + 1; } else cmd->command = cmdline; @@ -361,53 +374,47 @@ struct qrexec_parsed_command *parse_qubes_rpc_command( goto err; } - if (end - start > MAX_SERVICE_NAME_LEN) { - LOG(ERROR, "Command too long (length %zu)", - end - start); + if (end <= start) { + LOG(ERROR, "Service descriptor is empty (too many spaces after QUBESRPC?)"); goto err; } - cmd->service_descriptor = strndup(start, end - start); - if (!cmd->service_descriptor) { - PERROR("strndup"); + size_t const descriptor_len = (size_t)(end - start); + if (descriptor_len > MAX_SERVICE_NAME_LEN) { + LOG(ERROR, "Command too long (length %zu)", descriptor_len); goto err; } /* Parse service name ("qubes.Service") */ - const char *plus = memchr(start, '+', end - start); - if (plus) { - if (plus - start == 0) { - LOG(ERROR, "Service name empty"); - goto err; - } - if (plus - start > NAME_MAX) { - LOG(ERROR, "Service name too long to execute (length %zu)", - plus - start); - goto err; - } - cmd->service_name = strndup(start, plus - start); - if (!cmd->service_name) { - PERROR("strndup"); - goto err; - } - } else { - cmd->service_name = strndup(start, end - start); - if (!cmd->service_name) { - PERROR("strdup"); - goto err; - } + const char *const plus = memchr(start, '+', descriptor_len); + size_t const name_len = plus != NULL ? (size_t)(plus - start) : descriptor_len; + if (name_len > NAME_MAX) { + LOG(ERROR, "Service name too long to execute (length %zu)", name_len); + goto err; + } + if (name_len < 1) { + LOG(ERROR, "Service name empty"); + goto err; } + cmd->service_name = memdupnul(start, name_len); + if (!cmd->service_name) + goto err; + + /* If there is no service argument, add a trailing "+" to the descriptor */ + cmd->service_descriptor = memdupnul(start, descriptor_len + (plus == NULL)); + if (!cmd->service_descriptor) + goto err; + if (plus == NULL) + cmd->service_descriptor[descriptor_len] = '+'; /* Parse source domain */ start = end + 1; /* after the space */ end = strchrnul(start, ' '); - cmd->source_domain = strndup(start, end - start); - if (!cmd->source_domain) { - PERROR("strndup"); + cmd->source_domain = memdupnul(start, (size_t)(end - start)); + if (!cmd->source_domain) goto err; - } } return cmd; @@ -480,7 +487,7 @@ static int execute_qrexec_service( int ret = find_file(qrexec_service_path, cmd->service_descriptor, service_full_path, sizeof(service_full_path), &statbuf); - if (ret == -1 && strcmp(cmd->service_descriptor, cmd->service_name) != 0) + if (ret == -1) ret = find_file(qrexec_service_path, cmd->service_name, service_full_path, sizeof(service_full_path), &statbuf); diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 0f93c5ee..9490000c 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -585,7 +585,6 @@ def test_exec_broken_specific_service(self): ) self.check_dom0(dom0) - @unittest.expectedFailure def test_exec_null_argument_finds_service_for_empty_argument(self): self.make_executable_service( "local-rpc", @@ -617,6 +616,42 @@ def test_exec_null_argument_finds_service_for_empty_argument(self): ) self.check_dom0(dom0) + def test_socket_null_argument_finds_service_for_empty_argument(self): + good_socket_path = os.path.join( + self.tempdir, "rpc", "qubes.SocketService+" + ) + bad_socket_path = os.path.join( + self.tempdir, "rpc", "qubes.SocketService" + ) + good_server = qrexec.socket_server(good_socket_path) + self.addCleanup(good_server.close) + bad_server = qrexec.socket_server(bad_socket_path) + self.addCleanup(bad_server.close) + + target, dom0 = self.execute_qubesrpc("qubes.SocketService", "domX") + + good_server.accept() + + message = b"stdin data" + target.send_message(qrexec.MSG_DATA_STDIN, message) + target.send_message(qrexec.MSG_DATA_STDIN, b"") + expected = b"qubes.SocketService domX\0" + message + self.assertEqual(good_server.recvall(len(expected)), expected) + + good_server.sendall(b"stdout data") + good_server.close() + messages = target.recv_all_messages() + # No stderr + self.assertListEqual( + util.sort_messages(messages), + [ + (qrexec.MSG_DATA_STDOUT, b"stdout data"), + (qrexec.MSG_DATA_STDOUT, b""), + (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), + ], + ) + self.check_dom0(dom0) + def test_connect_socket_no_metadata(self): socket_path = os.path.join( self.tempdir, "rpc", "qubes.SocketService+arg2" From a8052a3794c60c061cdfc4cd75e93dfe013dcbe4 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 4 Apr 2024 21:35:50 -0400 Subject: [PATCH 07/15] Add test for invalid service name for old protocol version This would have previously been allowed. The test currently fails. Also add various tests for the modern protocol version, which all pass, and a test for QSB-089. --- qrexec/tests/socket/daemon.py | 89 +++++++++++++++++++++++++++++++++++ qrexec/tests/socket/qrexec.py | 1 + 2 files changed, 90 insertions(+) diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index 73785d1b..417d985f 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -203,6 +203,95 @@ def test_bad_request_id_bad_byte_after_nul_terminator(self): self.assertEqual(data[:4], b"a\0b\0") self.assertEqual(data[4:], b"\0" * 28) + @unittest.expectedFailure + def test_bad_old_style_request(self): + agent = self.start_daemon_with_agent() + agent.send_message(qrexec.MSG_HELLO, struct.pack(" Date: Thu, 4 Apr 2024 15:55:01 -0400 Subject: [PATCH 08/15] Forbid empty service names in legacy MSG_TRIGGER_SERVICE There is no point in allowing calls with an empty service name: if the argument was empty, the call would be unconditionally rejected by the policy daemon, and even if it was _not_ empty, libqrexec would refuse to execute the call. Furthermore, MSG_TRIGGER_SERVICE3 already checks for empty service names and sends MSG_SERVICE_REFUSED without even querying the policy daemon, so querying the policy daemon for MSG_TRIGGER_SERVICE is inconsistent. Fixes: QubesOS/qubes-issues#9098 --- daemon/qrexec-daemon.c | 57 +++++++++++++++++++++-------------- qrexec/tests/socket/daemon.py | 1 - 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index 28c3cabe..6da80767 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -1262,6 +1262,21 @@ static bool validate_request_id(struct service_params *untrusted_params, const c #define ENSURE_NULL_TERMINATED(x) x[sizeof(x)-1] = 0 +static bool validate_service_name(char *untrusted_service_name) +{ + switch (untrusted_service_name[0]) { + case '\0': + LOG(ERROR, "Empty service name not allowed"); + return false; + case '+': + LOG(ERROR, "Service name must not start with '+'"); + return false; + default: + sanitize_name(untrusted_service_name, "+"); + return true; + } +} + #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION static #endif @@ -1296,6 +1311,10 @@ void handle_message_from_agent(void) send_service_refused(vchan, &untrusted_params.request_id); return; } + if (!validate_service_name(untrusted_params.service_name)) { + send_service_refused(vchan, &untrusted_params.request_id); + return; + } params = untrusted_params; /* sanitize end */ @@ -1329,38 +1348,30 @@ void handle_message_from_agent(void) ENSURE_NULL_TERMINATED(untrusted_params3.target_domain); sanitize_name(untrusted_params3.target_domain, "@:"); if (!validate_request_id(&untrusted_params3.request_id, "MSG_TRIGGER_SERVICE3")) - goto fail; + goto fail3; params3 = untrusted_params3; if (untrusted_service_name[service_name_len] != 0) { LOG(ERROR, "Service name not NUL-terminated"); - goto fail; + goto fail3; } nul_offset = strlen(untrusted_service_name); if (nul_offset != service_name_len) { LOG(ERROR, "Service name contains NUL byte at offset %zu", nul_offset); - goto fail; + goto fail3; } - switch (untrusted_service_name[0]) { - case '\0': - LOG(ERROR, "Empty service name not allowed"); - goto fail; - case '+': - LOG(ERROR, "Service name must not start with '+'"); - goto fail; - default: - sanitize_name(untrusted_service_name, "+"); - service_name = untrusted_service_name; - untrusted_service_name = NULL; - /* sanitize end */ + if (!validate_service_name(untrusted_service_name)) + goto fail3; + service_name = untrusted_service_name; + untrusted_service_name = NULL; + /* sanitize end */ - handle_execute_service(remote_domain_id, remote_domain_name, - params3.target_domain, - service_name, - ¶ms3.request_id); - free(service_name); - return; - } -fail: + handle_execute_service(remote_domain_id, remote_domain_name, + params3.target_domain, + service_name, + ¶ms3.request_id); + free(service_name); + return; +fail3: send_service_refused(vchan, &untrusted_params3.request_id); free(untrusted_service_name); return; diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index 417d985f..956005ba 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -203,7 +203,6 @@ def test_bad_request_id_bad_byte_after_nul_terminator(self): self.assertEqual(data[:4], b"a\0b\0") self.assertEqual(data[4:], b"\0" * 28) - @unittest.expectedFailure def test_bad_old_style_request(self): agent = self.start_daemon_with_agent() agent.send_message(qrexec.MSG_HELLO, struct.pack(" Date: Sat, 6 Apr 2024 11:19:34 -0400 Subject: [PATCH 09/15] Avoid using /tmp for qrexec return pipes This avoids a privilege escalation from unprivileged users (not in the "qubes" group). Fixes: QubesOS/qubes-issues#9097 --- lib/qubes-rpc-multiplexer | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/qubes-rpc-multiplexer b/lib/qubes-rpc-multiplexer index eea0ca30..0c34d740 100755 --- a/lib/qubes-rpc-multiplexer +++ b/lib/qubes-rpc-multiplexer @@ -1,22 +1,25 @@ #!/bin/sh -l +# we don't use globbing, disable it +set -f -if [ -z "$QREXEC_SERVICE_PATH" ]; then +if [ -z "${QREXEC_SERVICE_PATH+x}" ]; then QREXEC_SERVICE_PATH=/usr/local/etc/qubes-rpc:/etc/qubes-rpc fi +tmpdir=${XDG_RUNTIME_DIR-/tmp} # write stderr to both calling party and local log; be very careful about # closing file descriptors here - if either stdout or stderr will not be closed # when service process does the same - service call will hang (waiting for EOF # on stdout/stderr) -stderr_pipe=/tmp/qrexec-rpc-stderr.$$ -mkfifo $stderr_pipe +stderr_pipe=$tmpdir/qrexec-rpc-stderr.$$ +mkfifo -- "$stderr_pipe" # tee can't write to file descriptor, nor /proc/self/fd/2 (EXIO on open) -return_stderr_pipe=/tmp/qrexec-rpc-stderr-return.$$ -mkfifo $return_stderr_pipe -{ cat <$return_stderr_pipe >&2 2>/dev/null; rm -f $return_stderr_pipe; } /dev/null & -{ tee $return_stderr_pipe <$stderr_pipe |\ - logger -t "$1-$2"; rm -f $stderr_pipe; } /dev/null 2>&1 & -exec 2>$stderr_pipe +return_stderr_pipe=$tmpdir/qrexec-rpc-stderr-return.$$ +mkfifo -- "$return_stderr_pipe" +{ cat <"$return_stderr_pipe" >&2 2>/dev/null; rm -f -- "$return_stderr_pipe"; } /dev/null & +{ tee -- "$return_stderr_pipe" <"$stderr_pipe" | + logger -t "$1-$2"; rm -f -- "$stderr_pipe"; } /dev/null 2>&1 & +exec 2>"$stderr_pipe" if ! [ $# = 2 -o $# = 4 ] ; then echo "$0: bad argument count, usage: $0 SERVICE-NAME REMOTE-DOMAIN-NAME [REQUESTED_TARGET_TYPE REQUESTED_TARGET]" >&2 From 6967f3c64a6741cdc14a2d53e5003abfb38a35f9 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 8 Apr 2024 20:14:42 -0400 Subject: [PATCH 10/15] Test that service configs are found in all places they should be Service config for qubes.Service+arg should also be checked for in the location for qubes.Service. Add a test for that. --- qrexec/tests/socket/agent.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 9490000c..a950fbc4 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -408,6 +408,10 @@ def test_exec_service_with_config(self): self.check_dom0(dom0) def test_wait_for_session(self): + self._test_wait_for_session("qubes.Service+arg") + def test_wait_for_session_config_in_location_sans_argument(self): + self._test_wait_for_session("qubes.Service") + def _test_wait_for_session(self, config_name): log = os.path.join(self.tempdir, "wait-for-session.log") util.make_executable_service( self.tempdir, @@ -438,7 +442,7 @@ def test_wait_for_session(self): assert "'" not in user assert "\n" not in user with open( - os.path.join(self.tempdir, "rpc-config", "qubes.Service+arg"), "w" + os.path.join(self.tempdir, "rpc-config", config_name), "w" ) as f: f.write(f"""\ From 175e5add996eded0aa73e68501239ada8574c6e0 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 8 Apr 2024 21:46:10 -0400 Subject: [PATCH 11/15] Test that config in a long path is loaded Fails because of QubesOS/qubes-issues#9099. --- qrexec/tests/socket/agent.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index a950fbc4..ba62c591 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -409,9 +409,14 @@ def test_exec_service_with_config(self): def test_wait_for_session(self): self._test_wait_for_session("qubes.Service+arg") + @unittest.expectedFailure + def test_wait_for_session_huge_path(self): + l = 255 - len("qubes.Service+") + arg = l * "a" + self._test_wait_for_session("qubes.Service", argument=arg) def test_wait_for_session_config_in_location_sans_argument(self): self._test_wait_for_session("qubes.Service") - def _test_wait_for_session(self, config_name): + def _test_wait_for_session(self, config_name, service_name="qubes.Service", argument="arg"): log = os.path.join(self.tempdir, "wait-for-session.log") util.make_executable_service( self.tempdir, @@ -428,7 +433,7 @@ def _test_wait_for_session(self, config_name): util.make_executable_service( self.tempdir, "rpc", - "qubes.Service", + service_name, """\ #!/bin/sh cat {} @@ -451,7 +456,7 @@ def _test_wait_for_session(self, config_name): wait-for-session = 1 # line comment """) - target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") + target, dom0 = self.execute_qubesrpc(service_name + "+" + argument, "domX") self.assertEqual(target.recv_message(), ( qrexec.MSG_DATA_STDOUT, ( @@ -470,7 +475,8 @@ def _test_wait_for_session(self, config_name): [ ( qrexec.MSG_DATA_STDOUT, - b"arg: arg, remote domain: domX, input: stdin data\n", + b"arg: " + argument.encode("ascii", "strict") + + b", remote domain: domX, input: stdin data\n", ), (qrexec.MSG_DATA_STDOUT, b""), (qrexec.MSG_DATA_STDERR, b""), From ef32e6e9b734b8e524e11e4b1edd15336c49ca4c Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 8 Apr 2024 21:47:18 -0400 Subject: [PATCH 12/15] Load service configuration files with long names Previously they were skipped. Fixes: QubesOS/qubes-issues#9099 --- libqrexec/exec.c | 2 +- qrexec/tests/socket/agent.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/libqrexec/exec.c b/libqrexec/exec.c index df65a68e..5d2eae07 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -278,7 +278,7 @@ static int load_service_config_raw(struct qrexec_parsed_command *cmd, if (!config_path) config_path = QUBES_RPC_CONFIG_PATH; - char config_full_path[256]; + char config_full_path[QUBES_SOCKADDR_UN_MAX_PATH_LEN]; int ret = find_file(config_path, cmd->service_descriptor, config_full_path, sizeof(config_full_path), NULL); diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index ba62c591..0cef2f50 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -409,7 +409,6 @@ def test_exec_service_with_config(self): def test_wait_for_session(self): self._test_wait_for_session("qubes.Service+arg") - @unittest.expectedFailure def test_wait_for_session_huge_path(self): l = 255 - len("qubes.Service+") arg = l * "a" From 7ed430393313ee1405cd92c468119284c929fd16 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 8 Apr 2024 21:50:12 -0400 Subject: [PATCH 13/15] Test for errors reading a service config file This fails because of QubesOS/qubes-issues#9100 --- qrexec/tests/socket/agent.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 0cef2f50..2efbc351 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -508,10 +508,12 @@ def exec_service_with_invalid_config(self, invalid_config): echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN" """, ) - with open( - os.path.join(self.tempdir, "rpc-config", "qubes.Service+arg"), "w" - ) as f: - f.write(invalid_config) + config_path = os.path.join(self.tempdir, "rpc-config", "qubes.Service+arg") + if invalid_config is not None: + with open(config_path, "w") as f: + f.write(invalid_config) + else: + os.symlink("/dev/null/doesnotexist", config_path) target, dom0 = self.execute_qubesrpc("qubes.Service+arg", "domX") messages = target.recv_all_messages() self.assertListEqual( @@ -539,6 +541,10 @@ def test_exec_service_with_invalid_config_4(self): def test_exec_service_with_invalid_config_5(self): self.exec_service_with_invalid_config("wait-for-session\n") + @unittest.expectedFailure + def test_exec_service_with_invalid_config_6(self): + self.exec_service_with_invalid_config(None) + def test_exec_service_with_arg(self): self.make_executable_service( "local-rpc", From 0d85560dc1882379e453c890fc2a5f3ed616ee07 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 8 Apr 2024 21:54:10 -0400 Subject: [PATCH 14/15] Fail service call if config file cannot be read This includes the case where there is an I/O error or it is e.g. a symlink to a nonexistent file. Fixes: QubesOS/qubes-issues#9100 --- libqrexec/exec.c | 2 +- qrexec/tests/socket/agent.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/libqrexec/exec.c b/libqrexec/exec.c index 5d2eae07..1ec94bf6 100644 --- a/libqrexec/exec.c +++ b/libqrexec/exec.c @@ -285,7 +285,7 @@ static int load_service_config_raw(struct qrexec_parsed_command *cmd, if (ret == -1) ret = find_file(config_path, cmd->service_name, config_full_path, sizeof(config_full_path), NULL); - if (ret < 0) + if (ret == -1) return 0; return qubes_toml_config_parse(config_full_path, &cmd->wait_for_session, user, &cmd->send_service_descriptor); diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 2efbc351..8aaa5fbc 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -541,7 +541,6 @@ def test_exec_service_with_invalid_config_4(self): def test_exec_service_with_invalid_config_5(self): self.exec_service_with_invalid_config("wait-for-session\n") - @unittest.expectedFailure def test_exec_service_with_invalid_config_6(self): self.exec_service_with_invalid_config(None) From 2be9adc2d988a5cfb910d2a0050200adbd351898 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 8 Apr 2024 22:11:36 -0400 Subject: [PATCH 15/15] qrexec-client: fail if service configuration loading fails Previously all such errors were ignored. Fixes: QubesOS/qubes-issues#9101 --- daemon/qrexec-client.c | 31 +++++++++++++++----- qrexec/tests/socket/daemon.py | 55 +++++++++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 14 deletions(-) diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index 87e2dfc2..8517bf5d 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -211,25 +211,33 @@ static _Noreturn void do_exec(const char *prog, const char *username __attribute /* See also qrexec-agent.c:wait_for_session_maybe() */ -static void wait_for_session_maybe(char *cmdline) +static bool wait_for_session_maybe(char *cmdline) { struct qrexec_parsed_command *cmd; pid_t pid; int status; + bool rc = false; cmd = parse_qubes_rpc_command(cmdline, false); if (!cmd) goto out; - if (cmd->nogui) + if (cmd->nogui) { + rc = true; goto out; + } - if (!cmd->service_descriptor) + if (!cmd->service_descriptor) { + rc = true; goto out; + } - load_service_config_v2(cmd); - if (!cmd->wait_for_session) + if (load_service_config_v2(cmd) < 0) goto out; + if (!cmd->wait_for_session) { + rc = true; + goto out; + } pid = fork(); switch (pid) { @@ -237,10 +245,12 @@ static void wait_for_session_maybe(char *cmdline) close(0); exec_wait_for_session(cmd->source_domain); PERROR("exec"); - exit(1); + _exit(1); case -1: PERROR("fork"); goto out; + default: + rc = true; } if (waitpid(local_pid, &status, 0) > 0) { @@ -251,6 +261,7 @@ static void wait_for_session_maybe(char *cmdline) out: destroy_qrexec_parsed_command(cmd); + return rc; } static int prepare_local_fds(char *cmdline, struct buffer *stdin_buffer) @@ -617,8 +628,12 @@ int main(int argc, char **argv) struct buffer stdin_buffer; buffer_init(&stdin_buffer); - wait_for_session_maybe(remote_cmdline); - prepare_ret = prepare_local_fds(remote_cmdline, &stdin_buffer); + if (!wait_for_session_maybe(remote_cmdline)) { + LOG(ERROR, "Cannot load service configuration, or forking process failed"); + prepare_ret = -1; + } else { + prepare_ret = prepare_local_fds(remote_cmdline, &stdin_buffer); + } if (request_id) { void (*old_handler)(int); diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index 956005ba..bbc928df 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -782,19 +782,56 @@ def test_run_dom0_command_and_connect_vm(self): self.client.wait() self.assertEqual(self.client.returncode, 0) - def test_run_dom0_service_exec(self): + def exec_service_with_invalid_config(self, invalid_config): + config_path = os.path.join(self.tempdir, "rpc-config", "qubes.Service+arg") + if invalid_config is not None: + with open(config_path, "w") as f: + f.write(invalid_config) + else: + os.symlink("/dev/null/doesnotexist", config_path) util.make_executable_service( self.tempdir, "rpc", "qubes.Service", """\ - #!/bin/sh - read input - echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN, input: $input" - """, +#!/bin/sh +read input +echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN, input: $input" +""", ) + self.test_run_dom0_service_failed() - cmd = "QUBESRPC qubes.Service+arg src_domain name src_domain" + def test_exec_service_with_invalid_config_1(self): + self.exec_service_with_invalid_config("wait-for-session = 00\n") + + def test_exec_service_with_invalid_config_2(self): + self.exec_service_with_invalid_config("wait-for-session = 01\n") + + def test_exec_service_with_invalid_config_3(self): + self.exec_service_with_invalid_config("wait-for-session = \n") + + def test_exec_service_with_invalid_config_4(self): + self.exec_service_with_invalid_config("wait-for-session = \"a\"\n") + + def test_exec_service_with_invalid_config_5(self): + self.exec_service_with_invalid_config("wait-for-session\n") + + def test_exec_service_with_invalid_config_6(self): + self.exec_service_with_invalid_config(None) + + def _test_run_dom0_service_exec(self, nogui): + util.make_executable_service( + self.tempdir, + "rpc", + "qubes.Service", + """\ +#!/bin/sh +read input +echo "arg: $1, remote domain: $QREXEC_REMOTE_DOMAIN, input: $input" +""", + ) + + cmd = ("nogui:" if nogui else "") + "QUBESRPC qubes.Service+arg src_domain name src_domain" source = self.connect_service_request(cmd) source.send_message(qrexec.MSG_DATA_STDIN, b"stdin data\n") @@ -814,6 +851,12 @@ def test_run_dom0_service_exec(self): self.client.wait() self.assertEqual(self.client.returncode, 0) + def test_run_dom0_service_exec(self): + self._test_run_dom0_service_exec(False) + + def test_run_dom0_service_exec_nogui(self): + self._test_run_dom0_service_exec(True) + def test_run_dom0_service_failed(self): # qubes.Service does not exist cmd = "QUBESRPC qubes.Service+arg src_domain name src_domain"