Fix SSRF bypass vectors and HTTPS 502 double-logging#44
Merged
Conversation
The CONNECT handler was returning Err(e) after sending a 502 Bad Gateway response when the upstream TCP connection failed. This caused the caller in main.rs to log the error again, producing noisy duplicate log lines. Change return Err(e.into()) to return Ok(()) to match the pattern already used by the SSRF and DNS rebinding error paths. Closes #29
…n SSRF protection The IPv6 path in is_private_ip only checked is_loopback() and is_unspecified(), missing: - Unique-local addresses (fc00::/7) — the IPv6 equivalent of RFC 1918 private ranges, allowing SSRF to internal networks - Link-local addresses (fe80::/10) - IPv4-mapped IPv6 addresses (::ffff:0:0/96) — allowing attackers to bypass SSRF protection by encoding private IPv4 addresses in IPv6 notation (e.g., ::ffff:127.0.0.1) Also simplify is_private_address to delegate to is_private_ip after parsing, eliminating duplicated IPv4 check logic and ensuring both functions have identical coverage. Closes #32 Closes #33
…nning The HTTP handler resolved DNS once for SSRF verification, then reqwest resolved DNS independently when connecting. An attacker with a short-TTL DNS record could return a public IP on the first resolution (passing the SSRF check) and a private IP on the second (reaching internal services). Fix by passing the pre-verified resolved addresses into the request and building a per-request reqwest client with .resolve() to pin the hostname to those addresses. This matches the HTTPS handler which already connects directly to verified addresses. Closes #34
…ients The TOCTOU fix built per-request clients with hardcoded timeouts and no connection pooling settings. This caused every request to open a fresh TCP connection, losing keepalive, pool reuse, and HTTP/2 multiplexing — a significant throughput regression. Extract base_client_builder() so both HTTP_CLIENT and per-host pinned clients share identical timeout, pooling, and keepalive settings. Also clarify the defensive fallback path with a comment explaining it is unreachable in normal proxy operation.
The ignored test required network access and ~75s for TCP timeout, meaning it never ran in CI — providing false coverage. The one-line fix (Err → Ok) is trivially correct by inspection and follows the identical pattern used by the SSRF and DNS rebinding error paths directly above it. Replace the dead test with an inline comment explaining why the return must be Ok(()) to prevent double-logging.
Rust's IpAddr parser rejects IPv6 addresses with zone ID suffixes (e.g., "fe80::1%eth0"). This caused is_private_address to return false for link-local addresses with zone IDs, bypassing the SSRF check. Strip the zone ID (everything after %) before parsing.
This was referenced Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix HTTPS 502 double-logging — The CONNECT handler returned
Err(e)after sending a 502 response, causing the caller to log the same error again. Changed toreturn Ok(())with an inline comment explaining the invariant.Block IPv6 unique-local (fc00::/7) and link-local (fe80::/10) addresses — The IPv6 SSRF check only covered loopback and unspecified, missing the IPv6 equivalents of RFC 1918 private ranges.
Block IPv4-mapped IPv6 bypass (::ffff:0:0/96) — Attackers could encode private IPv4 addresses in IPv6 notation (e.g.,
::ffff:127.0.0.1) to bypass SSRF protection. Now extracts and validates the embedded IPv4 address.Close DNS TOCTOU gap in HTTP forwarding — The HTTP handler resolved DNS for SSRF verification, then reqwest resolved DNS independently. Fixed by building a per-host client pinned to the pre-verified addresses via
reqwest::Client::builder().resolve().Restore connection pooling on pinned clients — Extract
base_client_builder()so both the staticHTTP_CLIENTand per-host pinned clients share identical timeout, pooling, and keepalive settings.Handle IPv6 zone IDs in SSRF check —
fe80::1%eth0failed to parse as anIpAddr, causingis_private_addressto return false. Now strips the zone ID suffix before parsing.Also simplifies
is_private_addressto delegate tois_private_ipafter parsing, eliminating duplicated IPv4 check logic.Test plan
test_is_private_ip_v6_unique_local— fc00::/7 and fe80::/10 blockedtest_is_private_ip_v4_mapped_v6— ::ffff:private blocked, ::ffff:public allowedtest_is_private_address_v6_unique_local— string-based check covers unique-localtest_is_private_address_v4_mapped_v6— string-based check covers IPv4-mappedtest_is_private_address_v6_zone_id— fe80::1%eth0 correctly detected as privatetest_send_request_uses_resolved_addrs— proves reqwest uses pinned IPs, not re-resolving DNScargo fmtandcargo clippycleanCloses #29, closes #32, closes #33, closes #34