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, as there is too
much code in dom0 that depends on the current behavior.

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#9089
  • Loading branch information
@DemiMarie
DemiMarie committed Apr 6, 2024
1 parent 03feaf7 commit 33a17c4
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 32 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
70 changes: 39 additions & 31 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 && cmd->service_descriptor != cmd->service_name)
ret = find_file(config_path, cmd->service_name,
config_full_path, sizeof(config_full_path), NULL);
if (ret < 0)
Expand Down Expand Up @@ -361,53 +361,61 @@ 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 = malloc(name_len + 1);
if (!cmd->service_name) {
PERROR("malloc");
goto err;
}
memcpy(cmd->service_name, start, name_len);
cmd->service_name[name_len] = '\0';

/* If there is no service argument, add a trailing "+" to the descriptor */
size_t const descriptor_buffer_size = descriptor_len + 1 + (plus == NULL);
cmd->service_descriptor = malloc(descriptor_buffer_size);
if (!cmd->service_descriptor) {
PERROR("malloc");
goto err;
}
memcpy(cmd->service_descriptor, start, descriptor_len);
/* if plus != NULL, this + will be immediately overwritten */
cmd->service_descriptor[descriptor_len] = '+';
cmd->service_descriptor[descriptor_buffer_size - 1] = '\0';

/* Parse source domain */

start = end + 1; /* after the space */
end = strchrnul(start, ' ');
cmd->source_domain = strndup(start, end - start);
size_t const source_domain_len = (size_t)(end - start);
cmd->source_domain = malloc(source_domain_len + 1);
if (!cmd->source_domain) {
PERROR("strndup");
goto err;
}
memcpy(cmd->source_domain, start, source_domain_len);
cmd->source_domain[source_domain_len] = '\0';
}

return cmd;
Expand Down Expand Up @@ -480,7 +488,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 33a17c4

Please sign in to comment.