connectd: bound the SOCKS5 request hostname to the request buffer
What the developer wrote
Authored by Níckolas Goline
connectd: bound the SOCKS5 request hostname to the request buffer
The SOCKS5 CONNECT request is built into a fixed 255-byte buffer, but a
domain-name request needs 7 + strlen(host) bytes and the host length was
never checked. A host longer than 248 bytes overran the buffer while the
request was assembled, corrupting the adjacent length field, which was then
used as the io_write() length and produced a large out-of-bounds read: the
proxy socket received connectd's heap instead of the request, and connectd
died. The host reaches this both from a proxied connect and from a gossiped
DNS address.
Size the buffer to hold the largest legal request (header plus a
maximum-length domain name), and refuse a hostname which still does not fit
rather than building a request we cannot represent. Truncating the host is
not an option: that is a connection to the wrong destination.
Changelog-Fixed: connectd: a proxied connection to a very long hostname (including a gossiped DNS address) could crash the node.
(cherry picked from commit 04ca809936794de47943951b9d116d9fd1ea1844)
What changed, and why it matters
This update fixes a bug in Core Lightning's Tor/proxy connection code. When the node tried to connect through a SOCKS5 proxy to a hostname longer than 248 characters, it wrote past the end of a small fixed buffer. That corrupted the recorded message length, causing the proxy to be sent random data from the program's memory and crashing the node. The fix enlarges the buffer to the legal maximum and rejects hostnames that are still too long instead of overflowing.
Apply the patch and ensure the regression test test_connect_proxy_maxlen_hostname passes. Nodes using Tor/proxy outbound connections should upgrade, since a malicious or malformed long hostname can crash connectd.
Security signals we found
Stack buffer overflow / out-of-bounds write in SOCKS5 request construction
Out-of-bounds read caused by corrupted length used in io_write
Denial of service: connectd crash on long proxied hostname
Input from gossiped DNS address can reach the vulnerable code path
Changelog-Fixed explicitly describes the crash scenario
Evidence from the diff
connectd/tor.c built a SOCKS5 CONNECT request into a 255-byte stack buffer. A domain-name request needs SOCK_REQ_V5_HEADER_LEN (7) + strlen(host) bytes, but host length was unvalidated. A host > 248 bytes overflowed the buffer, clobbering connect->hlen. That corrupted length was then passed to io_write, producing a large out-of-bounds read of heap memory and a crash. The patch redefines MAX_SIZE_OF_SOCKS5_REQ_OR_RESP to 7 + 255, adds an explicit hlen > 255 check that closes the connection with ECONNREFUSED, and removes an xfail from the existing test_connect_proxy_maxlen_hostname test.
Changed components
connectd/tor.cSOCKS5 proxy CONNECT request handlingconnectd daemonInspect captured patch +24 / −3
### connectd/tor.c
@@ -17,7 +17,6 @@
#define SOCKS_TYP_IPV6 4
#define SOCKS_V5 5
-#define MAX_SIZE_OF_SOCKS5_REQ_OR_RESP 255
#define SIZE_OF_RESPONSE 4
#define SIZE_OF_REQUEST 3
#define SIZE_OF_IPV4_RESPONSE 6
@@ -26,6 +25,14 @@
#define SOCK_REQ_V5_LEN 5
#define SOCK_REQ_V5_HEADER_LEN 7
+/* The domain name in a SOCKS5 request is preceded by a single length
+ * byte, so it can never be longer than this. */
+#define MAX_SIZE_OF_SOCKS5_DOMAIN 255
+/* The largest thing we ever put in the buffer is a domain-name CONNECT
+ * request: the header plus a maximum-length domain name. */
+#define MAX_SIZE_OF_SOCKS5_REQ_OR_RESP (SOCK_REQ_V5_HEADER_LEN \
+ + MAX_SIZE_OF_SOCKS5_DOMAIN)
+
/* some crufts can not forward ipv6 */
#undef BIND_FIRST_TO_IPV6
@@ -154,14 +161,29 @@ static struct io_plan *io_tor_connect_after_resp_to_connect(struct io_conn
if (connect->buffer[1] == '\0') {
/* make the V5 request */
connect->hlen = strlen(connect->host);
+
+ /* The length is carried in a single byte, and the whole
+ * request has to fit in our buffer: refuse rather than
+ * build a request we can't represent. */
+ if (connect->hlen > MAX_SIZE_OF_SOCKS5_DOMAIN) {
+ const char *msg = tal_fmt(tmpctx,
+ "Connected out for %s error: hostname too long for socks5 request",
+ connect->host);
+ status_debug("%s", msg);
+ add_errors_to_error_list(connect->connect, msg);
+
+ errno = ECONNREFUSED;
+ return io_close(conn);
+ }
+
connect->buffer[0] = SOCKS_V5;
connect->buffer[1] = SOCKS_CONNECT;
connect->buffer[2] = 0;
connect->buffer[3] = SOCKS_DOMAIN;
connect->buffer[4] = connect->hlen;
memcpy(connect->buffer + SOCK_REQ_V5_LEN, connect->host, connect->hlen);
- memcpy(connect->buffer + SOCK_REQ_V5_LEN + strlen(connect->host),
+ memcpy(connect->buffer + SOCK_REQ_V5_LEN + connect->hlen,
&(connect->port), sizeof connect->port);
status_io(LOG_IO_OUT, NULL, "proxy", connect->buffer,
### tests/test_connection.py
@@ -5140,7 +5140,6 @@ def test_open_channel_funding_above_max_supply(node_factory, bitcoind):
assert l1.rpc.getinfo()['id'] == l1.info['id']
-@pytest.mark.xfail(strict=True)
def test_connect_proxy_maxlen_hostname(node_factory):
"""A maximum-length hostname must produce a well-formed SOCKS5 request.
Why this scored 73/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.