Skip to content

Commit

Permalink
Use flexible array member for 'struct trigger_service_params3'
Browse files Browse the repository at this point in the history
No functional change, but the new code is shorter (and simpler).  Also
drop a pointless check that the command is NUL-terminated, as
qrexec-daemon will do that anyway.
  • Loading branch information
DemiMarie committed Apr 23, 2024
1 parent 72643d7 commit 4342589
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 50 deletions.
32 changes: 11 additions & 21 deletions agent/qrexec-agent.c
Original file line number Diff line number Diff line change
Expand Up @@ -819,9 +819,7 @@ static void reap_children(void)
static void handle_trigger_io(void)
{
struct msg_header hdr;
struct trigger_service_params3 params;
char *command = NULL;
size_t command_len;
struct trigger_service_params3 *params = NULL;
int client_fd;

client_fd = do_accept(trigger_fd);
Expand All @@ -830,40 +828,32 @@ static void handle_trigger_io(void)
if (!read_all(client_fd, &hdr, sizeof(hdr)))
goto error;
if (hdr.type != MSG_TRIGGER_SERVICE3 ||
hdr.len <= sizeof(params) ||
hdr.len > sizeof(params) + MAX_SERVICE_NAME_LEN) {
hdr.len <= sizeof(*params) ||
hdr.len > sizeof(*params) + MAX_SERVICE_NAME_LEN) {
LOG(ERROR, "Invalid request received from qrexec-client-vm, is it outdated?");
goto error;
}
if (!read_all(client_fd, &params, sizeof(params)))
params = malloc(hdr.len);
if (!params)
goto error;
command_len = hdr.len - sizeof(params);
command = malloc(command_len);
if (!command)
goto error;
if (!read_all(client_fd, command, command_len))
goto error;
if (command[command_len-1] != '\0')
if (!read_all(client_fd, params, hdr.len))
goto error;

int res = snprintf(params.request_id.ident, sizeof(params.request_id), "SOCKET%d", client_fd);
if (res < 0 || res >= (int)sizeof(params.request_id))
int res = snprintf(params->request_id.ident, sizeof(params->request_id), "SOCKET%d", client_fd);
if (res < 0 || res >= (int)sizeof(params->request_id))
abort();
if (libvchan_send(ctrl_vchan, &hdr, sizeof(hdr)) != sizeof(hdr))
handle_vchan_error("write hdr");
if (libvchan_send(ctrl_vchan, &params, sizeof(params)) != sizeof(params))
if (libvchan_send(ctrl_vchan, params, hdr.len) != (int)hdr.len)
handle_vchan_error("write params");
if (libvchan_send(ctrl_vchan, command, command_len) != (int)command_len)
handle_vchan_error("write command");

free(command);
free(params);
/* do not close client_fd - we'll need it to send the connection details
* later (when dom0 accepts the request) */
return;
error:
LOG(ERROR, "Failed to retrieve/execute request from qrexec-client-vm");
if (command)
free(command);
free(params);
close(client_fd);
}

Expand Down
49 changes: 21 additions & 28 deletions daemon/qrexec-daemon.c
Original file line number Diff line number Diff line change
Expand Up @@ -1412,55 +1412,48 @@ void handle_message_from_agent(void)
return;
}
case MSG_TRIGGER_SERVICE3: {
struct trigger_service_params3 untrusted_params3, params3;
size_t service_name_len = hdr.len - sizeof(untrusted_params3), nul_offset;
char *untrusted_service_name = malloc(service_name_len), *service_name = NULL;
struct trigger_service_params3 *untrusted_params3, *params3;

if (!untrusted_service_name)
untrusted_params3 = malloc(hdr.len);
if (!untrusted_params3)
handle_vchan_error("malloc(service_name)");

if (libvchan_recv(vchan, &untrusted_params3, sizeof(untrusted_params3))
!= sizeof(untrusted_params3)) {
free(untrusted_service_name);
handle_vchan_error("recv params3");
}
if (libvchan_recv(vchan, untrusted_service_name, service_name_len)
!= (int)service_name_len) {
free(untrusted_service_name);
if (libvchan_recv(vchan, untrusted_params3, hdr.len)
!= (int)hdr.len) {
free(untrusted_params3);
handle_vchan_error("recv params3(service_name)");
}
service_name_len -= 1;
size_t const service_name_len = hdr.len - sizeof(*untrusted_params3) - 1;

/* sanitize start */
ENSURE_NULL_TERMINATED(untrusted_params3.target_domain);
sanitize_name(untrusted_params3.target_domain, "@:");
if (!validate_request_id(&untrusted_params3.request_id, "MSG_TRIGGER_SERVICE3"))
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 fail3;
params3 = untrusted_params3;
if (untrusted_service_name[service_name_len] != 0) {
if (untrusted_params3->service_name[service_name_len] != 0) {
LOG(ERROR, "Service name not NUL-terminated");
goto fail3;
}
nul_offset = strlen(untrusted_service_name);
size_t const nul_offset = strlen(untrusted_params3->service_name);
if (nul_offset != service_name_len) {
LOG(ERROR, "Service name contains NUL byte at offset %zu", nul_offset);
goto fail3;
}
if (!validate_service_name(untrusted_service_name))
if (!validate_service_name(untrusted_params3->service_name))
goto fail3;
service_name = untrusted_service_name;
untrusted_service_name = NULL;
params3 = untrusted_params3;
untrusted_params3 = NULL;
/* sanitize end */

handle_execute_service(remote_domain_id, remote_domain_name,
params3.target_domain,
service_name,
&params3.request_id);
free(service_name);
params3->target_domain,
params3->service_name,
&params3->request_id);
free(params3);
return;
fail3:
send_service_refused(vchan, &untrusted_params3.request_id);
free(untrusted_service_name);
send_service_refused(vchan, &untrusted_params3->request_id);
free(untrusted_params3);
return;
}
case MSG_CONNECTION_TERMINATED:
Expand Down
2 changes: 1 addition & 1 deletion libqrexec/qrexec.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ struct trigger_service_params {
struct trigger_service_params3 {
char target_domain[64]; /* null terminated ASCII string */
struct service_params request_id; /* service request id */
// char service_name[0]; /* null terminated ASCII string, size = msg_header.len - sizeof(struct trigger_service_params3) */
char service_name[]; /* null terminated ASCII string, size = msg_header.len - sizeof(struct trigger_service_params3) */
};

struct peer_info {
Expand Down

0 comments on commit 4342589

Please sign in to comment.