Skip to content

Commit

Permalink
Report correct statuses for service execution failure
Browse files Browse the repository at this point in the history
"Service not found" must be reported with status 127, and "service
cannot be executed" must be reported with status 125.  However, the code
had lots of bugs and would often report wrong status codes in these
error conditions.

This fixes all of the problems I could find, and adds tests to ensure
that many cases stay fixed.  This breaks the libqrexec ABI, but
thankfully the ABI that was broken is not in any release yet.
  • Loading branch information
DemiMarie committed Apr 28, 2024
1 parent a91949a commit 163597e
Show file tree
Hide file tree
Showing 13 changed files with 160 additions and 125 deletions.
30 changes: 18 additions & 12 deletions agent/qrexec-agent-data.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,24 @@ static int handle_just_exec(struct qrexec_parsed_command *cmd)
int fdn, pid;

if (cmd == NULL)
return 127;
return QREXEC_EXIT_PROBLEM;

if (cmd->service_descriptor) {
int socket_fd;
struct buffer stdin_buffer;
buffer_init(&stdin_buffer);
if (!find_qrexec_service(cmd, &socket_fd, &stdin_buffer))
return 127;
int status = find_qrexec_service(cmd, &socket_fd, &stdin_buffer);
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 : 127;
return write_all(socket_fd, stdin_buffer.data, stdin_buffer.buflen) ? 0 : QREXEC_EXIT_PROBLEM;
}
switch (pid = fork()) {
case -1:
PERROR("fork");
return 127;
return QREXEC_EXIT_PROBLEM;
case 0:
fdn = open("/dev/null", O_RDWR);
fix_fds(fdn, fdn, fdn);
Expand Down Expand Up @@ -226,23 +229,26 @@ static int handle_new_process_common(
libvchan_close(data_vchan);
return 0;
case MSG_EXEC_CMDLINE:
exit_code = QREXEC_EXIT_PROBLEM;
buffer_init(&stdin_buf);
if (cmd == NULL ||
execute_parsed_qubes_rpc_command(cmd, &pid, &stdin_fd, &stdout_fd, &stderr_fd, &stdin_buf) < 0) {
struct msg_header hdr = {
.type = MSG_DATA_STDOUT,
.len = 0,
};
if (cmd != NULL) {
int status = execute_parsed_qubes_rpc_command(cmd, &pid, &stdin_fd, &stdout_fd, &stderr_fd, &stdin_buf);
if (status == -1)
exit_code = QREXEC_EXIT_SERVICE_NOT_FOUND;
else if (status == 0)
exit_code = 0;
}
if (exit_code != 0) {
LOG(ERROR, "failed to spawn process");
/* Send stdout+stderr EOF first, since the service is expected to send
* one before exit code in case of MSG_EXEC_CMDLINE. Ignore
* libvchan_send error if any, as we're going to terminate soon
* anyway.
*/
struct msg_header hdr = { .type = MSG_DATA_STDOUT, .len = 0 };
libvchan_send(data_vchan, &hdr, sizeof(hdr));
hdr.type = MSG_DATA_STDERR;
libvchan_send(data_vchan, &hdr, sizeof(hdr));
exit_code = 127;
send_exit_code(data_vchan, exit_code);
libvchan_close(data_vchan);
return exit_code;
Expand Down
6 changes: 3 additions & 3 deletions agent/qrexec-agent.c
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,9 @@ _Noreturn void do_exec(const char *cmd, const char *user)
/* child */

if (setgid (pw->pw_gid))
exit(126);
_exit(QREXEC_EXIT_PROBLEM);
if (setuid (pw->pw_uid))
exit(126);
_exit(QREXEC_EXIT_PROBLEM);
setsid();
/* This is a copy but don't care to free as we exec later anyway. */
env = pam_getenvlist (pamh);
Expand All @@ -289,7 +289,7 @@ _Noreturn void do_exec(const char *cmd, const char *user)

/* otherwise exec shell */
execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env);
exit(127);
_exit(QREXEC_EXIT_PROBLEM);
default:
/* parent */
/* close std*, so when child process closes them, qrexec-agent will receive EOF */
Expand Down
2 changes: 1 addition & 1 deletion agent/qrexec-client-vm.c
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ int main(int argc, char **argv)
ret = read(trigger_fd, &exec_params, sizeof(exec_params));
if (ret == 0) {
fprintf(stderr, "Request refused\n");
exit(126);
exit(QREXEC_EXIT_REQUEST_REFUSED);
}
if (ret < 0 || ret != sizeof(exec_params)) {
PERROR("read");
Expand Down
4 changes: 3 additions & 1 deletion agent/qrexec-client-vm.rst
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ way to learn exit code of remote service in this case.
In both cases, if process (local or remote) was terminated by a signal, exit
status is 128+signal number.

If service call is denied by dom0, ``qrexec-client-vm`` exit with status 126.
If service call is denied by dom0, ``qrexec-client-vm`` exits with status 126.
If invoking the service fails for some other reason, such as resource exhaustion
or a system configuration problem, ``qrexec-client-vm`` exits with status 125.

AUTHORS
=======
Expand Down
4 changes: 2 additions & 2 deletions daemon/qrexec-client.c
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ int main(int argc, char **argv)
bool replace_chars_stdout = false;
bool replace_chars_stderr = false;
bool exit_with_code = true;
int rc = 126;
int rc = QREXEC_EXIT_PROBLEM;

if (argc < 3) {
// certainly too few arguments
Expand Down Expand Up @@ -301,7 +301,7 @@ int main(int argc, char **argv)
struct qrexec_parsed_command *command =
parse_qubes_rpc_command(local_cmdline, false);
if (!command)
prepare_ret = 127;
prepare_ret = QREXEC_EXIT_PROBLEM;
else {
prepare_ret = prepare_local_fds(command, &stdin_buffer);
destroy_qrexec_parsed_command(command);
Expand Down
24 changes: 11 additions & 13 deletions daemon/qrexec-daemon-common.c
Original file line number Diff line number Diff line change
Expand Up @@ -277,16 +277,14 @@ int prepare_local_fds(struct qrexec_parsed_command *command, struct buffer *stdi
};
sigemptyset(&action.sa_mask);
if (sigaction(SIGCHLD, &action, NULL))
return 126;
return QREXEC_EXIT_PROBLEM;
return execute_parsed_qubes_rpc_command(command, &local_pid, &local_stdin_fd, &local_stdout_fd,
NULL, stdin_buffer);
}

// See also qrexec-agent/qrexec-agent-data.c
__attribute__((warn_unused_result))
static int handle_failed_exec(libvchan_t *data_vchan, bool is_service)
static void handle_failed_exec(libvchan_t *data_vchan, bool is_service, int exit_code)
{
int exit_code = 127;
struct msg_header hdr = {
.type = MSG_DATA_STDOUT,
.len = 0,
Expand All @@ -308,7 +306,6 @@ static int handle_failed_exec(libvchan_t *data_vchan, bool is_service)
libvchan_send(data_vchan, &hdr, sizeof(hdr));
send_exit_code(data_vchan, exit_code);
}
return exit_code;
}

static int select_loop(struct handshake_params *params)
Expand Down Expand Up @@ -351,25 +348,25 @@ int run_qrexec_to_dom0(const struct service_params *svc_params,
set_remote_domain(src_domain_name);
s = connect_unix_socket_by_id(src_domain_id);
if (s < 0)
return 126;
return QREXEC_EXIT_PROBLEM;
if (!negotiate_connection_params(s,
0, /* dom0 */
MSG_SERVICE_CONNECT,
svc_params,
sizeof(*svc_params),
&data_domain,
&data_port))
return 126;
return QREXEC_EXIT_PROBLEM;

struct buffer stdin_buffer;
buffer_init(&stdin_buffer);
struct qrexec_parsed_command *command =
parse_qubes_rpc_command(remote_cmdline, false);
if (command == NULL) {
prepare_ret = -1;
prepare_ret = -2;
} else if (!wait_for_session_maybe(command)) {
LOG(ERROR, "Cannot load service configuration, or forking process failed");
prepare_ret = -1;
prepare_ret = -2;
} else {
prepare_ret = prepare_local_fds(command, &stdin_buffer);
}
Expand Down Expand Up @@ -401,15 +398,16 @@ int handshake_and_go(struct handshake_params *params)
{
if (params->data_vchan == NULL || !libvchan_is_open(params->data_vchan)) {
LOG(ERROR, "Failed to open data vchan connection");
return 126;
return QREXEC_EXIT_PROBLEM;
}
int rc;
int data_protocol_version = handle_agent_handshake(params->data_vchan,
params->remote_send_first);
if (data_protocol_version < 0) {
rc = 126;
} else if (params->prepare_ret < 0) {
rc = handle_failed_exec(params->data_vchan, params->remote_send_first);
rc = QREXEC_EXIT_PROBLEM;
} else if (params->prepare_ret != 0) {
rc = params->prepare_ret == -1 ? QREXEC_EXIT_SERVICE_NOT_FOUND : QREXEC_EXIT_PROBLEM;
handle_failed_exec(params->data_vchan, params->remote_send_first, rc);
} else {
params->data_protocol_version = data_protocol_version;
rc = select_loop(params);
Expand Down
44 changes: 22 additions & 22 deletions daemon/qrexec-daemon.c
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,7 @@ static void send_request_to_daemon(
service_name);
if (command_size < 0) {
PERROR("failed to construct request");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}

for (int i = 0; i < command_size; i += bytes_sent) {
Expand All @@ -934,7 +934,7 @@ static void send_request_to_daemon(
if (bytes_sent < 0) {
assert(bytes_sent == -1);
PERROR("send to socket failed");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
}
free(command);
Expand All @@ -945,7 +945,7 @@ static _Noreturn void null_exit(void)
#ifdef COVERAGE
__gcov_dump();
#endif
_exit(126);
_exit(QREXEC_EXIT_PROBLEM);
}

/*
Expand All @@ -970,7 +970,7 @@ static enum policy_response connect_daemon_socket(
int daemon_socket = socket(AF_UNIX, SOCK_STREAM, 0);
if (daemon_socket < 0) {
PERROR("socket creation failed");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}

int connect_result = connect(daemon_socket,
Expand Down Expand Up @@ -1001,21 +1001,21 @@ static enum policy_response connect_daemon_socket(
int fds[2];
if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, fds)) {
PERROR("socketpair()");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
daemon_socket = fds[0];

pid = fork();
switch (pid) {
case -1:
LOG(ERROR, "Could not fork!");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
case 0:
if (close(fds[0]))
_exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
if (dup2(fds[1], 0) != 0 || dup2(fds[1], 1) != 1) {
PERROR("dup2()");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
if (close(fds[1]))
abort();
Expand All @@ -1034,7 +1034,7 @@ static enum policy_response connect_daemon_socket(
} else {
PERROR("snprintf");
}
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
default:
if (close(fds[1]))
abort();
Expand All @@ -1045,12 +1045,12 @@ static enum policy_response connect_daemon_socket(
do {
if (waitpid(pid, &status, 0) != pid) {
PERROR("waitpid");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
} while (!WIFEXITED(status));
if (WEXITSTATUS(status) != 0) {
LOG(ERROR, "qrexec-policy-exec failed");
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
// This leaks 'result', but as the code execs later anyway this isn't a problem.
// 'result' cannot be freed as 'user', 'target', and 'requested_target' point into
Expand All @@ -1077,7 +1077,7 @@ static _Noreturn void do_exec(const char *prog, const char *username __attribute
/* if above haven't executed qubes-rpc-multiplexer, pass it to shell */
execl("/bin/bash", "bash", "-c", prog, NULL);
PERROR("exec bash");
_exit(126);
_exit(QREXEC_EXIT_PROBLEM);
}

_Noreturn static void handle_execute_service_child(
Expand All @@ -1098,7 +1098,7 @@ _Noreturn static void handle_execute_service_child(
&user, &target, &requested_target, &autostart);

if (policy_response != RESPONSE_ALLOW)
daemon__exit(126);
daemon__exit(QREXEC_EXIT_REQUEST_REFUSED);

/* Replace the target domain with the version normalized by the policy engine */
target_domain = requested_target;
Expand Down Expand Up @@ -1130,7 +1130,7 @@ _Noreturn static void handle_execute_service_child(
remote_domain_name,
type,
target_domain) <= 0)
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
register_exec_func(&do_exec);
daemon__exit(run_qrexec_to_dom0(request_id,
remote_domain_id,
Expand All @@ -1144,7 +1144,7 @@ _Noreturn static void handle_execute_service_child(
disposable = true;
buf = qubesd_call(target + 8, "admin.vm.CreateDisposable", "", &resp_len);
if (!buf) // error already printed by qubesd_call
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
if (memcmp(buf, "0", 2) == 0) {
/* we exec later so memory leaks do not matter */
target = buf + 2;
Expand All @@ -1154,42 +1154,42 @@ _Noreturn static void handle_execute_service_child(
} else {
LOG(ERROR, "invalid response to admin.vm.CreateDisposable");
}
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
}
if (asprintf(&cmd, "%s:QUBESRPC %s%s %s",
user,
service_name,
trailer,
remote_domain_name) <= 0)
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
if (autostart) {
buf = qubesd_call(target, "admin.vm.Start", "", &resp_len);
if (!buf) // error already printed by qubesd_call
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
if (!((memcmp(buf, "0", 2) == 0) ||
(resp_len >= 24 && memcmp(buf, "2\0QubesVMNotHaltedError", 24) == 0))) {
if (memcmp(buf, "2", 2) == 0) {
LOG(ERROR, "qubesd could not start VM %s: %s", target, buf + 2);
} else {
LOG(ERROR, "invalid response to admin.vm.Start");
}
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
}
free(buf);
}
int s = connect_unix_socket(target);
int data_domain;
int data_port;
int rc = 126;
int rc = QREXEC_EXIT_PROBLEM;
if (!negotiate_connection_params(s,
remote_domain_id,
MSG_EXEC_CMDLINE,
cmd,
compute_service_length(cmd),
&data_domain,
&data_port))
daemon__exit(126);
daemon__exit(QREXEC_EXIT_PROBLEM);
int wait_connection_end = -1;
if (disposable) {
wait_connection_end = s;
Expand Down Expand Up @@ -1237,7 +1237,7 @@ static void handle_execute_service(
exit(1);
case 0:
if (atexit(null_exit))
_exit(126);
_exit(QREXEC_EXIT_PROBLEM);
handle_execute_service_child(remote_domain_id, remote_domain_name,
target_domain, service_name, request_id);
abort();
Expand Down
Loading

0 comments on commit 163597e

Please sign in to comment.