diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c index fb20aaf5..6ada95ef 100644 --- a/daemon/qrexec-client.c +++ b/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) { diff --git a/daemon/qrexec-daemon-common.c b/daemon/qrexec-daemon-common.c index b465a54d..444ce9ef 100644 --- a/daemon/qrexec-daemon-common.c +++ b/daemon/qrexec-daemon-common.c @@ -5,6 +5,8 @@ #include #include #include +#include +#include #include "qrexec.h" #include "libqrexec-utils.h" @@ -14,6 +16,23 @@ const char *socket_dir = QREXEC_DAEMON_SOCKET_DIR; #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, diff --git a/daemon/qrexec-daemon-common.h b/daemon/qrexec-daemon-common.h index 02b36397..4a64b1ce 100644 --- a/daemon/qrexec-daemon-common.h +++ b/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); diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c index 3d20849e..5dc66af8 100644 --- a/daemon/qrexec-daemon.c +++ b/daemon/qrexec-daemon.c @@ -296,7 +296,7 @@ static void init(int xid, bool opt_direct) } 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 @@ static _Noreturn void do_exec(const char *prog, const char *username __attribute _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 @@ _Noreturn static void handle_execute_service_child( 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 @@ _Noreturn static void handle_execute_service_child( } 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 @@ int main(int argc, char **argv) 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); diff --git a/qrexec/policy/parser.py b/qrexec/policy/parser.py index cfcdf5d5..3bb9dc18 100644 --- a/qrexec/policy/parser.py +++ b/qrexec/policy/parser.py @@ -747,7 +747,12 @@ 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) @@ -755,6 +760,11 @@ async def execute(self) -> str: 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,7 +784,7 @@ 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'} @@ -782,7 +792,7 @@ async def execute(self) -> str: target={self.target} target_uuid=uuid:{target_info['uuid']} autostart={self.autostart} -requested_target={request.target}""" +requested_target={requested_target}""" class AskResolution(AbstractResolution): diff --git a/qrexec/tests/policy_parser.py b/qrexec/tests/policy_parser.py index 44eb72bf..92b03ed4 100644 --- a/qrexec/tests/policy_parser.py +++ b/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\ """, )