From f05a59323ab20b8225fd749bc563a6925174dd8d Mon Sep 17 00:00:00 2001 From: monosans Date: Wed, 29 Jul 2026 15:20:45 +0300 Subject: [PATCH] fix(client): write a single NULL after USERID in SOCKS4 requests `Request::write_to_buf` emitted two NULL bytes after DSTIP, labelled `USERID` and `NULL`. Per SOCKS4 ("SOCKS: A protocol for TCP proxy across firewalls", Ying-Da Lee, https://www.openssh.com/txt/socks4.protocol), a CONNECT request is VN(1) CD(1) DSTPORT(2) DSTIP(4) USERID(variable) NULL(1) where USERID is a variable-length field terminated by a single NULL. This connector never sends a user ID, so USERID is zero bytes long and only its terminator belongs on the wire: 9 bytes, not 10. The extra byte breaks SOCKS4a ("SOCKS 4A: A Simple Extension to SOCKS 4 Protocol", https://www.openssh.com/txt/socks4a.protocol), which appends a NULL-terminated hostname after the USERID terminator. A server reads USERID up to the first NULL (correctly empty), then reads the hostname up to the next NULL -- the stray byte -- so the destination arrives as an empty string. The request fails, and the real hostname plus its terminator remain in the server's receive buffer, to be relayed into the tunnel as the first bytes of application data. Plain SOCKS4 requests still parse, since the destination is carried in DSTIP, but the trailing byte is likewise left over and reaches the target as a stray leading NULL of the tunneled stream. The bug dates back to the connectors' introduction in #187, and the domain path had no test coverage, so a SOCKS4a test is added alongside. Two stale comments on the touched lines are corrected as well: `put_u16(*port)` was labelled `IP`, and the layout diagram read "only do IP is 0.0.0.X". Co-Authored-By: Claude Opus 5 (1M context) --- .../legacy/connect/proxy/socks/v4/messages.rs | 18 +++---- tests/proxy.rs | 50 ++++++++++++++++++- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/client/legacy/connect/proxy/socks/v4/messages.rs b/src/client/legacy/connect/proxy/socks/v4/messages.rs index bec8d081..18b0d0ee 100644 --- a/src/client/legacy/connect/proxy/socks/v4/messages.rs +++ b/src/client/legacy/connect/proxy/socks/v4/messages.rs @@ -9,7 +9,7 @@ use std::net::SocketAddrV4; /// | 1 | 1 | 2 | 4 | Variable | 1 | Variable | 1 | /// +-----+-----+----+----+----+----+----+----+-------------+------+------------+------+ /// ^^^^^^^^^^^^^^^^^^^^^ -/// optional: only do IP is 0.0.0.X +/// optional: only if IP is 0.0.0.X #[derive(Debug)] pub struct Request<'a>(pub &'a Address); @@ -41,7 +41,7 @@ impl Request<'_> { pub fn write_to_buf(&self, mut buf: B) -> Result { match self.0 { Address::Socket(socket) => { - if buf.remaining_mut() < 10 { + if buf.remaining_mut() < 9 { return Err(SerializeError::WouldOverflow); } @@ -51,30 +51,28 @@ impl Request<'_> { buf.put_u16(socket.port()); // Port buf.put_slice(&socket.ip().octets()); // IP - buf.put_u8(0x00); // USERID - buf.put_u8(0x00); // NULL + buf.put_u8(0x00); // NULL terminating an empty USERID - Ok(10) + Ok(9) } Address::Domain(domain, port) => { - if buf.remaining_mut() < 10 + domain.len() + 1 { + if buf.remaining_mut() < 9 + domain.len() + 1 { return Err(SerializeError::WouldOverflow); } buf.put_u8(0x04); // Version buf.put_u8(0x01); // CONNECT - buf.put_u16(*port); // IP + buf.put_u16(*port); // Port buf.put_slice(&[0x00, 0x00, 0x00, 0xFF]); // Invalid IP - buf.put_u8(0x00); // USERID - buf.put_u8(0x00); // NULL + buf.put_u8(0x00); // NULL terminating an empty USERID buf.put_slice(domain.as_bytes()); // Domain buf.put_u8(0x00); // NULL - Ok(10 + domain.len() + 1) + Ok(9 + domain.len() + 1) } } } diff --git a/tests/proxy.rs b/tests/proxy.rs index 65e7e819..9ff48d4e 100644 --- a/tests/proxy.rs +++ b/tests/proxy.rs @@ -381,7 +381,7 @@ async fn test_socks_v4_works() { let [p1, p2] = target_addr.port().to_be_bytes(); let [ip1, ip2, ip3, ip4] = [127, 0, 0, 1]; - let message = [4, 0x01, p1, p2, ip1, ip2, ip3, ip4, 0, 0]; + let message = [4, 0x01, p1, p2, ip1, ip2, ip3, ip4, 0]; let n = to_client.read(&mut buf).await.expect("read"); assert_eq!(&buf[..n], message); @@ -418,6 +418,54 @@ async fn test_socks_v4_works() { t3.await.expect("task - target"); } +#[cfg(not(miri))] +#[tokio::test] +async fn test_socks_v4a_with_server_resolved_domain_works() { + let proxy_tcp = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let proxy_addr = proxy_tcp.local_addr().expect("local_addr"); + let proxy_dst = format!("http://{proxy_addr}").parse().expect("uri"); + + let mut connector = SocksV4::new(proxy_dst, HttpConnector::new()); + + // Client + // + // Will use `SocksV4` to establish a tunnel to a domain name, leaving + // resolution to the proxy server (the SOCKS4a extension). + let t1 = tokio::spawn(async move { + let _conn = connector + .call("https://hyper.rs:443".try_into().unwrap()) + .await + .expect("tunnel"); + }); + + // Proxy + // + // Will receive a SOCKS4a CONNECT command carrying the domain name, and + // reply with a success status. + let t2 = tokio::spawn(async move { + let (mut to_client, _) = proxy_tcp.accept().await.expect("accept"); + let mut buf = [0u8; 512]; + + let host = "hyper.rs"; + let [p1, p2] = 443u16.to_be_bytes(); + + // VN, CD, DSTPORT, DSTIP (0.0.0.x marks a SOCKS4a request), then the + // NULL terminating an empty USERID, then the NULL-terminated domain. + let mut message = vec![0x04, 0x01, p1, p2, 0x00, 0x00, 0x00, 0xFF, 0x00]; + message.extend(host.as_bytes()); + message.push(0x00); + + let n = to_client.read(&mut buf).await.expect("read"); + assert_eq!(&buf[..n], message); + + let message = [0, 90, p1, p2, 0, 0, 0, 0]; + to_client.write_all(&message).await.expect("write"); + }); + + t1.await.expect("task - client"); + t2.await.expect("task - proxy"); +} + #[cfg(not(miri))] #[tokio::test] async fn test_socks_v5_optimistic_works() {