Extra internal validation

daemon/qrexec-client.c

@@ -108,23 +108,6 @@ _Noreturn static void usage(const char *const name, int status)
     exit(status);
 }
 
-static int parse_int(const char *str, const char *msg) {
-    long value;
-    char *end = (char *)str;
-
-    if (str[0] < '0' || str[0] > '9')
-        errx(1, "%s '%s' does not start with an ASCII digit", msg, str);
-    errno = 0;
-    value = strtol(str, &end, 0);
-    if (*end != '\0')
-        errx(1, "trailing junk '%s' after %s", end, msg);
-    if (errno == 0 && (value < 0 || value > INT_MAX))
-        errno = ERANGE;
-    if (errno)
-        err(1, "invalid %s '%s': strtol", msg, str);
-    return (int)value;
-}
-
 static void parse_connect(char *str, char **request_id,
         char **src_domain_name, int *src_domain_id)
 {

daemon/qrexec-daemon-common.c

@@ -5,6 +5,8 @@
 #include <sys/wait.h>
 #include <stdio.h>
 #include <unistd.h>
+#include <err.h>
+#include <limits.h>
 
 #include "qrexec.h"
 #include "libqrexec-utils.h"
@@ -14,6 +16,23 @@
 #define QREXEC_DISPVM_PREFIX "@dispvm:"
 #define QREXEC_DISPVM_PREFIX_SIZE (sizeof QREXEC_DISPVM_PREFIX - 1)
 
+int parse_int(const char *str, const char *msg) {
+    long value;
+    char *end = (char *)str;
+
+    if (str[0] < '0' || str[0] > '9')
+        errx(1, "%s '%s' does not start with an ASCII digit", msg, str);
+    errno = 0;
+    value = strtol(str, &end, 0);
+    if (*end != '\0')
+        errx(1, "trailing junk '%s' after %s", end, msg);
+    if (errno == 0 && (value < 0 || value > INT_MAX))
+        errno = ERANGE;
+    if (errno)
+        err(1, "invalid %s '%s': strtol", msg, str);
+    return (int)value;
+}
+
 /* ask the daemon to allocate vchan port */
 bool negotiate_connection_params(int s, int other_domid, unsigned type,
         const void *cmdline_param, int cmdline_size,

daemon/qrexec-daemon-common.h

@@ -147,3 +147,12 @@ bool qrexec_execute_vm(const char *target, bool autostart, int remote_domain_id,
 extern int local_stdin_fd;
 __attribute__((warn_unused_result))
 bool target_refers_to_dom0(const char *target);
+/**
+ * Parse an integer, which must not be negative.
+ *
+ * @param str The integer.
+ * @param message A human-readable string that will be used in error messages.
+ *
+ * Returns a non-negative int on success.  Calls exit(1) on failure.
+ */
+int parse_int(const char *str, const char *msg);

daemon/qrexec-daemon.c

@@ -296,7 +296,7 @@
     }
     startup_timeout_str = getenv("QREXEC_STARTUP_TIMEOUT");
     if (startup_timeout_str) {
-        startup_timeout = atoi(startup_timeout_str);
+        startup_timeout = parse_int(startup_timeout_str, "startup timeout");
         if (startup_timeout <= 0)
             // invalid or negative number
             startup_timeout = MAX_STARTUP_TIME_DEFAULT;
@@ -1142,6 +1142,36 @@
     _exit(QREXEC_EXIT_PROBLEM);
 }
 
+/* check that the input is non-empty with only safe characters */
+static bool check_single_word(const char *token)
+{
+    const char *cursor = token;
+    switch (*cursor++) {
+    case 'A' ... 'Z':
+    case 'a' ... 'z':
+        break;
+    default:
+        return false;
+    }
+    for (;;) {
+        switch (*cursor++) {
+        case 'A' ... 'Z':
+        case 'a' ... 'z':
+        case '0' ... '9':
+        case '_':
+        case ':':
+        case '-':
+        case '.':
+        case '@': // not used today but might be in future
+            break;
+        case '\0':
+            return true;
+        default:
+            return false;
+        }
+    }
+}
+
 _Noreturn static void handle_execute_service_child(
         const int remote_domain_id,
         const char *remote_domain_name,
@@ -1167,6 +1197,19 @@
 
     if (policy_response != RESPONSE_ALLOW)
         daemon__exit(QREXEC_EXIT_REQUEST_REFUSED);
+    else {
+        const char *p = strchr(user, ':');
+        // It is possible to use a user ending in ":nogui" to emulate
+        // --no-gui on the dom0 qvm-run command line.  This is a bug,
+        // but it can be used to work around a limitation in the Windows
+        // agent, so allow it until R5.0.  However, don't mention the
+        // misfeature in the error message, so that others are not
+        // encouraged to use it.
+        if (p != NULL && strcmp(p + 1, "nogui") != 0) {
+            LOG(ERROR, "Username %s contains a colon, refusing", user);
+            daemon__exit(QREXEC_EXIT_REQUEST_REFUSED);
+        }
+    }
 
     /* Replace the target domain with the version normalized by the policy engine */
     target_domain = requested_target;
@@ -1189,6 +1232,11 @@
         } else {
             type = "name";
         }
+        /* ensure that commands are syntactically correct by construction */
+        if (!check_single_word(target_domain)) {
+            LOG(ERROR, "BUG: policy engine returned invalid target '%s'", target_domain);
+            daemon__exit(126);
+        }
         if (asprintf(&cmd, "QUBESRPC %s%s %s %s %s",
                      service_name,
                      trailer,
@@ -1622,10 +1670,16 @@
         fprintf(stderr, "Domain UUID option missing!\n");
         usage(argv[0]);
     }
-    remote_domain_id = atoi(argv[optind]);
+    remote_domain_id = parse_int(argv[optind], "remote domain ID");
     remote_domain_name = argv[optind+1];
+    /* this is inserted into the generated command line */
+    if (!check_single_word(remote_domain_name))
+        errx(1, "Invalid remote domain name %s", remote_domain_name);
     if (argc - optind >= 3)
         default_user = argv[optind+2];
+    /* qubes_parse_rpc_command() considers ':' to terminate the username */
+    if (strchr(default_user, ':') != NULL)
+        errx(1, "Invalid default username '%s' (colon not allowed)", default_user);
     init(remote_domain_id, opt_direct);
 
     sigemptyset(&selectmask);

qrexec/policy/parser.py

@@ -747,14 +747,24 @@ def from_ask_resolution(
 
     async def execute(self) -> str:
         """Return the allowed action"""
-        request, target = self.request, self.target
+        user, request, target = (
+            self.user or "DEFAULT",
+            self.request,
+            self.target,
+        )
+        requested_target = request.target
         assert target is not None
         assert isinstance(self.autostart, bool)
 
         # XXX remove when #951 gets fixed
         if request.source == target:
             raise AccessDenied("loopback qrexec connection not supported")
 
+        # XXX disallow this in parsing in R5.0
+        colon_index = user.find(":")
+        if colon_index != -1 and user[colon_index:] != ":nogui":
+            raise AccessDenied("colon in username not supported")
+
         if target in (
             "@adminvm",
             "dom0",
@@ -765,7 +775,7 @@ async def execute(self) -> str:
 result=allow
 target=@adminvm
 autostart={self.autostart}
-requested_target={request.target}"""
+requested_target={requested_target}"""
         if target.startswith("@dispvm:"):
             target_info = request.system_info["domains"][target[8:]]
             return f"""\
@@ -774,15 +784,15 @@ async def execute(self) -> str:
 target={self.target}
 target_uuid=@dispvm:uuid:{target_info['uuid']}
 autostart={self.autostart}
-requested_target={request.target}"""
+requested_target={requested_target}"""
         target_info = request.system_info["domains"][target]
         return f"""\
 user={self.user or 'DEFAULT'}
 result=allow
 target={self.target}
 target_uuid=uuid:{target_info['uuid']}
 autostart={self.autostart}
-requested_target={request.target}"""
+requested_target={requested_target}"""
 
 
 class AskResolution(AbstractResolution):

qrexec/tests/policy_parser.py

@@ -2017,6 +2017,76 @@ async def _test_123_execute_already_running(self):
 target_uuid=uuid:b3eb69d0-f9d9-4c3c-ad5c-454500303ea4
 autostart=True
 requested_target=test-vm2\
+""",
+        )
+
+    def test_124_execute_bad_username(self):
+        asyncio.run(self._test_124_execute_bad_username())
+
+    async def _test_124_execute_bad_username(self):
+        rule = parser.Rule.from_line(
+            None,
+            "* * @anyvm @anyvm allow user=:bogus",
+            filepath="filename",
+            lineno=12,
+        )
+        request = _req("test-vm1", "test-vm2")
+        resolution = parser.AllowResolution(
+            rule,
+            request,
+            user=":bogus",
+            target="test-vm2",
+            autostart=True,
+        )
+        with self.assertRaises(exc.AccessDenied) as e:
+            await resolution.execute()
+
+    def test_125_execute_loopback(self):
+        asyncio.run(self._test_125_execute_loopback())
+
+    async def _test_125_execute_loopback(self):
+        rule = parser.Rule.from_line(
+            None, "* * @anyvm @anyvm allow", filepath="filename", lineno=12
+        )
+        request = _req("test-vm1", "test-vm1")
+        resolution = parser.AllowResolution(
+            rule,
+            request,
+            user=None,
+            target="test-vm1",
+            autostart=True,
+        )
+        with self.assertRaises(exc.AccessDenied) as e:
+            await resolution.execute()
+
+    def test_126_execute_bad_but_permitted_username(self):
+        asyncio.run(self._test_126_execute_bad_but_permitted_username())
+
+    async def _test_126_execute_bad_but_permitted_username(self):
+        rule = parser.Rule.from_line(
+            None,
+            "* * @anyvm @anyvm allow user=a:nogui",
+            filepath="filename",
+            lineno=12,
+        )
+        request = _req("test-vm1", "test-vm2")
+        resolution = parser.AllowResolution(
+            rule,
+            request,
+            user="a:nogui",
+            target="test-vm2",
+            autostart=True,
+        )
+        result = await resolution.execute()
+        self.assertEqual(
+            result,
+            """\
+user=a:nogui
+result=allow
+target=test-vm2
+target_uuid=uuid:b3eb69d0-f9d9-4c3c-ad5c-454500303ea4
+autostart=True
+requested_target=test-vm2\
 """,
         )