From 48944be517fc5567e4ea817d99e5edcde758a126 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 4 Apr 2024 15:55:01 -0400
Subject: [PATCH] Forbid empty service names in legacy MSG_TRIGGER_SERVICE

There is no point in allowing calls with an empty service name: if the
argument was empty, the call would be unconditionally rejected by the
policy daemon, and even if it was _not_ empty, libqrexec would refuse to
execute the call.  Furthermore, MSG_TRIGGER_SERVICE3 already checks for
empty service names and sends MSG_SERVICE_REFUSED without even querying
the policy daemon, so querying the policy daemon for MSG_TRIGGER_SERVICE
is inconsistent.

Fixes: QubesOS/qubes-issues#9098
---
 daemon/qrexec-daemon.c        | 57 +++++++++++++++++++++--------------
 qrexec/tests/socket/daemon.py |  1 -
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c
index 28c3cabe..6da80767 100644
--- a/daemon/qrexec-daemon.c
+++ b/daemon/qrexec-daemon.c
@@ -1262,6 +1262,21 @@ static bool validate_request_id(struct service_params *untrusted_params, const c
 
 #define ENSURE_NULL_TERMINATED(x) x[sizeof(x)-1] = 0
 
+static bool validate_service_name(char *untrusted_service_name)
+{
+    switch (untrusted_service_name[0]) {
+    case '\0':
+        LOG(ERROR, "Empty service name not allowed");
+        return false;
+    case '+':
+        LOG(ERROR, "Service name must not start with '+'");
+        return false;
+    default:
+        sanitize_name(untrusted_service_name, "+");
+        return true;
+    }
+}
+
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
 static
 #endif
@@ -1296,6 +1311,10 @@ void handle_message_from_agent(void)
                 send_service_refused(vchan, &untrusted_params.request_id);
                 return;
             }
+            if (!validate_service_name(untrusted_params.service_name)) {
+                send_service_refused(vchan, &untrusted_params.request_id);
+                return;
+            }
             params = untrusted_params;
             /* sanitize end */
 
@@ -1329,38 +1348,30 @@ void handle_message_from_agent(void)
             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 fail;
+                goto fail3;
             params3 = untrusted_params3;
             if (untrusted_service_name[service_name_len] != 0) {
                 LOG(ERROR, "Service name not NUL-terminated");
-                goto fail;
+                goto fail3;
             }
             nul_offset = strlen(untrusted_service_name);
             if (nul_offset != service_name_len) {
                 LOG(ERROR, "Service name contains NUL byte at offset %zu", nul_offset);
-                goto fail;
+                goto fail3;
             }
-            switch (untrusted_service_name[0]) {
-            case '\0':
-                LOG(ERROR, "Empty service name not allowed");
-                goto fail;
-            case '+':
-                LOG(ERROR, "Service name must not start with '+'");
-                goto fail;
-            default:
-                sanitize_name(untrusted_service_name, "+");
-                service_name = untrusted_service_name;
-                untrusted_service_name = NULL;
-                /* sanitize end */
+            if (!validate_service_name(untrusted_service_name))
+                goto fail3;
+            service_name = untrusted_service_name;
+            untrusted_service_name = NULL;
+            /* sanitize end */
 
-                handle_execute_service(remote_domain_id, remote_domain_name,
-                        params3.target_domain,
-                        service_name,
-                        &params3.request_id);
-                free(service_name);
-                return;
-            }
-fail:
+            handle_execute_service(remote_domain_id, remote_domain_name,
+                    params3.target_domain,
+                    service_name,
+                    &params3.request_id);
+            free(service_name);
+            return;
+fail3:
             send_service_refused(vchan, &untrusted_params3.request_id);
             free(untrusted_service_name);
             return;
diff --git a/qrexec/tests/socket/daemon.py b/qrexec/tests/socket/daemon.py
index 417d985f..956005ba 100644
--- a/qrexec/tests/socket/daemon.py
+++ b/qrexec/tests/socket/daemon.py
@@ -203,7 +203,6 @@ def test_bad_request_id_bad_byte_after_nul_terminator(self):
         self.assertEqual(data[:4], b"a\0b\0")
         self.assertEqual(data[4:], b"\0" * 28)
 
-    @unittest.expectedFailure
     def test_bad_old_style_request(self):
         agent = self.start_daemon_with_agent()
         agent.send_message(qrexec.MSG_HELLO, struct.pack("<L", 2))