Clean up configuration loading

This technically breaks the libqrexec ABI by increasing the size of
struct qrexec_parsed_command.  However, nothing outside of libqrexec
actually allocates instances of this struct, so adding new members at
the end is harmless.  It also changes a function that took a const
pointer to instead take a non-const pointer, but this disappears at the
ABI level.

It also fixes some memory leaks.  These memory leaks are small enough
that they were likely not observed in practice, especially since they
require invalid configuration files to trigger.

Author: DemiMarie

agent/qrexec-agent.c

@@ -592,21 +592,13 @@ static void handle_server_exec_request_init(struct msg_header *hdr)
 
         /* load service config only for service requests */
         if (cmd->service_descriptor) {
-            int wait_for_session = 0;
-            char *user = NULL;
-
-            if (load_service_config(cmd, &wait_for_session, &user) < 0) {
+            if (load_service_config_v2(cmd) < 0) {
                 LOG(ERROR, "Could not load config for command %s", buf);
                 return;
             }
 
-            if (user != NULL) {
-                free(cmd->username);
-                cmd->username = user;
-            }
-
             /* "nogui:" prefix has priority */
-            if (!cmd->nogui && wait_for_session && wait_for_session_maybe(cmd)) {
+            if (!cmd->nogui && cmd->wait_for_session && wait_for_session_maybe(cmd)) {
                 /* waiting for session, postpone actual call */
                 int slot_index;
                 for (slot_index = 0; slot_index < MAX_FDS; slot_index++)

daemon/qrexec-client.c

@@ -213,7 +213,6 @@ static _Noreturn void do_exec(const char *prog, const char *username __attribute
 /* See also qrexec-agent.c:wait_for_session_maybe() */
 static void wait_for_session_maybe(char *cmdline)
 {
-    int wait_for_session = 0;
     struct qrexec_parsed_command *cmd;
     pid_t pid;
     int status;
@@ -228,9 +227,8 @@ static void wait_for_session_maybe(char *cmdline)
     if (!cmd->service_descriptor)
         goto out;
 
-    char *user = NULL;
-    load_service_config(cmd, &wait_for_session, &user);
-    if (!wait_for_session)
+    load_service_config_v2(cmd);
+    if (!cmd->wait_for_session)
         goto out;
 
     pid = fork();

libqrexec/exec.c

@@ -252,11 +252,10 @@ static int find_file(
     }
     return -1;
 }
-int load_service_config(const struct qrexec_parsed_command *cmd,
-                        int *wait_for_session, char **user) {
-    assert(cmd->service_descriptor);
-    assert(*user == NULL);
 
+static int load_service_config_raw(struct qrexec_parsed_command *cmd,
+                                   char **user)
+{
     const char *config_path = getenv("QUBES_RPC_CONFIG_PATH");
     if (!config_path)
         config_path = QUBES_RPC_CONFIG_PATH;
@@ -270,10 +269,30 @@ int load_service_config(const struct qrexec_parsed_command *cmd,
                         config_full_path, sizeof(config_full_path), NULL);
     if (ret < 0)
         return 0;
+    return qubes_toml_config_parse(config_full_path, &cmd->wait_for_session, user);
+}
+
+int load_service_config_v2(struct qrexec_parsed_command *cmd) {
+    assert(cmd->service_descriptor);
+    char *tmp_user = NULL;
+    int res = load_service_config_raw(cmd, &tmp_user);
+    if (res >= 0 && tmp_user != NULL) {
+        free(cmd->username);
+        cmd->username = tmp_user;
+    }
+    return res;
+}
 
-    return qubes_toml_config_parse(config_full_path, wait_for_session, user);
+int load_service_config(struct qrexec_parsed_command *cmd,
+                        int *wait_for_session, char **user) {
+    int rc = load_service_config_raw(cmd, user);
+    if (rc >= 0) {
+        *wait_for_session = cmd->wait_for_session;
+    }
+    return rc;
 }
 
+
 struct qrexec_parsed_command *parse_qubes_rpc_command(
     const char *cmdline, bool strip_username) {
 

libqrexec/libqrexec-utils.h

@@ -52,7 +52,8 @@ struct buffer {
 struct qrexec_parsed_command {
     const char *cmdline;
 
-    /* Username ("user", NULL when strip_username is false) */
+    /* Username ("user", NULL when strip_username is false).
+     * Always safe to pass to free(). */
     char *username;
 
     /* Override to disable "wait for session" */
@@ -75,6 +76,9 @@ struct qrexec_parsed_command {
 
     /* Source domain (the part after service name). */
     char *source_domain;
+
+    /* Should a session be waited for? */
+    bool wait_for_session;
 };
 
 /* Parse a command, return NULL on failure. Uses cmd->cmdline
@@ -89,9 +93,13 @@ void destroy_qrexec_parsed_command(struct qrexec_parsed_command *cmd);
  *  1  - config successfuly loaded
  *  0  - config not found
  *  -1 - other error
+ *
+ * Deprecated, use load_service_config_v2() instead.
  */
-int load_service_config(const struct qrexec_parsed_command *cmd_name,
-                        int *wait_for_session, char **user);
+int load_service_config(struct qrexec_parsed_command *cmd_name,
+                        int *wait_for_session, char **user)
+    __attribute__((deprecated("use load_service_config_v2() instead")));
+int load_service_config_v2(struct qrexec_parsed_command *cmd);
 
 typedef void (do_exec_t)(const char *cmdline, const char *user);
 void register_exec_func(do_exec_t *func);

libqrexec/private.h

@@ -1,2 +1,2 @@
 #include <stdbool.h>
-int qubes_toml_config_parse(const char *config_full_path, int *wait_for_session, char **user);
+int qubes_toml_config_parse(const char *config_full_path, bool *wait_for_session, char **user);

libqrexec/toml.c

@@ -132,7 +132,46 @@ static bool qubes_is_key_byte(unsigned char c) {
     }
 }
 
-int qubes_toml_config_parse(const char *config_full_path, int *wait_for_session, char **user)
+static void toml_invalid_type(enum toml_type ty, const char *file, size_t lineno, const char *msg)
+{
+    const char *bad_type;
+    switch (ty) {
+        case TOML_TYPE_INVALID:
+            bad_type = "<invalid value>";
+            break;
+        case TOML_TYPE_BOOL:
+            bad_type = "Boolean";
+            break;
+        case TOML_TYPE_INTEGER:
+            bad_type = "Integer";
+            break;
+        case TOML_TYPE_STRING:
+            bad_type = "String";
+            break;
+        default:
+            abort();
+    }
+    LOG(ERROR, "%s:%zu: %s not valid for %s", file, lineno, bad_type, msg);
+}
+
+static bool toml_check_dup_key(bool *seen_already, const char *file, size_t lineno, const char *msg)
+{
+    if (*seen_already) {
+        LOG(ERROR, "%s:%zu: Key %s already seen", file, lineno, msg);
+        return true;
+    }
+    *seen_already = true;
+    return false;
+}
+
+static void toml_value_free(union toml_data *value, enum toml_type ty) {
+    if (ty == TOML_TYPE_STRING) {
+        free(value->string);
+        value->string = NULL;
+    }
+}
+
+int qubes_toml_config_parse(const char *config_full_path, bool *wait_for_session, char **user)
 {
     int result = -1; /* assume problem */
     FILE *config_file = fopen(config_full_path, "re");
@@ -147,6 +186,21 @@ int qubes_toml_config_parse(const char *config_full_path, int *wait_for_session,
     ssize_t signed_linelen;
     bool seen_wait_for_session = false;
     bool seen_user = false;
+    *wait_for_session = 0;
+#define CHECK_DUP_KEY(v) do {                                               \
+    if (toml_check_dup_key(&(v), config_full_path, lineno, current_line)) { \
+        toml_value_free(&value, ty);                                        \
+        goto bad;                                                           \
+    }                                                                       \
+} while (0);
+#define CHECK_TYPE(v, msg) do {                                             \
+    if ((v) != ty) {                                                        \
+        toml_invalid_type(v, config_full_path, lineno, msg);                \
+        toml_value_free(&value, ty);                                        \
+        goto bad;                                                           \
+    }                                                                       \
+} while (0);
+
     while ((signed_linelen = getline(&current_line, &bufsize, config_file)) != -1) {
         lineno++;
         /* Other negative values are invalid.  If nothing at all is read that means EOF. */
@@ -218,55 +272,30 @@ int qubes_toml_config_parse(const char *config_full_path, int *wait_for_session,
         current_line[key_len] = '\0';
         union toml_data value;
         enum toml_type ty = parse_toml_value(config_full_path, lineno, key_cursor, &value);
+        if (ty == TOML_TYPE_INVALID)
+            goto bad;
 
         if (strcmp(current_line, "wait-for-session") == 0) {
-            if (seen_wait_for_session) {
-                LOG(ERROR, "%s:%zu: Key '%s' appears more than once", config_full_path, lineno, current_line);
-                goto bad;
-            }
-            seen_wait_for_session = true;
-            if (ty == TOML_TYPE_BOOL) {
-                *wait_for_session = (int)value.boolean;
-                continue;
-            } else if (ty == TOML_TYPE_INTEGER) {
+            CHECK_DUP_KEY(seen_wait_for_session);
+            if (ty == TOML_TYPE_INTEGER) {
                 if (value.integer < 2) {
                     *wait_for_session = (int)value.integer;
                     continue;
                 }
-                LOG(ERROR, "Integer value %llu used when a boolean was expected", value.integer);
-            } else if (ty == TOML_TYPE_STRING) {
-                LOG(ERROR, "String value '%s' not valid for 'wait-for-session'", value.string);
-                free(value.string);
-                value.string = NULL;
-            }
-        } else if (strcmp(current_line, "force-user") == 0) {
-            if (seen_user) {
-                LOG(ERROR, "%s:%zu: Key '%s' appears more than once", config_full_path, lineno, current_line);
-                goto bad;
-            }
-            seen_user = true;
-            char *bad_type;
-            switch (ty) {
-            case TOML_TYPE_INVALID:
+                LOG(ERROR, "%s:%zu: Unsupported integer value %llu for boolean", config_full_path, lineno, value.integer);
                 goto bad;
-            case TOML_TYPE_BOOL:
-                bad_type = "Boolean";
-                break;
-            case TOML_TYPE_INTEGER:
-                bad_type = "Integer";
-                break;
-            case TOML_TYPE_STRING:
-                *user = value.string;
-                continue;
-            default:
-                abort();
+            } else {
+                CHECK_TYPE(TOML_TYPE_BOOL, "wait-for-session");
+                *wait_for_session = value.boolean;
             }
-            LOG(ERROR, "%s:%zu: %s not valid for user name or user ID", config_full_path, lineno, bad_type);
+        } else if (strcmp(current_line, "force-user") == 0) {
+            CHECK_DUP_KEY(seen_user);
+            CHECK_TYPE(TOML_TYPE_STRING, "user name or user ID");
+            *user = value.string;
         } else {
             LOG(ERROR, "%s:%zu: Unsupported key %s", config_full_path, lineno, current_line);
-            continue;
+            toml_value_free(&value, ty);
         }
-        goto bad;
     }
 
     result = 1;