From 1f13095368ec41bc48d8ce70046c4d6582980a41 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 16 Apr 2024 17:25:04 -0400 Subject: [PATCH 1/8] Add exit codes to qrexec.h They were open-coded all over the place, and status 126 ("Request refused") used when other statuses would be more appropriate. This just adds the exit codes. It doesn't change any existing behavior. --- libqrexec/qrexec.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libqrexec/qrexec.h b/libqrexec/qrexec.h index a466c006..462f5464 100644 --- a/libqrexec/qrexec.h +++ b/libqrexec/qrexec.h @@ -173,4 +173,11 @@ enum { // support only very small configuration files, #define MAX_CONFIG_SIZE 4096 +// Exit codes + +// Service call refused +#define QREXEC_EXIT_REQUEST_REFUSED 126 +// Problem with qrexec itself +#define QREXEC_EXIT_PROBLEM 125 + #endif /* QREXEC_H */ From c664954bf06a9d4c0411ab624b7c60440413344d Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 11 Apr 2024 20:14:04 -0400 Subject: [PATCH 2/8] Avoid using alarm(2) for timeouts libvchan has much better ways of dealing with timeouts now, so use them instead of calling an async signal unsafe function from a signal handler. Both qrexec-agent-data.c and qrexec-daemon-common.c reimplemented timeouts, so write common code in libqrexec for both to use. --- agent/qrexec-agent-data.c | 68 ++--------------------------------- daemon/qrexec-client.c | 62 +++++--------------------------- daemon/qrexec-daemon-common.c | 27 +++++++------- fuzz/Makefile | 2 +- fuzz/fuzz.c | 11 ++++++ fuzz/fuzz.h | 2 ++ fuzz/mock-fuzz.h | 2 ++ libqrexec/Makefile | 2 +- libqrexec/libqrexec-utils.h | 12 +++++++ libqrexec/vchan_timeout.c | 67 ++++++++++++++++++++++++++++++++++ 10 files changed, 120 insertions(+), 135 deletions(-) create mode 100644 libqrexec/vchan_timeout.c diff --git a/agent/qrexec-agent-data.c b/agent/qrexec-agent-data.c index f84cff61..e5cc3bb9 100644 --- a/agent/qrexec-agent-data.c +++ b/agent/qrexec-agent-data.c @@ -150,69 +150,6 @@ static int handle_just_exec(struct qrexec_parsed_command *cmd) return 0; } -static const long BILLION_NANOSECONDS = 1000000000L; - -static int wait_for_vchan_connection_with_timeout( - libvchan_t *conn, int wait_fd, bool is_server, time_t timeout) { - struct timespec end_tp, now_tp, timeout_tp; - - if (timeout && clock_gettime(CLOCK_MONOTONIC, &end_tp)) { - PERROR("clock_gettime"); - return -1; - } - assert(end_tp.tv_nsec >= 0 && end_tp.tv_nsec < BILLION_NANOSECONDS); - end_tp.tv_sec += timeout; - while (true) { - bool did_timeout = true; - struct pollfd fds = { .fd = wait_fd, .events = POLLIN | POLLHUP, .revents = 0 }; - - /* calculate how much time left until connection timeout expire */ - if (clock_gettime(CLOCK_MONOTONIC, &now_tp)) { - PERROR("clock_gettime"); - return -1; - } - assert(now_tp.tv_nsec >= 0 && now_tp.tv_nsec < BILLION_NANOSECONDS); - if (now_tp.tv_sec <= end_tp.tv_sec) { - timeout_tp.tv_sec = end_tp.tv_sec - now_tp.tv_sec; - timeout_tp.tv_nsec = end_tp.tv_nsec - now_tp.tv_nsec; - if (timeout_tp.tv_nsec < 0) { - timeout_tp.tv_nsec += BILLION_NANOSECONDS; - timeout_tp.tv_sec--; - } - did_timeout = timeout_tp.tv_sec < 0; - } - switch (did_timeout ? 0 : ppoll(&fds, 1, &timeout_tp, NULL)) { - case -1: - if (errno == EINTR) - break; - LOG(ERROR, "vchan connection error"); - return -1; - case 0: - LOG(ERROR, "vchan connection timeout"); - return -1; - case 1: - break; - default: - abort(); - } - if (fds.revents & POLLIN) { - if (is_server) { - libvchan_wait(conn); - return 0; - } else { - int connect_ret = libvchan_client_init_async_finish(conn, true); - - if (connect_ret < 0) { - LOG(ERROR, "vchan connection error"); - return -1; - } else if (connect_ret == 0) { - return 0; - } - } - } - } -} - /* Behaviour depends on type parameter: * MSG_JUST_EXEC - connect to vchan server, fork+exec process given by cmdline @@ -253,7 +190,8 @@ static int handle_new_process_common( LOG(ERROR, "Data vchan connection failed"); exit(1); } - if (wait_for_vchan_connection_with_timeout(data_vchan, wait_fd, false, connection_timeout) < 0) { + if (qubes_wait_for_vchan_connection_with_timeout( + data_vchan, wait_fd, false, connection_timeout) < 0) { LOG(ERROR, "Data vchan connection failed"); exit(1); } @@ -381,7 +319,7 @@ int handle_data_client( LOG(ERROR, "Data vchan connection failed"); exit(1); } - if (wait_for_vchan_connection_with_timeout( + if (qubes_wait_for_vchan_connection_with_timeout( data_vchan, libvchan_fd_for_select(data_vchan), true, connection_timeout) < 0) { LOG(ERROR, "Data vchan connection failed"); exit(1); diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index fe84005e..848a270a 100644 --- a/daemon/qrexec-client.c +++ b/daemon/qrexec-client.c @@ -143,57 +143,6 @@ static void parse_connect(char *str, char **request_id, exit(1); } -static const long BILLION_NANOSECONDS = 1000000000L; - -static void wait_for_vchan_client_with_timeout(libvchan_t *conn, time_t timeout) { - struct timespec end_tp, now_tp, timeout_tp; - - if (timeout && clock_gettime(CLOCK_MONOTONIC, &end_tp)) { - PERROR("clock_gettime"); - exit(1); - } - assert(end_tp.tv_nsec >= 0 && end_tp.tv_nsec < BILLION_NANOSECONDS); - end_tp.tv_sec += timeout; - int const fd = libvchan_fd_for_select(conn); - while (conn && libvchan_is_open(conn) == VCHAN_WAITING) { - if (timeout) { - bool did_timeout = true; - struct pollfd fds = { .fd = fd, .events = POLLIN | POLLHUP, .revents = 0 }; - - /* calculate how much time left until connection timeout expire */ - if (clock_gettime(CLOCK_MONOTONIC, &now_tp)) { - PERROR("clock_gettime"); - exit(1); - } - assert(now_tp.tv_nsec >= 0 && now_tp.tv_nsec < BILLION_NANOSECONDS); - if (now_tp.tv_sec <= end_tp.tv_sec) { - timeout_tp.tv_sec = end_tp.tv_sec - now_tp.tv_sec; - timeout_tp.tv_nsec = end_tp.tv_nsec - now_tp.tv_nsec; - if (timeout_tp.tv_nsec < 0) { - timeout_tp.tv_nsec += BILLION_NANOSECONDS; - timeout_tp.tv_sec--; - } - did_timeout = timeout_tp.tv_sec < 0; - } - switch (did_timeout ? 0 : ppoll(&fds, 1, &timeout_tp, NULL)) { - case -1: - if (errno == EINTR) - break; - LOG(ERROR, "vchan connection error"); - exit(1); - case 0: - LOG(ERROR, "vchan connection timeout"); - exit(1); - case 1: - break; - default: - abort(); - } - } - libvchan_wait(conn); - } -} - static size_t compute_service_length(const char *const remote_cmdline, const char *const prog_name) { const size_t service_length = strlen(remote_cmdline) + 1; if (service_length < 2 || service_length > MAX_QREXEC_CMD_LEN) { @@ -365,9 +314,16 @@ int main(int argc, char **argv) VCHAN_BUFFER_SIZE, VCHAN_BUFFER_SIZE); if (!data_vchan) { LOG(ERROR, "Failed to start data vchan server"); - exit(1); + rc = QREXEC_EXIT_PROBLEM; + goto cleanup; + } + int fd = libvchan_fd_for_select(data_vchan); + if (qubes_wait_for_vchan_connection_with_timeout( + data_vchan, fd, true, connection_timeout) < 0) { + LOG(ERROR, "qrexec connection timeout"); + rc = QREXEC_EXIT_PROBLEM; + goto cleanup; } - wait_for_vchan_client_with_timeout(data_vchan, connection_timeout); struct handshake_params params = { .data_vchan = data_vchan, .stdin_buffer = &stdin_buffer, diff --git a/daemon/qrexec-daemon-common.c b/daemon/qrexec-daemon-common.c index 867e188b..bb12166a 100644 --- a/daemon/qrexec-daemon-common.c +++ b/daemon/qrexec-daemon-common.c @@ -330,12 +330,6 @@ static int select_loop(struct handshake_params *params) return (params->exit_with_code ? exit_code : 0); } -static void sigalrm_handler(int x __attribute__((__unused__))) -{ - LOG(ERROR, "vchan connection timeout"); - _exit(1); -} - int run_qrexec_to_dom0(const struct service_params *svc_params, int src_domain_id, const char *src_domain_name, @@ -374,15 +368,18 @@ int run_qrexec_to_dom0(const struct service_params *svc_params, } else { prepare_ret = prepare_local_fds(command, &stdin_buffer); } - void (*old_handler)(int); - - /* libvchan_client_init is blocking and does not support connection - * timeout, so use alarm(2) for that... */ - old_handler = signal(SIGALRM, sigalrm_handler); - alarm(connection_timeout); - data_vchan = libvchan_client_init(data_domain, data_port); - alarm(0); - signal(SIGALRM, old_handler); + int wait_fd; + data_vchan = libvchan_client_init_async(data_domain, data_port, &wait_fd); + if (!data_vchan) { + LOG(ERROR, "Cannot create data vchan connection"); + return QREXEC_EXIT_PROBLEM; + } + if (qubes_wait_for_vchan_connection_with_timeout( + data_vchan, wait_fd, false, connection_timeout) < 0) { + LOG(ERROR, "qrexec connection timeout"); + return QREXEC_EXIT_PROBLEM; + } + struct handshake_params params = { .data_vchan = data_vchan, .stdin_buffer = &stdin_buffer, diff --git a/fuzz/Makefile b/fuzz/Makefile index d21d5603..bc510ce7 100644 --- a/fuzz/Makefile +++ b/fuzz/Makefile @@ -16,7 +16,7 @@ CXXFLAGS += -O1 -fno-omit-frame-pointer -gline-tables-only \ -fsanitize-address-use-after-scope -fsanitize=fuzzer endif -_LIBQREXEC_OBJS = remote.o write-stdin.o ioall.o txrx-vchan.o buffer.o replace.o exec.o log.o unix-server.o toml.o process_io.o +_LIBQREXEC_OBJS = remote.o write-stdin.o ioall.o txrx-vchan.o buffer.o replace.o exec.o log.o unix-server.o toml.o process_io.o vchan_timeout.o LIBQREXEC_OBJS = $(patsubst %.o,libqrexec-%.o,$(_LIBQREXEC_OBJS)) FUZZERS = qubesrpc_parse_fuzzer qrexec_remote_fuzzer qrexec_daemon_fuzzer diff --git a/fuzz/fuzz.c b/fuzz/fuzz.c index cf2e7f55..7579b711 100644 --- a/fuzz/fuzz.c +++ b/fuzz/fuzz.c @@ -140,7 +140,18 @@ ssize_t fuzz_write(int fd, const void *buf, size_t count) { return count; } +typedef int EVTCHN; fuzz_file_t *fuzz_libvchan_client_init(int domain, int port) { /* not implemented yet */ abort(); } + +fuzz_file_t *fuzz_libvchan_client_init_async(int domain, int port, EVTCHN *watch_fd) { + /* not implemented yet */ + abort(); +} + +int fuzz_libvchan_client_init_async_finish(fuzz_file_t *ctrl, bool blocking) { + /* not implemented yet */ + abort(); +} diff --git a/fuzz/fuzz.h b/fuzz/fuzz.h index aa0d56d7..9a879002 100644 --- a/fuzz/fuzz.h +++ b/fuzz/fuzz.h @@ -31,6 +31,8 @@ int fuzz_libvchan_is_open(fuzz_file_t *file); int fuzz_libvchan_data_ready(fuzz_file_t *file); int fuzz_libvchan_buffer_space(fuzz_file_t *file); fuzz_file_t *fuzz_libvchan_client_init(int domain, int port); +fuzz_file_t *fuzz_libvchan_client_init_async(int domain, int port, int *fd); +int fuzz_libvchan_client_init_async_finish(fuzz_file_t *file, bool blocking); ssize_t fuzz_read(int fd, void *buf, size_t count); ssize_t fuzz_write(int fd, const void *buf, size_t count); diff --git a/fuzz/mock-fuzz.h b/fuzz/mock-fuzz.h index d8eda723..f8769ec0 100644 --- a/fuzz/mock-fuzz.h +++ b/fuzz/mock-fuzz.h @@ -16,6 +16,8 @@ #define libvchan_data_ready fuzz_libvchan_data_ready #define libvchan_buffer_space fuzz_libvchan_buffer_space #define libvchan_client_init fuzz_libvchan_client_init +#define libvchan_client_init_async fuzz_libvchan_client_init_async +#define libvchan_client_init_async_finish fuzz_libvchan_client_init_async_finish #define read fuzz_read #define write fuzz_write diff --git a/libqrexec/Makefile b/libqrexec/Makefile index cfe4ef2d..baecffa6 100644 --- a/libqrexec/Makefile +++ b/libqrexec/Makefile @@ -21,7 +21,7 @@ endif all: libqrexec-utils.so -libqrexec-utils.so.$(SO_VER): unix-server.o ioall.o buffer.o exec.o txrx-vchan.o write-stdin.o replace.o remote.o process_io.o log.o toml.o +libqrexec-utils.so.$(SO_VER): unix-server.o ioall.o buffer.o exec.o txrx-vchan.o write-stdin.o replace.o remote.o process_io.o log.o toml.o vchan_timeout.o $(CC) $(LDFLAGS) -Wl,-soname,$@ -o $@ $^ $(VCHANLIBS) libqrexec-utils.so: libqrexec-utils.so.$(SO_VER) diff --git a/libqrexec/libqrexec-utils.h b/libqrexec/libqrexec-utils.h index a042577c..9e312712 100644 --- a/libqrexec/libqrexec-utils.h +++ b/libqrexec/libqrexec-utils.h @@ -32,6 +32,7 @@ #include #include +#include #include #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION @@ -347,4 +348,15 @@ void *qubes_read_all_to_malloc(int fd, size_t initial_buffer_size, size_t max_by */ bool qubes_sendmsg_all(struct msghdr *msg, int sock); +/** + * Wait for a vchan connection with a timeout. + * + * @param conn the vchan + * @param wait_fd The FD set by libvchan_client_init_async() for clients, + * or the FD returned by libvchan_fd_for_select() for servers. + * @param is_server Is this a server or a client vchan? + * @param timeout The timeout to use. + */ +int qubes_wait_for_vchan_connection_with_timeout( + libvchan_t *conn, int wait_fd, bool is_server, time_t timeout); #endif /* LIBQREXEC_UTILS_H */ diff --git a/libqrexec/vchan_timeout.c b/libqrexec/vchan_timeout.c new file mode 100644 index 00000000..e15a8a0c --- /dev/null +++ b/libqrexec/vchan_timeout.c @@ -0,0 +1,67 @@ +#include +#include +#include +#include + +static const long BILLION_NANOSECONDS = 1000000000L; + +int qubes_wait_for_vchan_connection_with_timeout( + libvchan_t *conn, int wait_fd, bool is_server, time_t timeout) { + struct timespec end_tp, now_tp, timeout_tp; + + if (clock_gettime(CLOCK_MONOTONIC, &end_tp)) { + PERROR("clock_gettime"); + return -1; + } + assert(end_tp.tv_nsec >= 0 && end_tp.tv_nsec < BILLION_NANOSECONDS); + end_tp.tv_sec += timeout; + for (;;) { + bool did_timeout = true; + struct pollfd fds = { .fd = wait_fd, .events = POLLIN | POLLHUP, .revents = 0 }; + + /* calculate how much time left until connection timeout expire */ + if (clock_gettime(CLOCK_MONOTONIC, &now_tp)) { + PERROR("clock_gettime"); + return -1; + } + assert(now_tp.tv_nsec >= 0 && now_tp.tv_nsec < BILLION_NANOSECONDS); + if (now_tp.tv_sec <= end_tp.tv_sec) { + timeout_tp.tv_sec = end_tp.tv_sec - now_tp.tv_sec; + timeout_tp.tv_nsec = end_tp.tv_nsec - now_tp.tv_nsec; + if (timeout_tp.tv_nsec < 0) { + timeout_tp.tv_nsec += BILLION_NANOSECONDS; + timeout_tp.tv_sec--; + } + did_timeout = timeout_tp.tv_sec < 0; + } + switch (did_timeout ? 0 : ppoll(&fds, 1, &timeout_tp, NULL)) { + case -1: + if (errno == EINTR) + break; + LOG(ERROR, "vchan connection error"); + return -1; + case 0: + LOG(ERROR, "vchan connection timeout"); + return -1; + case 1: + break; + default: + abort(); + } + if (fds.revents & POLLIN) { + if (is_server) { + libvchan_wait(conn); + return 0; + } else { + int connect_ret = libvchan_client_init_async_finish(conn, true); + + if (connect_ret < 0) { + LOG(ERROR, "vchan connection error"); + return -1; + } else if (connect_ret == 0) { + return 0; + } + } + } + } +} From f3768d23b25e17ae21dd53bdb6dcfd2d5622d2b4 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 12 Apr 2024 17:08:52 -0400 Subject: [PATCH 3/8] Use sigemptyset() to initialize signal sets This is required by POSIX and musl libc in at least some situations. --- daemon/qrexec-daemon.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index 87993071..118b1213 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -413,6 +413,8 @@ static void init(int xid) .sa_handler = signal_handler, .sa_flags = 0, }; + sigemptyset(&sigchld_action.sa_mask); + sigemptyset(&sigterm_action.sa_mask); if (signal(SIGPIPE, SIG_IGN) == SIG_ERR) err(1, "signal"); if (sigaction(SIGCHLD, &sigchld_action, NULL)) From 3d658cc82739a6cb8ff8366737003a4f56ecdd35 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 12 Apr 2024 16:55:11 -0400 Subject: [PATCH 4/8] Use a pipe instead of signals to notify readiness This avoids using async signal unsafe functions in a signal handler. It also reduces the total amount of code. --- daemon/qrexec-daemon.c | 60 +++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index 118b1213..a4626a20 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -120,7 +120,6 @@ static const char *policy_program = QREXEC_POLICY_PROGRAM; # define UNUSED(x) UNUSED_ ## x #endif -static volatile int children_count; static volatile int child_exited; static volatile int terminate_requested; @@ -135,24 +134,6 @@ static #endif int protocol_version; -static void sigusr1_handler(int UNUSED(x)) -{ - if (!opt_quiet) - LOG(INFO, "Connected to VM"); - exit(0); -} - -static void sigchld_parent_handler(int UNUSED(x)) -{ - children_count--; - /* starting value is 0 so we see dead real qrexec-daemon as -1 */ - if (children_count < 0) { - LOG(ERROR, "Connection to the VM failed"); - exit(1); - } -} - - static char *remote_domain_name; // guess what static int remote_domain_id; @@ -308,29 +289,41 @@ static void init(int xid) startup_timeout = MAX_STARTUP_TIME_DEFAULT; } + int pipes[2]; if (!opt_direct) { - struct sigaction action = { - .sa_handler = sigusr1_handler, - .sa_flags = 0, - }; - if (sigaction(SIGUSR1, &action, NULL)) - err(1, "sigaction"); - action.sa_handler = sigchld_parent_handler; - if (sigaction(SIGCHLD, &action, NULL)) - err(1, "sigaction"); + if (pipe2(pipes, O_CLOEXEC)) + err(1, "pipe2()"); switch (pid=fork()) { case -1: PERROR("fork"); exit(1); case 0: + close(pipes[0]); break; default: if (getenv("QREXEC_STARTUP_NOWAIT")) exit(0); + close(pipes[1]); if (!opt_quiet) LOG(ERROR, "Waiting for VM's qrexec agent."); + struct pollfd fds[1] = {{ .fd = pipes[0], .events = POLLIN | POLLHUP, .revents = 0 }}; for (i=0;i Date: Fri, 12 Apr 2024 17:04:49 -0400 Subject: [PATCH 5/8] Use SOCK_CLOEXEC instead of setting O_CLOEXEC manually No other functional change intended. --- agent/qrexec-fork-server.c | 4 ---- libqrexec/unix-server.c | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/agent/qrexec-fork-server.c b/agent/qrexec-fork-server.c index 8a8ecd6e..9edba38e 100644 --- a/agent/qrexec-fork-server.c +++ b/agent/qrexec-fork-server.c @@ -124,10 +124,6 @@ int main(int argc, char **argv) { } s = get_server_socket(socket_path); - if (fcntl(s, F_SETFD, O_CLOEXEC) < 0) { - PERROR("fcntl"); - exit(1); - } /* fork into background */ switch (fork()) { case -1: diff --git a/libqrexec/unix-server.c b/libqrexec/unix-server.c index f5c4ff36..ba046493 100644 --- a/libqrexec/unix-server.c +++ b/libqrexec/unix-server.c @@ -36,7 +36,7 @@ int get_server_socket(const char *socket_address) unlink(socket_address); - s = socket(AF_UNIX, SOCK_STREAM, 0); + s = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); if (s < 0) { PERROR("socket"); exit(1); From 61a8e1fa30429645e659c776c7f374b751dfd6f1 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 12 Apr 2024 17:05:39 -0400 Subject: [PATCH 6/8] Avoid using signal() to establish a signal handler This is explicitly warned against by man:signal(2). With glibc, its behavior depends on compiler options. Use sigaction(2) instead. --- agent/qrexec-agent-data.c | 10 ++++++++-- agent/qrexec-agent.c | 10 ++++++++-- daemon/qrexec-daemon-common.c | 8 ++++++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/agent/qrexec-agent-data.c b/agent/qrexec-agent-data.c index e5cc3bb9..d3e38378 100644 --- a/agent/qrexec-agent-data.c +++ b/agent/qrexec-agent-data.c @@ -74,8 +74,14 @@ static void sigusr1_handler(int __attribute__((__unused__))x) void prepare_child_env(void) { char pid_s[10]; - signal(SIGCHLD, sigchld_handler); - signal(SIGUSR1, sigusr1_handler); + struct sigaction action = { + .sa_handler = sigchld_handler, + .sa_flags = 0, + }; + sigemptyset(&action.sa_mask); + if (sigaction(SIGCHLD, &action, NULL)) abort(); + action.sa_handler = sigusr1_handler; + if (sigaction(SIGUSR1, &action, NULL)) abort(); int res = snprintf(pid_s, sizeof(pid_s), "%d", getpid()); if (res < 0) abort(); if (res >= (int)sizeof(pid_s)) abort(); diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index 4e10f3ef..e50f2f3e 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -928,8 +928,14 @@ int main(int argc, char **argv) } init(); - signal(SIGCHLD, sigchld_handler); - signal(SIGTERM, sigterm_handler); + struct sigaction action = { + .sa_handler = sigchld_handler, + .sa_flags = SA_RESTART, + }; + sigemptyset(&action.sa_mask); + sigaction(SIGCHLD, &action, NULL); + action.sa_handler = sigterm_handler; + sigaction(SIGTERM, &action, NULL); signal(SIGPIPE, SIG_IGN); sigemptyset(&selectmask); diff --git a/daemon/qrexec-daemon-common.c b/daemon/qrexec-daemon-common.c index bb12166a..d4bcf64d 100644 --- a/daemon/qrexec-daemon-common.c +++ b/daemon/qrexec-daemon-common.c @@ -219,7 +219,6 @@ int handle_agent_handshake(libvchan_t *vchan, bool remote_send_first) static void sigchld_handler(int x __attribute__((__unused__))) { sigchld = 1; - signal(SIGCHLD, sigchld_handler); } /* See also qrexec-agent.c:wait_for_session_maybe() */ @@ -271,7 +270,12 @@ int prepare_local_fds(struct qrexec_parsed_command *command, struct buffer *stdi { if (stdin_buffer == NULL) abort(); - if (signal(SIGCHLD, sigchld_handler) == SIG_ERR) + struct sigaction action = { + .sa_handler = sigchld_handler, + .sa_flags = 0, + }; + sigemptyset(&action.sa_mask); + if (sigaction(SIGCHLD, &action, NULL)) return 126; return execute_parsed_qubes_rpc_command(command, &local_pid, &local_stdin_fd, &local_stdout_fd, NULL, stdin_buffer); From a9aa1c5344c41cd5684d41b38d05815af735dddd Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Fri, 12 Apr 2024 18:18:03 -0400 Subject: [PATCH 7/8] Use libvchan_client_init_async() instead of parent process timeout This avoids leaving the child process running. --- daemon/qrexec-daemon.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index a4626a20..2b9e5a4e 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -307,7 +307,7 @@ static void init(int xid) if (!opt_quiet) LOG(ERROR, "Waiting for VM's qrexec agent."); struct pollfd fds[1] = {{ .fd = pipes[0], .events = POLLIN | POLLHUP, .revents = 0 }}; - for (i=0;i Date: Thu, 11 Apr 2024 13:56:02 -0400 Subject: [PATCH 8/8] Don't close file descriptor 0 Closing a standard FD is a bad idea, as it will be reused by subsequent system calls that create file descriptors. Point FD 0 at /dev/null instead. --- daemon/qrexec-daemon.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index 2b9e5a4e..6759361b 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -338,7 +338,6 @@ static void init(int xid) } } - close(0); if (chdir(socket_dir) < 0) { PERROR("chdir %s failed", socket_dir); @@ -1572,6 +1571,18 @@ int main(int argc, char **argv) int i, opt; sigset_t selectmask; + { + int null_fd = open("/dev/null", O_RDONLY|O_NOCTTY); + if (null_fd < 0) + err(1, "open(%s)", "/dev/null"); + if (null_fd > 0) { + if (dup2(null_fd, 0) != 0) + err(1, "dup2(%d, 0)", null_fd); + if (null_fd > 2 && close(null_fd) != 0) + err(1, "close(%d)", null_fd); + } + } + setup_logging("qrexec-daemon"); while ((opt=getopt_long(argc, argv, "hqp:D", longopts, NULL)) != -1) {