(CVE-2024-52533) Buffer overflow in set_connect_msg()
We have a complaint from a SAST tool (probably Coverity):
"Error: OVERRUN (CWE-119):
glib-2.79.1/gio/gsocks4aproxy.c:220: assignment: Assigning: ""len"" = ""set_connect_msg(msg, hostname, port, username, error)"". The value of ""len"" is now between 11 and 520 (inclusive).
glib-2.79.1/gio/gsocks4aproxy.c:225: overrun-buffer-arg: Overrunning array ""msg"" of 519 bytes by passing it to a function which accesses it at byte offset 519 using argument ""len"" (which evaluates to 520). [Note: The source code implementation of the function has been overridden by a user model.]
# 223| goto error;
# 224|
# 225|-> if (!g_output_stream_write_all (out, msg, len, NULL,
# 226| cancellable, error))
# 227| goto error;"
Normally these are false positives, but in this case I think the tool is correct. set_connect_msg()
receives a buffer of size SOCKS4_CONN_MSG_LEN
but it writes up to SOCKS4_CONN_MSG_LEN + 1
bytes to it. This is because SOCKS4_CONN_MSG_LEN
doesn't account for the trailing nul character that set_connect_msg()
appends after the hostname.
Fix should be:
diff --git a/gio/gsocks4aproxy.c b/gio/gsocks4aproxy.c
index 3dad118eb..dcf8369d0 100644
--- a/gio/gsocks4aproxy.c
+++ b/gio/gsocks4aproxy.c
@@ -79,9 +79,9 @@ g_socks4a_proxy_init (GSocks4aProxy *proxy)
* +----+----+----+----+----+----+----+----+----+----+....+----+------+....+------+
* | VN | CD | DSTPORT | DSTIP | USERID |NULL| HOST | | NULL |
* +----+----+----+----+----+----+----+----+----+----+....+----+------+....+------+
- * 1 1 2 4 variable 1 variable
+ * 1 1 2 4 variable 1 variable 1
*/
-#define SOCKS4_CONN_MSG_LEN (9 + SOCKS4_MAX_LEN * 2)
+#define SOCKS4_CONN_MSG_LEN (10 + SOCKS4_MAX_LEN * 2)
static gint
set_connect_msg (guint8 *msg,
const gchar *hostname,
@@ -458,3 +458,4 @@ g_socks4a_proxy_iface_init (GProxyInterface *proxy_iface)
proxy_iface->connect_finish = g_socks4a_proxy_connect_finish;
proxy_iface->supports_hostname = g_socks4a_proxy_supports_hostname;
}
+
I'm a little concerned because it looks like it was omitted from the size intentionally, as we see a very explicit diagram where the size of the trailing nul is not counted. But we do write it to the buffer, so surely the buffer needs to be large enough to accommodate it.