From 122466c42e4f2ceaa0591ef00c7516ef68ca8cfc Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 9 Feb 2024 21:50:46 -0500 Subject: [PATCH] Avoid qrexec-client for VM -> VM calls This saves an execve(). It also allows qrexec-daemon to change how it performs a VM -> VM service call without having to change qrexec-client. During an upgrade, it is possible that qrexec-daemon is newer than qrexec-client, causing the new qrexec-daemon to try to use a calling convention that the old qrexec-client doesn't support. Doing VM -> VM calls without calling execve() means this cannot happen. VM -> dom0 and dom0 -> VM calls still use qrexec-client, but VM -> dom0 calls are safe from domain name reuse races as of [1], and the latter do not involve qrexec-daemon at all. [1]: 100fbb9aaf3737ae3f10cdf265d187ca31dbc366 ("qrexec-client: Use XID to connect to qrexec daemon when possible") --- daemon/Makefile | 7 +- daemon/qrexec-client.c | 156 +++------------------------------- daemon/qrexec-daemon-common.c | 145 +++++++++++++++++++++++++++++++ daemon/qrexec-daemon-common.h | 9 ++ daemon/qrexec-daemon.c | 74 ++++++++++++---- fuzz/Makefile | 2 +- 6 files changed, 227 insertions(+), 166 deletions(-) create mode 100644 daemon/qrexec-daemon-common.c create mode 100644 daemon/qrexec-daemon-common.h diff --git a/daemon/Makefile b/daemon/Makefile index ce61d83d..d3946d41 100644 --- a/daemon/Makefile +++ b/daemon/Makefile @@ -4,7 +4,8 @@ override QUBES_CFLAGS:=-I../libqrexec -g -O2 -Wall -Wextra -Werror -fPIC \ $(shell pkg-config --cflags $(VCHAN_PKG)) -fstack-protector \ -D_FORTIFY_SOURCE=2 -fstack-protector-strong -std=gnu11 -D_POSIX_C_SOURCE=200809L \ -D_GNU_SOURCE $(CFLAGS) \ - -Wstrict-prototypes -Wold-style-definition -Wmissing-declarations + -Wstrict-prototypes -Wold-style-definition -Wmissing-declarations \ + -fvisibility=hidden override LDFLAGS += -pie -Wl,-z,relro,-z,now -L../libqrexec override LDLIBS += $(shell pkg-config --libs $(VCHAN_PKG)) -lqrexec-utils @@ -22,8 +23,8 @@ install: all ln -sf ../../bin/qrexec-client $(DESTDIR)/usr/lib/qubes/qrexec-client .PHONY: all clean install -qrexec-daemon qrexec-client: %: %.o - $(CC) $(LDFLAGS) -pie -g -o $@ $< $(LDLIBS) +qrexec-daemon qrexec-client: %: %.o qrexec-daemon-common.o + $(CC) $(LDFLAGS) -pie -g -o $@ $^ $(LDLIBS) %.o: %.c $(CC) $< -c -o $@ $(QUBES_CFLAGS) -MD -MP -MF $@.dep diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index 7430aa69..78e953e5 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -39,6 +39,7 @@ #include #include "libqrexec-utils.h" +#include "qrexec-daemon-common.h" // whether qrexec-client should replace problematic bytes with _ before printing the output static bool replace_chars_stdout = false; @@ -58,8 +59,6 @@ static bool is_service = false; static volatile sig_atomic_t sigchld = 0; -static const char *socket_dir = QREXEC_DAEMON_SOCKET_DIR; - extern char **environ; static char *xstrdup(const char *arg) { @@ -128,71 +127,6 @@ static int handle_agent_handshake(libvchan_t *vchan, bool remote_send_first) return data_protocol_version; } -static int handle_daemon_handshake(int fd) -{ - struct msg_header hdr; - struct peer_info info; - - /* daemon send MSG_HELLO first */ - if (!read_all(fd, &hdr, sizeof(hdr))) { - PERROR("daemon handshake"); - return -1; - } - if (hdr.type != MSG_HELLO || hdr.len != sizeof(info)) { - LOG(ERROR, "Invalid daemon MSG_HELLO"); - return -1; - } - if (!read_all(fd, &info, sizeof(info))) { - PERROR("daemon handshake"); - return -1; - } - - if (info.version != QREXEC_PROTOCOL_VERSION) { - LOG(ERROR, "Incompatible daemon protocol version " - "(daemon %d, client %d)", - info.version, QREXEC_PROTOCOL_VERSION); - return -1; - } - - hdr.type = MSG_HELLO; - hdr.len = sizeof(info); - info.version = QREXEC_PROTOCOL_VERSION; - - if (!write_all(fd, &hdr, sizeof(hdr))) { - LOG(ERROR, "Failed to send MSG_HELLO hdr to daemon"); - return -1; - } - if (!write_all(fd, &info, sizeof(info))) { - LOG(ERROR, "Failed to send MSG_HELLO to daemon"); - return -1; - } - return 0; -} - -static int connect_unix_socket(const char *domname) -{ - int s, len, res; - struct sockaddr_un remote; - - if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) - err(1, "socket"); - - remote.sun_family = AF_UNIX; - res = snprintf(remote.sun_path, sizeof remote.sun_path, - "%s/qrexec.%s", socket_dir, domname); - if (res < 0) - err(1, "snprintf"); - if (res >= (int)sizeof(remote.sun_path)) - errx(1, "%s/qrexec.%s is too long for AF_UNIX socket path", - socket_dir, domname); - len = (size_t)res + 1 + offsetof(struct sockaddr_un, sun_path); - if (connect(s, (struct sockaddr *) &remote, len) == -1) - err(1, "connect %s", remote.sun_path); - if (handle_daemon_handshake(s) < 0) - exit(1); - return s; -} - static void sigchld_handler(int x __attribute__((__unused__))) { sigchld = 1; @@ -299,66 +233,6 @@ static _Noreturn void handle_failed_exec(libvchan_t *data_vchan) } exit(exit_code); } - -/* ask the daemon to allocate vchan port */ -static void negotiate_connection_params(int s, int other_domid, unsigned type, - void *cmdline_param, int cmdline_size, - int *data_domain, int *data_port) -{ - struct msg_header hdr; - struct exec_params params; - hdr.type = type; - hdr.len = sizeof(params) + cmdline_size; - params.connect_domain = other_domid; - params.connect_port = 0; - if (!write_all(s, &hdr, sizeof(hdr)) - || !write_all(s, ¶ms, sizeof(params)) - || !write_all(s, cmdline_param, cmdline_size)) { - PERROR("write daemon"); - exit(1); - } - /* the daemon will respond with the same message with connect_port filled - * and empty cmdline */ - if (!read_all(s, &hdr, sizeof(hdr))) { - PERROR("read daemon"); - exit(1); - } - assert(hdr.type == type); - if (hdr.len != sizeof(params)) { - LOG(ERROR, "Invalid response for 0x%x", type); - exit(1); - } - if (!read_all(s, ¶ms, sizeof(params))) { - PERROR("read daemon"); - exit(1); - } - *data_port = params.connect_port; - *data_domain = params.connect_domain; -} - -static void send_service_connect(int s, char *conn_ident, - int connect_domain, int connect_port) -{ - struct msg_header hdr; - struct exec_params exec_params; - struct service_params srv_params; - - hdr.type = MSG_SERVICE_CONNECT; - hdr.len = sizeof(exec_params) + sizeof(srv_params); - - exec_params.connect_domain = connect_domain; - exec_params.connect_port = connect_port; - strncpy(srv_params.ident, conn_ident, sizeof(srv_params.ident) - 1); - srv_params.ident[sizeof(srv_params.ident) - 1] = '\0'; - - if (!write_all(s, &hdr, sizeof(hdr)) - || !write_all(s, &exec_params, sizeof(exec_params)) - || !write_all(s, &srv_params, sizeof(srv_params))) { - PERROR("write daemon"); - exit(1); - } -} - static void select_loop(libvchan_t *vchan, int data_protocol_version, struct buffer *stdin_buf) { struct process_io_request req = { 0 }; @@ -547,7 +421,7 @@ int main(int argc, char **argv) int data_domain; int s; bool just_exec = false; - int wait_connection_end = 0; + int wait_connection_end = -1; char *local_cmdline = NULL; char *remote_cmdline = NULL; char *request_id = NULL; @@ -614,13 +488,6 @@ int main(int argc, char **argv) usage(argv[0]); } - char src_domain_id_str[11]; - { - int snprintf_res = snprintf(src_domain_id_str, sizeof(src_domain_id_str), "%d", src_domain_id); - if (snprintf_res < 0 || snprintf_res >= (int)sizeof(src_domain_id_str)) - err(1, "snprintf()"); - } - if (strcmp(domname, "dom0") == 0 || strcmp(domname, "@adminvm") == 0) { if (request_id == NULL) { fprintf(stderr, "ERROR: when target domain is 'dom0', -c must be specified\n"); @@ -633,7 +500,7 @@ int main(int argc, char **argv) abort(); } set_remote_domain(src_domain_name); - s = connect_unix_socket(src_domain_id_str); + s = connect_unix_socket_by_id(src_domain_id); negotiate_connection_params(s, 0, /* dom0 */ MSG_SERVICE_CONNECT, @@ -665,17 +532,18 @@ int main(int argc, char **argv) compute_service_length(remote_cmdline, argv[0]), &data_domain, &data_port); - if (wait_connection_end && request_id) - /* save socket fd, 's' will be reused for the other qrexec-daemon - * connection */ - wait_connection_end = s; - else - close(s); if (request_id) { - s = connect_unix_socket(src_domain_id_str); + if (wait_connection_end != -1) { + /* save socket fd, 's' will be reused for the other qrexec-daemon + * connection */ + wait_connection_end = s; + } else { + close(s); + } + s = connect_unix_socket_by_id(src_domain_id); send_service_connect(s, request_id, data_domain, data_port); close(s); - if (wait_connection_end) { + if (wait_connection_end != -1) { /* wait for EOF */ struct pollfd fds[1] = { { .fd = wait_connection_end, .events = POLLIN | POLLHUP, .revents = 0 }, diff --git a/daemon/qrexec-daemon-common.c b/daemon/qrexec-daemon-common.c new file mode 100644 index 00000000..3215e570 --- /dev/null +++ b/daemon/qrexec-daemon-common.c @@ -0,0 +1,145 @@ +#include +#include +#include +#include +#include +#include + +#include "qrexec.h" +#include "libqrexec-utils.h" +#include "qrexec-daemon-common.h" + +const char *socket_dir = QREXEC_DAEMON_SOCKET_DIR; + +/* ask the daemon to allocate vchan port */ +void negotiate_connection_params(int s, int other_domid, unsigned type, + void *cmdline_param, int cmdline_size, + int *data_domain, int *data_port) +{ + struct msg_header hdr; + struct exec_params params; + hdr.type = type; + hdr.len = sizeof(params) + cmdline_size; + params.connect_domain = other_domid; + params.connect_port = 0; + if (!write_all(s, &hdr, sizeof(hdr)) + || !write_all(s, ¶ms, sizeof(params)) + || !write_all(s, cmdline_param, cmdline_size)) { + PERROR("write daemon"); + exit(1); + } + /* the daemon will respond with the same message with connect_port filled + * and empty cmdline */ + if (!read_all(s, &hdr, sizeof(hdr))) { + PERROR("read daemon"); + exit(1); + } + assert(hdr.type == type); + if (hdr.len != sizeof(params)) { + LOG(ERROR, "Invalid response for 0x%x", type); + exit(1); + } + if (!read_all(s, ¶ms, sizeof(params))) { + PERROR("read daemon"); + exit(1); + } + *data_port = params.connect_port; + *data_domain = params.connect_domain; +} + +int handle_daemon_handshake(int fd) +{ + struct msg_header hdr; + struct peer_info info; + + /* daemon send MSG_HELLO first */ + if (!read_all(fd, &hdr, sizeof(hdr))) { + PERROR("daemon handshake"); + return -1; + } + if (hdr.type != MSG_HELLO || hdr.len != sizeof(info)) { + LOG(ERROR, "Invalid daemon MSG_HELLO"); + return -1; + } + if (!read_all(fd, &info, sizeof(info))) { + PERROR("daemon handshake"); + return -1; + } + + if (info.version != QREXEC_PROTOCOL_VERSION) { + LOG(ERROR, "Incompatible daemon protocol version " + "(daemon %d, client %d)", + info.version, QREXEC_PROTOCOL_VERSION); + return -1; + } + + hdr.type = MSG_HELLO; + hdr.len = sizeof(info); + info.version = QREXEC_PROTOCOL_VERSION; + + if (!write_all(fd, &hdr, sizeof(hdr))) { + LOG(ERROR, "Failed to send MSG_HELLO hdr to daemon"); + return -1; + } + if (!write_all(fd, &info, sizeof(info))) { + LOG(ERROR, "Failed to send MSG_HELLO to daemon"); + return -1; + } + return 0; +} + +int connect_unix_socket_by_id(unsigned int domid) +{ + char id_str[11]; + int snprintf_res = snprintf(id_str, sizeof(id_str), "%u", domid); + if (snprintf_res < 0 || snprintf_res >= (int)sizeof(id_str)) + err(1, "snprintf()"); + return connect_unix_socket(id_str); +} + +int connect_unix_socket(const char *domname) +{ + int s, len, res; + struct sockaddr_un remote; + + if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) + err(1, "socket"); + + remote.sun_family = AF_UNIX; + res = snprintf(remote.sun_path, sizeof remote.sun_path, + "%s/qrexec.%s", socket_dir, domname); + if (res < 0) + err(1, "snprintf"); + if (res >= (int)sizeof(remote.sun_path)) + errx(1, "%s/qrexec.%s is too long for AF_UNIX socket path", + socket_dir, domname); + len = (size_t)res + 1 + offsetof(struct sockaddr_un, sun_path); + if (connect(s, (struct sockaddr *) &remote, len) == -1) + err(1, "connect %s", remote.sun_path); + if (handle_daemon_handshake(s) < 0) + exit(1); + return s; +} + +void send_service_connect(int s, const char *conn_ident, + int connect_domain, int connect_port) +{ + struct msg_header hdr; + struct exec_params exec_params; + struct service_params srv_params; + + hdr.type = MSG_SERVICE_CONNECT; + hdr.len = sizeof(exec_params) + sizeof(srv_params); + + exec_params.connect_domain = connect_domain; + exec_params.connect_port = connect_port; + strncpy(srv_params.ident, conn_ident, sizeof(srv_params.ident) - 1); + srv_params.ident[sizeof(srv_params.ident) - 1] = '\0'; + + if (!write_all(s, &hdr, sizeof(hdr)) + || !write_all(s, &exec_params, sizeof(exec_params)) + || !write_all(s, &srv_params, sizeof(srv_params))) { + PERROR("write daemon"); + exit(1); + } +} diff --git a/daemon/qrexec-daemon-common.h b/daemon/qrexec-daemon-common.h new file mode 100644 index 00000000..ae483a99 --- /dev/null +++ b/daemon/qrexec-daemon-common.h @@ -0,0 +1,9 @@ +extern const char *socket_dir; +int connect_unix_socket_by_id(unsigned int domid); +int connect_unix_socket(const char *domname); +int handle_daemon_handshake(int fd); +void negotiate_connection_params(int s, int other_domid, unsigned type, + void *cmdline_param, int cmdline_size, + int *data_domain, int *data_port); +void send_service_connect(int s, const char *conn_ident, + int connect_domain, int connect_port); diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index 03f20c1f..c3798de4 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -36,6 +36,7 @@ #include "qrexec.h" #include "libqrexec-utils.h" #include "../libqrexec/ioall.h" +#include "qrexec-daemon-common.h" #define QREXEC_MIN_VERSION QREXEC_PROTOCOL_V2 #define QREXEC_SOCKET_PATH "/run/qubes/policy.sock" @@ -105,7 +106,6 @@ static const char default_user_keyword[] = "DEFAULT:"; static int opt_quiet = 0; static int opt_direct = 0; -static const char *socket_dir = QREXEC_DAEMON_SOCKET_DIR; static const char *policy_program = QREXEC_POLICY_PROGRAM; #ifdef __GNUC__ @@ -1039,6 +1039,16 @@ static enum policy_response connect_daemon_socket( } } +static size_t compute_service_length(const char *const remote_cmdline) { + const size_t service_length = strlen(remote_cmdline) + 1; + if (service_length < 2 || service_length > MAX_QREXEC_CMD_LEN) { + /* This is arbitrary, but it helps reduce the risk of overflows in other code */ + errx(1, "Bad command: command line too long or empty: length %zu\n", service_length); + } + return service_length; +} + + static void handle_execute_service( const int remote_domain_id, const char *remote_domain_name, @@ -1113,6 +1123,22 @@ static void handle_execute_service( type, target_domain) <= 0) _exit(126); + char *cid; + if (asprintf(&cid, "%s,%s,%d", request_id->ident, remote_domain_name, remote_domain_id) <= 0) + _exit(126); + + const char *to_exec[] = { + "/usr/bin/qrexec-client", + "-Ed@adminvm", + "-c", + cid, + "--", + cmd, + NULL, + }; + execv(to_exec[0], (char **)to_exec); + LOG(ERROR, "execve() failed: %m"); + _exit(126); } else { char *buf; if (strncmp("@dispvm:", target, sizeof("@dispvm:") - 1) == 0) { @@ -1153,24 +1179,36 @@ static void handle_execute_service( } free(buf); } - } - char *cid; - if (asprintf(&cid, "%s,%s,%d", request_id->ident, remote_domain_name, remote_domain_id) <= 0) - _exit(126); + int s = connect_unix_socket(target); + int data_domain; + int data_port; + negotiate_connection_params(s, + remote_domain_id, + MSG_EXEC_CMDLINE, + cmd, + compute_service_length(cmd), + &data_domain, + &data_port); + int wait_connection_end = -1; + if (disposable) { + wait_connection_end = s; + } else { + close(s); + } - const char *to_exec[] = { - "/usr/bin/qrexec-client", - disposable ? "-EWkd" : "-Ed", - target, - "-c", - cid, - "--", - cmd, - NULL, - }; - execv(to_exec[0], (char **)to_exec); - LOG(ERROR, "execve() failed: %m"); - _exit(126); + s = connect_unix_socket_by_id((unsigned)remote_domain_id); + send_service_connect(s, request_id->ident, data_domain, data_port); + close(s); + if (wait_connection_end != -1) { + /* wait for EOF */ + struct pollfd fds[1] = { + { .fd = wait_connection_end, .events = POLLIN | POLLHUP, .revents = 0 }, + }; + poll(fds, 1, -1); + size_t l; + qubesd_call(target, "admin.vm.Kill", "", &l); + } + } } diff --git a/fuzz/Makefile b/fuzz/Makefile index 01aa89de..9cfa3168 100644 --- a/fuzz/Makefile +++ b/fuzz/Makefile @@ -38,7 +38,7 @@ test-%: % %_seed_corpus.zip unzip $<_seed_corpus.zip ./$< $<_seed_corpus -runs=100000 -qrexec_daemon_fuzzer: qrexec_daemon_fuzzer.o fuzz.o $(LIBQREXEC_OBJS) daemon-qrexec-daemon.o +qrexec_daemon_fuzzer: qrexec_daemon_fuzzer.o fuzz.o $(LIBQREXEC_OBJS) daemon-qrexec-daemon.o daemon-qrexec-daemon-common.o %_fuzzer: %_fuzzer.o fuzz.o $(LIBQREXEC_OBJS) $(CXX) $(CXXFLAGS) -o $@ $^ $(LIB_FUZZING_ENGINE)