From 4f278aab2411eeb90fd4ea4349fab8a928949ac2 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Fri, 9 Feb 2024 20:07:26 -0500
Subject: [PATCH] Check return value of snprintf() and unlink()

No change in behavior if nothing goes wrong.
---
 agent/qrexec-agent.c     |  4 +++-
 agent/qrexec-client-vm.c |  3 +--
 daemon/qrexec-client.c   | 11 ++++++++---
 daemon/qrexec-daemon.c   | 31 +++++++++++++++++++++----------
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c
index 7195f1ac..4e10f3ef 100644
--- a/agent/qrexec-agent.c
+++ b/agent/qrexec-agent.c
@@ -846,7 +846,9 @@ static void handle_trigger_io(void)
     if (command[command_len-1] != '\0')
         goto error;
 
-    snprintf(params.request_id.ident, sizeof(params.request_id), "SOCKET%d", client_fd);
+    int res = snprintf(params.request_id.ident, sizeof(params.request_id), "SOCKET%d", client_fd);
+    if (res < 0 || res >= (int)sizeof(params.request_id))
+        abort();
     if (libvchan_send(ctrl_vchan, &hdr, sizeof(hdr)) != sizeof(hdr))
         handle_vchan_error("write hdr");
     if (libvchan_send(ctrl_vchan, &params, sizeof(params)) != sizeof(params))
diff --git a/agent/qrexec-client-vm.c b/agent/qrexec-client-vm.c
index 97ca9884..a646cd3f 100644
--- a/agent/qrexec-client-vm.c
+++ b/agent/qrexec-client-vm.c
@@ -232,8 +232,7 @@ int main(int argc, char **argv)
     strncpy(params.target_domain, argv[optind],
             sizeof(params.target_domain) - 1);
 
-    snprintf(params.request_id.ident,
-            sizeof(params.request_id.ident), "SOCKET");
+    memcpy(params.request_id.ident, "SOCKET", sizeof("SOCKET"));
 
     if (!write_all(trigger_fd, &hdr, sizeof(hdr))) {
         PERROR("write(hdr) to agent");
diff --git a/daemon/qrexec-client.c b/daemon/qrexec-client.c
index cc9ba8a9..d878f0c8 100644
--- a/daemon/qrexec-client.c
+++ b/daemon/qrexec-client.c
@@ -171,7 +171,7 @@ static int handle_daemon_handshake(int fd)
 
 static int connect_unix_socket(const char *domname)
 {
-    int s, len;
+    int s, len, res;
     struct sockaddr_un remote;
 
     if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
@@ -180,8 +180,13 @@ static int connect_unix_socket(const char *domname)
     }
 
     remote.sun_family = AF_UNIX;
-    snprintf(remote.sun_path, sizeof remote.sun_path,
-             "%s/qrexec.%s", socket_dir, domname);
+    res = snprintf(remote.sun_path, sizeof remote.sun_path,
+                   "%s/qrexec.%s", socket_dir, domname);
+    if (res < 0)
+        err(1, "snprintf");
+    if (res >= (int)sizeof(remote.sun_path))
+        errx(1, "%s/qrexec.%s is too long for AF_UNIX socket path",
+             socket_dir, domname);
     len = strlen(remote.sun_path) + sizeof(remote.sun_family);
     if (connect(s, (struct sockaddr *) &remote, len) == -1) {
         PERROR("connect");
diff --git a/daemon/qrexec-daemon.c b/daemon/qrexec-daemon.c
index 6da80767..5de5dc32 100644
--- a/daemon/qrexec-daemon.c
+++ b/daemon/qrexec-daemon.c
@@ -163,8 +163,12 @@ static void unlink_qrexec_socket(void)
                  "%s/qrexec.%s", socket_dir, remote_domain_name);
     if (v < (int)sizeof("/qrexec.") || v >= (int)sizeof(link_to_socket_name))
         abort();
-    unlink(socket_address);
-    unlink(link_to_socket_name);
+    v = unlink(socket_address);
+    if (v != 0 && !(v == -1 && errno == ENOENT))
+        err(1, "unlink(%s)", socket_address);
+    v = unlink(link_to_socket_name);
+    if (v != 0 && !(v == -1 && errno == ENOENT))
+        err(1, "unlink(%s)", link_to_socket_name);
 }
 
 static void handle_vchan_error(const char *op)
@@ -178,12 +182,17 @@ static int create_qrexec_socket(int domid, const char *domname)
 {
     char socket_address[40];
     char link_to_socket_name[strlen(domname) + sizeof(socket_address)];
-
-    snprintf(socket_address, sizeof(socket_address),
-             "%s/qrexec.%d", socket_dir, domid);
-    snprintf(link_to_socket_name, sizeof link_to_socket_name,
-             "%s/qrexec.%s", socket_dir, domname);
-    unlink(link_to_socket_name);
+    int res;
+
+    if ((unsigned)snprintf(socket_address, sizeof(socket_address),
+                           "%s/qrexec.%d", socket_dir, domid) >= sizeof(socket_address))
+        errx(1, "socket name too long");
+    if ((unsigned)snprintf(link_to_socket_name, sizeof link_to_socket_name,
+                           "%s/qrexec.%s", socket_dir, domname) >= sizeof link_to_socket_name)
+        errx(1, "socket link name too long");
+    res = unlink(link_to_socket_name);
+    if (res != 0 && !(res == -1 && errno == ENOENT))
+        err(1, "unlink(%s)", link_to_socket_name);
 
     /* When running as root, make the socket accessible; perms on /var/run/qubes still apply */
     umask(0);
@@ -330,8 +339,10 @@ static void init(int xid)
     close(0);
 
     if (!opt_direct) {
-        snprintf(qrexec_error_log_name, sizeof(qrexec_error_log_name),
-                 "/var/log/qubes/qrexec.%s.log", remote_domain_name);
+        if ((unsigned)snprintf(qrexec_error_log_name, sizeof(qrexec_error_log_name),
+                               "/var/log/qubes/qrexec.%s.log", remote_domain_name) >=
+                sizeof(qrexec_error_log_name))
+            errx(1, "remote domain name too long");
         umask(0007);        // make the log readable by the "qubes" group
         logfd =
             open(qrexec_error_log_name, O_WRONLY | O_CREAT | O_TRUNC,