SOCKS5 Fix - #486
Conversation
9b03594 to
84e6786
Compare
|
Hi @erebe — following up on #485 and this PR. Rebased onto current main, tests pass. Are you interested in this fix/PR? If not no-worries I'll go ahead and close #486 and #485. Let me know! The bug still reproduces on v10.6.2. The CONNECT path in socks5/tcp_server.rs calls reply_success with a hardcoded 127.0.0.1:0 before any upstream connection is attempted, so the client is told it succeeded whether or not the target is reachable. This is distinct from #515, which bounds the handshake but doesn't change when the reply is sent. One design note: #515 moved this path to Socks5ServerProtocol, which owns the socket and only hands it back from reply_success — so the reply is now withheld through a small adapter instead of taking the socket early. Happy to do it another way if you'd prefer. If the objection is scope rather than the bug, the first two commits are the actual fix; the timeout work can be dropped. |
|
Hello, Thank you for the PR. I am interested in it and fixing those issues. I will try to look at it during summer 🙈 I don't plan further changes in wstunnel (beside webtransport addition that I just added), so it is safe on your side, you will not get more merge conflict I guess |
…SOCKS5 handshake, and lower default to 1s
WebTransport landed after this branch was opened, adding a third transport to the paths this PR changes. Two gaps came with it. The reverse SOCKS5 connect handshake is a single byte the client writes into the tunnel and the server reads before replying to the SOCKS client. Only the websocket and http2 server handlers implemented that read, while the client wrote the byte on any transport. Over wts:// it was therefore never consumed as a handshake and arrived as the first byte of the payload, corrupting the stream. Separately, the forward path replies to the SOCKS client with a failure when connecting to the server fails, but the webtransport arm returned early instead, leaving the SOCKS client waiting until the socket closed. Add read_handshake_byte() to WebTransportTunnelRead and gate the deferred reply in handler_webtransport, mirroring the other two handlers. A QUIC stream is a byte stream, so taking one byte leaves the rest queued and no prefetch buffer is needed. Give the webtransport arm of the forward path the same failure reply as the others. Reverse and forward SOCKS5 now behave identically across all three transports, with integration coverage for connect success and failure over webtransport.
Summary
This PR fixes SOCKS5
CONNECTreply timing for both forward and reverse tunnel paths. Reported as an issue in #485.Before this change, SOCKS5 success could be returned before the upstream target connect result was known. In practice, proxy tooling (
proxychains+nc/nmap -sT) could observe false-positive connect success for unreachable ports.After this change, SOCKS5 success/failure is sent only when the real upstream connect outcome is known.
Problem
In affected paths, the SOCKS5 listener acknowledged
CONNECTtoo early.This breaks expected SOCKS5 semantics and can produce incorrect scan/probe results.
Root Cause
REP=Succeededimmediately after SOCKS handshake acceptance, instead of waiting for upstream tunnel connect outcome.What Changed
--timeout-connect(alias--timeout-connect-sec)WSTUNNEL_TIMEOUT_CONNECT3sWebTransportTunnelRead::read_handshake_byte()plus the same deferred reply in the webtransport handlerwtsarm now sends the SOCKS failure reply when connecting to the server fails, as thewsandhttp2arms already didBehavioral Outcomes
proxychains-based TCP connect probing now reflects target state more accurately.Rebase onto WebTransport
This branch is rebased onto
14168c5. WebTransport landed upstream in05a779eafter this PR was opened, adding a third transport to the same paths this PR touches, which surfaced two gaps.On the reverse path, the connect handshake is a single byte the client writes into the tunnel and the server reads before replying to the SOCKS client. Only the websocket and http2 server handlers implemented that read, while the client wrote the byte on any transport. Over
wts://it was therefore never consumed as a handshake and arrived as the first byte of the payload, corrupting the stream.On the forward path, a failed connection to the server sends the SOCKS client a failure reply, but the
wtsarm returned early instead, so the SOCKS client waited until the socket closed rather than being told the connect had failed.Both are now closed rather than worked around.
WebTransportTunnelRead::read_handshake_byte()mirrors the http2 read half, and the webtransport handler gates its deferred reply the same way the other two do. A QUIC stream is a byte stream, so taking one byte leaves the remainder queued and no prefetch buffer is needed — the webtransport read half is the simplest of the three. Reverse and forward SOCKS5 now behave identically across websocket, http2 and WebTransport, with integration coverage for connect success and failure over the new transport.Validation
The checks CI now runs on every pull request (per
CONTRIBUTING.md) were run locally, for both crypto providers:cargo fmt --all -- --checktaplo fmt --checkcargo clippy --all --all-features --locked -- -D warningscargo nextest run --lockedcargo nextest run --locked --no-default-features --features ringAll pass.
test_proxy_connectionneeds a Docker daemon and was skipped locally; it is covered by the CI run below.The full workflow was also run against this branch on a fork and is green: 15/15 jobs, with
Releaseskipped as tag-gated. That coversCheck - format, lint & testsand the entire build matrix — Linux x86_64/x86/aarch64/armv7hf/armv6, macOS x86_64/aarch64, Windows x86_64/x86/aarch64, FreeBSD x86_64/x86, and Android aarch64/armv7.End-to-end, against the built binary
Beyond the test suite, the release binary was driven directly over every transport that carries
SOCKS5 —
ws,wss,http,https,wts— in both forward (-L socks5://) and reverse(
-R socks5://) mode. Each combination was checked twice:CONNECTto a listening target, whichmust reply
0x00and pass bytes both ways, andCONNECTto a port nothing is bound to, which mustreply
0x01. Twenty cases in total, run identically against14168c5and against this branch:CONNECTto closed portCONNECTto listening target14168c50x00— false success, all 100x01— correct failure, all 10So the reported behavior reproduces on every transport in both directions, is fixed on every
transport in both directions, and the success path is unaffected.
The harness that produced this table is here, if you want to reproduce it yourself:
https://gist.github.com/0typos/3e823b2541470cb969c8100c9b187c3a — one stdlib-only Python file, run
as
python3 e2e_socks5.py ./target/release/wstunnel. It is deliberately not part of this PR: itdrives the built binary and spawns real listeners, so it does not belong in the crate's test suite.
Notes
--timeout-connect(alias--timeout-connect-sec) andWSTUNNEL_TIMEOUT_CONNECT.nmap/proxychains.clippy --all --all-features -- -D warningsgate; noallowattributes were added to get there.Reviewer-Oriented Change Summary (File-by-File)
wstunnel/src/protocols/socks5/tcp_server.rsSocks5WriteHalf::Tcpto holdpending_replystate.CONNECTsuccess reply in accept loop.send_reply_if_needed()to emit a one-shot SOCKS reply only when connect outcome is known.AsyncWriteimpl for new enum shape.wstunnel/src/tunnel/client/client.rsconnect()result to server transport.HANDSHAKE_CONNECT_OK/FAIL) to report local connector outcome back to server.Wtsarm now sends the SOCKS failure reply when connecting to the server fails, matching theWsandHttp2arms instead of returning early.wstunnel/src/tunnel/server/server.rshandle_tunnel_request()now carries whether request is reverse-socks5 and uses a writer type that can be downcast safely.self.config.timeout_connectin connector paths.wstunnel/src/tunnel/server/handler_websocket.rswstunnel/src/tunnel/server/handler_http2.rswstunnel/src/tunnel/transport/websocket.rsread_handshake_byte()andprefetchedbuffer inWebsocketTunnelRead.wstunnel/src/tunnel/transport/http2.rsread_handshake_byte()andprefetchedbuffer inHttp2TunnelRead.wstunnel/src/tunnel/reverse_socks5.rs(new)wstunnel/src/tunnel/server/socks5_reply.rs(new)AnyAsyncWritetrait and helper to downcast writer toSocks5WriteHalfsafely and send reply if needed.wstunnel/src/tunnel/server/mod.rssocks5_replymodule exports.wstunnel/src/tunnel/mod.rsreverse_socks5module internally.wstunnel/src/test_integrations.rsCONNECTsuccess and failure over wstunnel.free_addr()helper and theclient_ws(server_port, dns_resolver)form introduced in ci: run tests, formatting and lint checks on pull requests #529, rather than fixed ports.wstunnel/src/config.rs--timeout-connect(alias--timeout-connect-sec) andWSTUNNEL_TIMEOUT_CONNECTto both client and server options.3s.wstunnel/src/lib.rstimeout_connectdefaults with CLI-configured values for both client and server runtime config.wstunnel/src/tunnel/server/handler_webtransport.rswstunnel/src/tunnel/transport/webtransport.rsread_handshake_byte()toWebTransportTunnelRead, viaRecvStream::read_exact.Disclosure
The original code changes, repro script, issue draft, and first version of this description were prepared with assistance from ChatGPT 5.3 Codex.
The rebase onto
14168c5was prepared with assistance from Claude Opus 5, via Claude Code: conflict resolution against #529, the commit carrying the connect handshake and the forward failure reply over WebTransport, and the updates to this description.I manually reviewed the code and manually tested the behavior of all changes in this PR, including the rebase, before submitting.