From 365807945bf922084cf771efdb2ee744ddaab0a5 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 18 May 2024 15:24:50 -0400 Subject: [PATCH 1/4] Always send EOF on stderr Previously, this was not guaranteed to happen if the process was exiting, and MSG_DATA_STDERR would not be sent for socket services at all. There is still a case where an empty MSG_DATA_STDOUT might not be sent. Fixes: QubesOS/qubes-issues#9142 Fixes: QubesOS/qubes-issues#9248 --- daemon/qrexec-daemon-common.c | 14 ++++-- libqrexec/process_io.c | 5 +++ qrexec/tests/socket/agent.py | 85 ++++++----------------------------- qrexec/tests/socket/daemon.py | 1 + 4 files changed, 30 insertions(+), 75 deletions(-) diff --git a/daemon/qrexec-daemon-common.c b/daemon/qrexec-daemon-common.c index fa4a89ce..3d42ab5e 100644 --- a/daemon/qrexec-daemon-common.c +++ b/daemon/qrexec-daemon-common.c @@ -386,9 +386,15 @@ int prepare_local_fds(struct qrexec_parsed_command *command, struct buffer *stdi // See also qrexec-agent/qrexec-agent-data.c static void handle_failed_exec(libvchan_t *data_vchan, bool is_service, int exit_code) { - struct msg_header hdr = { - .type = MSG_DATA_STDOUT, - .len = 0, + const struct msg_header hdr[2] = { + { + .type = MSG_DATA_STDERR, + .len = 0, + }, + { + .type = MSG_DATA_STDOUT, + .len = 0, + }, }; LOG(ERROR, "failed to spawn process, exiting"); @@ -404,7 +410,7 @@ static void handle_failed_exec(libvchan_t *data_vchan, bool is_service, int exit * when we support sockets as a local process. */ if (is_service) { - libvchan_send(data_vchan, &hdr, sizeof(hdr)); + libvchan_send(data_vchan, hdr, sizeof(hdr)); send_exit_code(data_vchan, exit_code); } } diff --git a/libqrexec/process_io.c b/libqrexec/process_io.c index c65b48b6..aba6b78d 100644 --- a/libqrexec/process_io.c +++ b/libqrexec/process_io.c @@ -124,6 +124,11 @@ int qrexec_process_io(const struct process_io_request *req, struct timespec normal_timeout = { 10, 0 }; struct prefix_data empty = { 0, 0 }, prefix = req->prefix_data; + if (is_service && stderr_fd == -1) { + struct msg_header hdr = { .type = MSG_DATA_STDERR, .len = 0 }; + libvchan_send(vchan, &hdr, (int)sizeof(hdr)); + } + struct buffer remote_buffer = { .data = malloc(max_chunk_size), .buflen = max_chunk_size, diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 7b2f873a..6efc20fc 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -696,16 +696,7 @@ def test_socket_null_argument_finds_service_for_empty_argument(self): 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.assertExpectedStdout(target, b"stdout data") self.check_dom0(dom0) def _test_connect_socket_bad_config(self, forbidden_key): @@ -725,7 +716,6 @@ def _test_connect_socket_bad_config(self, forbidden_key): target, dom0 = self.execute_qubesrpc("qubes.SocketService+arg2", "domX") messages = target.recv_all_messages() - # No stderr self.assertListEqual( util.sort_messages(messages), [ @@ -765,14 +755,11 @@ def test_connect_socket_exit_on_stdin_eof(self): target.send_message(qrexec.MSG_DATA_STDIN, b"") # Check for EOF on stdin self.assertEqual(server.recvall(len(message) + 1), message) - messages = target.recv_all_messages() - # No stderr - self.assertListEqual( - util.sort_messages(messages), + self.assertEqual(target.recv_all_messages(), [ + (qrexec.MSG_DATA_STDERR, b""), (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), - ], - ) + ]) self.check_dom0(dom0) server.close() @@ -800,15 +787,7 @@ def test_connect_socket_exit_on_stdout_eof(self): # Trigger EOF on stdout server.shutdown(socket.SHUT_WR) # Server should exit - messages = target.recv_all_messages() - # No stderr - self.assertListEqual( - util.sort_messages(messages), - [ - (qrexec.MSG_DATA_STDOUT, b""), - (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), - ], - ) + self.assertExpectedStdout(target, b"") self.check_dom0(dom0) server.close() @@ -836,16 +815,7 @@ def test_connect_socket_no_metadata(self): server.sendall(b"stdout data") 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.assertExpectedStdout(target, b"stdout data") self.check_dom0(dom0) def test_connect_socket_tcp(self): @@ -880,20 +850,13 @@ def _test_tcp_raw(self, family: int, service: str, host: str, port: int, accept= self.assertEqual(server.recvall(len(message)), message) server.sendall(b"stdout data") server.close() - messages = target.recv_all_messages() self.check_dom0(dom0) - return util.sort_messages(messages) + return target def _test_tcp(self, family: int, service: str, host: str, port: int) -> None: - # No stderr - self.assertListEqual( + self.assertExpectedStdout( self._test_tcp_raw(family, service, host, port), - [ - (qrexec.MSG_DATA_STDOUT, b"stdout data"), - (qrexec.MSG_DATA_STDOUT, b""), - (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), - ], - ) + b"stdout data") def test_connect_socket_tcp_port_from_arg(self): socket_path = os.path.join( @@ -930,13 +893,9 @@ def test_connect_socket_tcp_ipv6_service_arg(self): host = "::1" os.symlink(f"/dev/tcp", socket_path) service = f"qubes.SocketService+{host.replace(':', '+')}+{port}" - self.assertListEqual( + self.assertExpectedStdout( self._test_tcp_raw(socket.AF_INET6, service, host, port, skip=False), - [ - (qrexec.MSG_DATA_STDOUT, b"stdout data"), - (qrexec.MSG_DATA_STDOUT, b""), - (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), - ], + b"stdout data", ) def _test_connect_socket_tcp_unexpected_host(self, host): @@ -946,16 +905,9 @@ def _test_connect_socket_tcp_unexpected_host(self, host): port = 65535 path = f"/dev/tcp/{host}" os.symlink(path, socket_path) - messages = self._test_tcp_raw(socket.AF_INET, f"qubes.SocketService+{host}+{port}", + target = self._test_tcp_raw(socket.AF_INET, f"qubes.SocketService+{host}+{port}", host, port, accept=False) - self.assertListEqual( - messages, - [ - (qrexec.MSG_DATA_STDOUT, b""), - (qrexec.MSG_DATA_STDERR, b""), - (qrexec.MSG_DATA_EXIT_CODE, b"\175\0\0\0"), - ], - ) + self.assertExpectedStdout(target, b"", exit_code=125) def test_connect_socket_tcp_missing_host(self): """ @@ -1063,16 +1015,7 @@ def test_connect_socket(self): server.sendall(b"stdout data") 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.assertExpectedStdout(target, b"stdout data") self.check_dom0(dom0) def test_service_close_stdout_stderr_early(self): diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index c3d6d78a..f605d70c 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -768,6 +768,7 @@ def connect_service_request(self, cmd, timeout=None): source.accept() source.handshake() + self.assertEqual(source.recv_message(), (qrexec.MSG_DATA_STDERR, b"")) return source def test_run_dom0_command_and_connect_vm(self): From 50bc9db0ae4d1ac30171633eff20af57b73c4326 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 19 May 2024 18:14:09 -0400 Subject: [PATCH 2/4] Send EOF on stdout when exiting due to EOF on stdin Previously, if a service with exit-on-client-eof=true received EOF on stdin while stdout was still open, no EOF would be sent on stdout. This is a (currently harmless) violation of the qrexec protocol. Fix this by sending an empty MSG_DATA_STDOUT to indicate EOF on stdout. Fixes: QubesOS/qubes-issues#9429 --- libqrexec/process_io.c | 8 ++++++++ qrexec/tests/socket/agent.py | 1 + 2 files changed, 9 insertions(+) diff --git a/libqrexec/process_io.c b/libqrexec/process_io.c index aba6b78d..f100fc72 100644 --- a/libqrexec/process_io.c +++ b/libqrexec/process_io.c @@ -169,6 +169,14 @@ int qrexec_process_io(const struct process_io_request *req, /* Convenience macros that eliminate a ton of error-prone boilerplate */ #define close_stdin() do { \ if (exit_on_stdin_eof) { \ + /* If stdout is still open, send EOF */ \ + if (stdout_fd != -1) { \ + const struct msg_header hdr = { \ + .type = stdout_msg_type, \ + .len = 0, \ + }; \ + libvchan_send(vchan, &hdr, sizeof(hdr)); \ + }; \ /* Set stdin_fd and stdout_fd to -1. \ * No need to close them as the process \ * will soon exit. */ \ diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 6efc20fc..32260a05 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -758,6 +758,7 @@ def test_connect_socket_exit_on_stdin_eof(self): self.assertEqual(target.recv_all_messages(), [ (qrexec.MSG_DATA_STDERR, b""), + (qrexec.MSG_DATA_STDOUT, b""), (qrexec.MSG_DATA_EXIT_CODE, b"\0\0\0\0"), ]) self.check_dom0(dom0) From 0701529b55d1f831c179aa2dd5bf0588d6983953 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 21 May 2024 21:38:58 -0400 Subject: [PATCH 3/4] Send EOF whenever closing stdout This is expected by the protocol. --- libqrexec/process_io.c | 5 +++++ qrexec/tests/socket/agent.py | 37 ++++++++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/libqrexec/process_io.c b/libqrexec/process_io.c index f100fc72..79df8bbf 100644 --- a/libqrexec/process_io.c +++ b/libqrexec/process_io.c @@ -333,6 +333,11 @@ int qrexec_process_io(const struct process_io_request *req, * local FDs. However, don't exit yet, because there might * still be some data in stdin_buf waiting to be flushed. */ + if (stdout_fd != -1) { + /* Send EOF */ + struct msg_header hdr = { .type = stdout_msg_type, .len = 0, }; + libvchan_send(vchan, &hdr, (int)sizeof(hdr)); + } close_stdout(); close_stderr(stderr_fd); stderr_fd = -1; diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index 32260a05..af9c15d7 100644 --- a/qrexec/tests/socket/agent.py +++ b/qrexec/tests/socket/agent.py @@ -1413,6 +1413,37 @@ def test_run_client_bidirectional_shutdown(self): remote.close() local.close() + def test_run_client_bidirectional_shutdown_early_exit(self): + try: + remote, local = socket.socketpair() + target_client = self.run_service(stdio=remote) + initial_data = b"stdout data\n" + target_client.send_message(qrexec.MSG_DATA_STDOUT, initial_data) + # FIXME: data can be received in multiple messages + self.assertEqual(local.recv(len(initial_data)), initial_data) + initial_data = b"stdin data\n" + local.sendall(initial_data) + self.assertStdoutMessages(target_client, initial_data, qrexec.MSG_DATA_STDIN) + target_client.send_message(qrexec.MSG_DATA_STDOUT, b"") + # Check that EOF got propagated on this side too, even though + # we still have a reference to the socket. This indicates that + # qrexec-client-vm shut down the socket for writing. + self.assertEqual(local.recv(1), b"") + with self.assertRaises(BrokenPipeError): + remote.send(b"a") + target_client.send_message( + qrexec.MSG_DATA_EXIT_CODE, struct.pack(" Date: Tue, 21 May 2024 21:42:10 -0400 Subject: [PATCH 4/4] Avoid closing stderr when MSG_DATA_EXIT_CODE is received MSG_DATA_EXIT_CODE is ignored by services, and clients always have stderr_fd == -1. --- libqrexec/process_io.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/libqrexec/process_io.c b/libqrexec/process_io.c index 79df8bbf..c1dd8714 100644 --- a/libqrexec/process_io.c +++ b/libqrexec/process_io.c @@ -339,8 +339,6 @@ int qrexec_process_io(const struct process_io_request *req, libvchan_send(vchan, &hdr, (int)sizeof(hdr)); } close_stdout(); - close_stderr(stderr_fd); - stderr_fd = -1; break; } if (prefix.len > 0 || (stdout_fd >= 0 && fds[FD_STDOUT].revents)) {