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"