Skip to content

fix(client): write a single NULL after USERID in SOCKS4 requests - #307

Open
monosans wants to merge 1 commit into
hyperium:masterfrom
monosans:patch-1
Open

fix(client): write a single NULL after USERID in SOCKS4 requests#307
monosans wants to merge 1 commit into
hyperium:masterfrom
monosans:patch-1

Conversation

@monosans

Copy link
Copy Markdown

What's wrong

The SOCKS4 connector writes one byte too many in its CONNECT request.

A SOCKS4 request is an 8-byte header followed by a user ID — a variable-length string ended by a single zero byte:

VN(1) CD(1) DSTPORT(2) DSTIP(4) USERID(variable) NULL(1)

SOCKS: A protocol for TCP proxy across firewalls, Ying-Da Lee

This connector never sends a user ID, so that field is empty and only its single terminating zero byte belongs on the wire — 9 bytes in total. Request::write_to_buf in src/client/legacy/connect/proxy/socks/v4/messages.rs wrote two zero bytes (one commented USERID, one commented NULL), so it sent 10.

Why it matters

The SOCKS4a extension — the one that lets the proxy resolve a domain name — puts the hostname immediately after that terminator, itself ended by a zero byte:

VN(1) CD(1) DSTPORT(2) DSTIP(4) USERID(variable) NULL(1) HOSTNAME(variable) NULL(1)

SOCKS 4A: A Simple Extension to SOCKS 4 Protocol, Ying-Da Lee

That is this connector's default mode: local_dns is off by default, so a domain destination is sent by name for the proxy to resolve. With an extra zero byte in front of it, the proxy reads the hostname field as an empty string, and the real hostname is never part of the request at all.

Feeding both versions of the bytes to a parser that follows the spec literally:

SOCKS4a BEFORE (19 bytes): port=443 ip=0.0.0.255 userid="" hostname=""         leftover="hyper.rs\0"
SOCKS4a AFTER  (18 bytes): port=443 ip=0.0.0.255 userid="" hostname="hyper.rs" leftover=""
SOCKS4  BEFORE (10 bytes): port=443 ip=127.0.0.1 userid="" (no hostname)       leftover="\0"
SOCKS4  AFTER  ( 9 bytes): port=443 ip=127.0.0.1 userid="" (no hostname)       leftover=""

In other words:

  • SOCKS4a (domain destination — the default) is broken. The proxy gets an empty hostname and the request fails. Worse, the leftover hyper.rs\0 stays in the proxy's receive buffer and is relayed into the tunnel as if the client had sent it as application data.
  • Plain SOCKS4 (IPv4 destination) still connects, because the address travels in DSTIP — but the leftover zero byte is still there and reaches the target as a stray first byte of the tunneled stream.

As an independent check, curl builds the same request with exactly one terminator: in lib/socks.c it appends strlen(user) + 1 bytes for the user ID (just the terminator when there is no user), then strlen(hostname) + 1 for SOCKS4a.

The change

  • Remove the extra zero byte, and adjust the two remaining_mut() checks and the returned lengths from 10 to 9.
  • Add test_socks_v4a_with_server_resolved_domain_works. The domain path had no test at all, which is how this went unnoticed since the connectors landed in feat(client): add proxy::SocksV4 and proxy::SocksV5 connectors #187 — the existing test_socks_v4_works only covers the IPv4 path, where the bug is much less visible.
  • Fix two comments on the lines being touched: buf.put_u16(*port) was labelled // IP, and the layout diagram read "only do IP is 0.0.0.X".

No public API change; the only behavioural difference is one fewer byte on the wire. cargo test --all-features --test proxy passes.

`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 hyperium#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) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant