From 66c15196232c7690899af37ba731af0885f6e797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 3 Dec 2024 05:32:23 +0100 Subject: [PATCH 1/2] Try to retrieve exit code of a failed service call When service call fails, the remote (service) side sends EOF + exit code and closes the vchan immediately. If the client side tries to send something, it will fail and exit immediately (with code 1). But reading data that was queued in the vchan before its closing is still possible. So, on send error check if there is anything interesting to receive (especially exit code, but potentially also some service output) and, if yes, don't exit immediately. Since the service exit code is sent last (after all stdout/stderr data and their EOF), it's okay to check just for remote_status. This is relevant only to the service client side, as exit status of the service-handling process is not relevant. Fixes QubesOS/qubes-issues#9618 --- libqrexec/process_io.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/libqrexec/process_io.c b/libqrexec/process_io.c index c1dd8714..b0524876 100644 --- a/libqrexec/process_io.c +++ b/libqrexec/process_io.c @@ -346,7 +346,16 @@ int qrexec_process_io(const struct process_io_request *req, vchan, stdout_fd, stdout_msg_type, &prefix, &remote_buffer)) { case REMOTE_ERROR: - handle_vchan_error("send(handle_input stdout)"); + if (!is_service && remote_status == -1) { + /* Even if sending fails, still try to read remaining + * data, if any - especially the exit code. But don't + * send anything anymore. + */ + LOG(ERROR, "Error while vchan send (handle_input stdout), reading remaining data"); + close_stdout(); + } else { + handle_vchan_error("send(handle_input stdout)"); + } break; case REMOTE_EOF: close_stdout(); From df250904469ac74c79c845a35c6cb9ca7acb126e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 10 Dec 2024 22:36:46 +0100 Subject: [PATCH 2/2] Add a test for qrexec-client race condition Check if the service exit code is correctly retrieved even if the the service terminates at the exact moment the qrexec-client tries to send some data. Try to win the race by initially sending SIGSTOP to the qrexec-client process, and sending SIGCONT only after preparing both local and remote data streams. qrexec-client will handle local data stream first, at which point remote socket is already closed. Similar issue applies to qrexec-client-vm, but since the implementation is shared, one test is enough. QubesOS/qubes-issues#9618 --- qrexec/tests/socket/daemon.py | 61 +++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py index 2b6b9dea..d561e740 100644 --- a/qrexec/tests/socket/daemon.py +++ b/qrexec/tests/socket/daemon.py @@ -28,6 +28,7 @@ import time import itertools import socket +import signal import psutil import pytest @@ -664,6 +665,7 @@ def start_client(self, args): self.client = subprocess.Popen( cmd, env=env, + stdin=subprocess.PIPE, stdout=subprocess.PIPE, ) self.addCleanup(self.stop_client) @@ -744,6 +746,65 @@ def test_run_vm_command_from_dom0(self): self.client.wait() self.assertEqual(self.client.returncode, 42) + def test_run_vm_command_from_dom0_reject_stdin(self): + """Test if qrexec-client properly returns remote exit code even if + service didn't read all of stdin""" + cmd = "user:command" + target_domain_name = "target_domain" + target_domain_uuid = "d95e1147-2d82-4595-90bb-5a7500cc3196" + target_domain = 42 + target_port = 513 + + target_daemon = self.connect_daemon( + target_domain, target_domain_name, target_domain_uuid + ) + self.start_client(["-d", target_domain_name, cmd]) + target_daemon.accept() + target_daemon.handshake() + + # negotiate_connection_params + self.assertEqual( + target_daemon.recv_message(), + ( + qrexec.MSG_EXEC_CMDLINE, + struct.pack("