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.

daemon/qrexec-daemon.c

@@ -1262,6 +1262,23 @@ 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 '+':
+    // TODO: should '-' be blocked, both here and in the first character of the argument?
+    // 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 +1313,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 +1350,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;

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))