Skip to content

Commit

Permalink
Search for qubes.Service+ if call for qubes.Service is made
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@DemiMarie
DemiMarie committed Apr 9, 2024
1 parent b16e0a5 commit 2a302d8
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 39 deletions.
3 changes: 3 additions & 0 deletions lib/qubes-rpc-multiplexer
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
83 changes: 45 additions & 38 deletions libqrexec/exec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
37 changes: 36 additions & 1 deletion qrexec/tests/socket/agent.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit 2a302d8

Please sign in to comment.