Merge remote-tracking branch 'origin/pr/138'

* origin/pr/138:
  Support not passing metadata to socket-based services
  Cleanly terminate connections if command or config is invalid
  Test service configuration better
  Clean up configuration loading
  Move TOML parsing function to private header
  tests: Allow running tests under ASAN+UBSAN
  tests: Allow altering arguments to test script
  tests: treat ECONNRESET as EOF
  tests: don't use sleep(1) to enforce message ordering
  tests: prevent unexpected message combining
  tests: tolerate alternate orders of messages

Author: marmarek

agent/qrexec-agent-data.c

@@ -134,10 +134,12 @@ static int handle_just_exec(struct qrexec_parsed_command *cmd)
 {
     int fdn, pid;
 
+    if (cmd == NULL)
+        return 127;
     switch (pid = fork()) {
         case -1:
             PERROR("fork");
-            return -1;
+            return 127;
         case 0:
             fdn = open("/dev/null", O_RDWR);
             fix_fds(fdn, fdn, fdn);
@@ -271,7 +273,8 @@ static int handle_new_process_common(
             return 0;
         case MSG_EXEC_CMDLINE:
             buffer_init(&stdin_buf);
-            if (execute_parsed_qubes_rpc_command(cmd, &pid, &stdin_fd, &stdout_fd, &stderr_fd, &stdin_buf) < 0) {
+            if (cmd == NULL ||
+                    execute_parsed_qubes_rpc_command(cmd, &pid, &stdin_fd, &stdout_fd, &stderr_fd, &stdin_buf) < 0) {
                 struct msg_header hdr = {
                     .type = MSG_DATA_STDOUT,
                     .len = 0,

agent/qrexec-agent.c

@@ -85,6 +85,7 @@ static const char *fork_server_path = QREXEC_FORK_SERVER_SOCKET;
 static void handle_server_exec_request_do(int type, int connect_domain, int connect_port,
                                           struct qrexec_parsed_command *cmd,
                                           char *cmdline);
+static void terminate_connection(uint32_t domain, uint32_t port);
 
 const bool qrexec_is_fork_server = false;
 
@@ -587,26 +588,20 @@ static void handle_server_exec_request_init(struct msg_header *hdr)
         cmd = parse_qubes_rpc_command(buf, true);
         if (cmd == NULL) {
             LOG(ERROR, "Could not parse command line: %s", buf);
-            return;
+            goto doit;
         }
 
         /* 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;
+                destroy_qrexec_parsed_command(cmd);
+                cmd = NULL;
+                goto doit;
             }
 
             /* "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++)
@@ -628,6 +623,7 @@ static void handle_server_exec_request_init(struct msg_header *hdr)
         }
     }
 
+doit:
     handle_server_exec_request_do(hdr->type, params.connect_domain, params.connect_port, cmd, buf);
     destroy_qrexec_parsed_command(cmd);
     free(buf);
@@ -671,7 +667,7 @@ static void handle_server_exec_request_do(int type,
         return;
     }
 
-    if (!cmd->nogui) {
+    if (cmd != NULL && !cmd->nogui) {
         /* try fork server */
         int child_socket = try_fork_server(type,
                 params.connect_domain, params.connect_port,
@@ -765,20 +761,29 @@ static int find_connection(int pid)
 }
 
 static void release_connection(int id) {
-    struct msg_header hdr;
-    struct exec_params params;
-
-    hdr.type = MSG_CONNECTION_TERMINATED;
-    hdr.len = sizeof(struct exec_params);
-    params.connect_domain = connection_info[id].connect_domain;
-    params.connect_port = connection_info[id].connect_port;
-    if (libvchan_send(ctrl_vchan, &hdr, sizeof(hdr)) != sizeof(hdr))
-        handle_vchan_error("send (MSG_CONNECTION_TERMINATED hdr)");
-    if (libvchan_send(ctrl_vchan, &params, sizeof(params)) != sizeof(params))
-        handle_vchan_error("send (MSG_CONNECTION_TERMINATED data)");
+    terminate_connection(connection_info[id].connect_domain,
+                         connection_info[id].connect_port);
     connection_info[id].pid = 0;
 }
 
+static void terminate_connection(uint32_t domain, uint32_t port) {
+    struct {
+        struct msg_header hdr;
+        struct exec_params params;
+    } data = {
+        .hdr = {
+            .type = MSG_CONNECTION_TERMINATED,
+            .len = sizeof(struct exec_params),
+        },
+        .params = {
+            .connect_domain = domain,
+            .connect_port = port,
+        },
+    };
+    if (libvchan_send(ctrl_vchan, &data, sizeof(data)) != sizeof(data))
+        handle_vchan_error("send (MSG_CONNECTION_TERMINATED)");
+}
+
 static void reap_children(void)
 {
     int status;

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/Makefile

@@ -1,10 +1,11 @@
-CC=gcc
+CC ?=gcc
 VCHAN_PKG = $(if $(BACKEND_VMM),vchan-$(BACKEND_VMM),vchan)
 override QUBES_CFLAGS := -I. -I../libqrexec -g -O2 -Wall -Wextra -Werror \
    $(shell pkg-config --cflags $(VCHAN_PKG)) -fstack-protector \
    -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fPIC -std=gnu11 -D_POSIX_C_SOURCE=200809L \
    -D_GNU_SOURCE $(CFLAGS) \
-   -Wstrict-prototypes -Wold-style-definition -Wmissing-declarations
+   -Wstrict-prototypes -Wold-style-definition -Wmissing-declarations \
+   -fno-delete-null-pointer-checks
 
 override LDFLAGS += -pie -Wl,-z,relro,-z,now -shared
 

libqrexec/buffer.c

@@ -23,6 +23,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 #include "libqrexec-utils.h"
 
 #define BUFFER_LIMIT 50000000
@@ -79,6 +80,7 @@ void buffer_append(struct buffer *b, const char *data, int len)
 {
     int newsize;
     char *qdata;
+    assert(data != NULL && "NULL data");
     if (b->buflen < 0 || b->buflen > BUFFER_LIMIT) {
         LOG(ERROR, "buffer_append buflen %d", len);
         exit(1);
@@ -91,7 +93,9 @@ void buffer_append(struct buffer *b, const char *data, int len)
         return;
     newsize = len + b->buflen;
     qdata = limited_malloc(len + b->buflen);
-    memcpy(qdata, b->data, (size_t)b->buflen);
+    if (b->data != 0) {
+        memcpy(qdata, b->data, (size_t)b->buflen);
+    }
     memcpy(qdata + b->buflen, data, (size_t)len);
     buffer_free(b);
     b->buflen = newsize;

libqrexec/exec.c

@@ -36,6 +36,7 @@
 #include <fcntl.h>
 #include "qrexec.h"
 #include "libqrexec-utils.h"
+#include "private.h"
 
 static do_exec_t *exec_func = NULL;
 void register_exec_func(do_exec_t *func) {
@@ -251,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;
@@ -269,10 +269,31 @@ 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,
+                                   &cmd->send_service_descriptor);
+}
 
-    return qubes_toml_config_parse(config_full_path, 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;
 }
 
+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) {
 
@@ -284,6 +305,7 @@ struct qrexec_parsed_command *parse_qubes_rpc_command(
     }
 
     memset(cmd, 0, sizeof(*cmd));
+    cmd->send_service_descriptor = true;
     cmd->cmdline = cmdline;
 
     if (strip_username) {
@@ -469,9 +491,11 @@ static int execute_qrexec_service(
         *pid = 0;
         set_nonblock(s);
 
-        /* send part after "QUBESRPC ", including trailing NUL */
-        const char *desc = cmd->command + RPC_REQUEST_COMMAND_LEN + 1;
-        buffer_append(stdin_buffer, desc, strlen(desc) + 1);
+        if (cmd->send_service_descriptor) {
+            /* send part after "QUBESRPC ", including trailing NUL */
+            const char *desc = cmd->command + RPC_REQUEST_COMMAND_LEN + 1;
+            buffer_append(stdin_buffer, desc, strlen(desc) + 1);
+        }
         return 0;
     }
 

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,12 @@ struct qrexec_parsed_command {
 
     /* Source domain (the part after service name). */
     char *source_domain;
+
+    /* Should a session be waited for? */
+    bool wait_for_session;
+
+    /* For socket-based services: Should the service descriptor be sent? */
+    bool send_service_descriptor;
 };
 
 /* Parse a command, return NULL on failure. Uses cmd->cmdline
@@ -89,9 +96,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);
@@ -303,7 +314,6 @@ void qrexec_log(int level, int errnoval, const char *file, int line,
                 const char *func, const char *fmt, ...) __attribute__((format(printf, 6, 7)));
 
 void setup_logging(const char *program_name);
-int qubes_toml_config_parse(const char *config_full_path, int *wait_for_session, char **user);
 
 /**
  * Make an Admin API call to qubesd.  The returned buffer must be released by

libqrexec/private.h

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