Full-project review + fixes: fail-open security, quiche driver OOM/stall, TUIC lifecycle leaks, vacuous tests#53
Merged
Conversation
Assisted-by: Claude:claude-fable-5
is_private_ip only inspected the raw address bytes, so an IPv4-mapped IPv6 target such as `::ffff:10.0.0.1` fell through the V6 arm (first octet 0x00) and was classified as public. That let a client bypass the `drop_private` / loopback SSRF guards in wind-acl and tuic-server by writing an internal target in mapped form. Canonicalize with `to_canonical()` before matching, and add the 100.64.0.0/10 CGNAT range (RFC 6598) while here. Assisted-by: Claude:claude-fable-5
…d type to direct make_outbound_action matched "socks5" and let every other value fall through to DirectOutbound. A typo such as type = "Socks5" or "sock5" would then build a direct outbound with no error or warning, sending traffic out via the server's own IP instead of the intended SOCKS5 tunnel -- a silent security downgrade for hidden-egress deployments. Match "direct" explicitly and warn on any unrecognized value (still falling back to direct so behavior is unchanged, but now visible). Assisted-by: Claude:claude-fable-5
…ently The Cidr arm of address_to_rule_types returned an empty Vec on parse failure with no log, while the neighboring Ip arm warns. Because the grammar permits prefixes such as /999, a malformed `reject 10.0.0.0/99` would load fine and then have its rule vanish silently -- traffic that should be rejected gets allowed (fail-open). Warn like the Ip arm does. Assisted-by: Claude:claude-fable-5
…keywords localhost_kw/private_kw/suffix_localhost were bare literals with no trailing boundary, so they matched the prefix of a longer token. A rule like `proxy privatetracker.org` parsed as addr=private + hijack=tracker.org, silently routing the entire RFC1918 range to `proxy` and ignoring the intended domain; `allow localhost5.com` became localhost + port 5. Make the keywords atomic with a trailing `&(WHITESPACE | EOI)` assertion (matching how `ipv6` fixed the same class of bug). Atomic is required so pest does not consume the following whitespace before the lookahead runs. Assisted-by: Claude:claude-fable-5
DnsConfig used #[educe(Default)] but educe only affects Default::default(), not serde. Fields mode/stack_prefer had no serde default, so a table like [dns]\nmode = "google" (omitting stack_prefer) failed with "missing field stack_prefer" -- the same class of bug as the previously-fixed `attempts` field, which only patched one field. Add #[serde(default)] on the container so any omitted field falls back to Default::default() while deny_unknown_fields still rejects typos. Assisted-by: Claude:claude-fable-5
DnsConfig's doc says it defaults to the OS resolver "matching the pre-existing behaviour", but #[educe(Default)] sat on CloudflareTls. Users who omitted the [dns] section had every outbound lookup silently rerouted through Cloudflare DoT -- a behaviour and privacy change that also fails where port 853 is blocked. Move the default to System so code matches the docs and preserves backward compatibility. Assisted-by: Claude:claude-fable-5
Both the ACME (http01) and self-signed key writers used tokio::fs::write, which creates the file at the process umask (typically 0644), leaving the private key world-readable to other local users. Add a shared write_key_file helper that restricts the key file to mode 0600 on Unix (no-op elsewhere) and route both key writes through it. Cert chains stay a plain write since they are public. Assisted-by: Claude:claude-fable-5
TuicOutbound::new always bound the local UDP socket to 0.0.0.0 (IPv4). A quinn endpoint bound to IPv4 cannot dial an IPv6 peer -- connect returns InvalidRemoteAddress -- so a TUIC server reached over IPv6 (e.g. [::1]:8444) was unreachable from the client. Bind [::]:0 when the peer is IPv6, 0.0.0.0:0 otherwise. Found via the now-asserting IPv6 integration test. Assisted-by: Claude:claude-fable-5
The IP-literal check that substitutes a placeholder SNI parsed
relay.server.0 directly, so a bracketed IPv6 literal like `[::1]` failed
IpAddr::parse and was announced verbatim as the SNI, which rustls
rejects ("invalid server name"). Strip surrounding brackets before the
parse so bracketed IPv6 literals also get the placeholder SNI.
Assisted-by: Claude:claude-fable-5
test_server_client_integration and test_ipv6_server_client_integration discarded the bool result of the TCP/UDP relay helpers and wrapped each phase in `let _ = timeout(...)`, so the tests had no assertions and passed even if the relay was completely broken. Capture the helper results and assert them; propagate timeouts via .expect() instead of swallowing them. The IPv6 test now probes for an [::1] loopback and skips (rather than silently passing) when IPv6 is unavailable, disables the loopback guard so its own [::1] echo target isn't rejected, and the server timeouts are raised to 30s so the server outlasts the assertions. These assertions surfaced two real client bugs, fixed in the preceding commits (IPv6 endpoint bind family, IPv6 SNI brackets). Assisted-by: Claude:claude-fable-5
AclEngine built its MatchContext with ..Default::default(), leaving geoip_lookup/geosite_lookup as None, so GEOIP/GEOSITE rules compiled but never matched -- their traffic silently fell through to the default outbound (fail-open), and wind-geodata had no consumer at all. Add AclEngineBuilder::geodata(Arc<GeoData>); do_route now binds the database's lookup closures into the MatchContext. When no database is provided, build() warns if any GEOIP/GEOSITE rules are present (and separately for the still-unsupported IP-ASN rules) instead of silently never matching. Adds tests proving the rules match with geodata wired and confirm the fail-open without it. Note: the tuic-server binary currently routes via wind-core's AclRouter; switching it to AclEngine and loading the database from config is a follow-up. Assisted-by: Claude:claude-fable-5
make_outbound_action hardcoded stream_timeout to Duration::ZERO, which wind-base treats as "disable half-close relay reaping". The operator's configured stream_timeout (default 60s) was silently ignored, so idle half-closed relays were never reaped. Thread cfg.stream_timeout through to both the direct and socks5 outbound options. Assisted-by: Claude:claude-fable-5
…g in debug From<io::Error> for ProtoError panicked under debug_assertions. Since tokio-util's Framed* requires Decoder::Error: From<io::Error>, driving these codecs over real IO would crash on any transport error (e.g. a peer reset) in debug builds. Map to the Io variant unconditionally. Assisted-by: Claude:claude-fable-5
-D/--work_dir was parsed into Cli::work_dir but never read, so passing it had no effect. Change the process working directory early (before config load and the init generator) so relative config paths resolve against it as documented. Assisted-by: Claude:claude-fable-5
The 10s drain used `timeout(...).await?`, so exceeding it returned Err from main and exited non-zero -- turning a normal "long-lived sessions still open at shutdown" into a spurious error. Log a warning and exit cleanly instead. Assisted-by: Claude:claude-fable-5
validate() only ran rkyv's structural check, which does not cover the application-level invariant that each category/country slice [start, start+len) lies within its backing vector. A corrupt or bit-flipped cache that still passed the structural check could drive an out-of-bounds index panic on every routing query (DoS). Add ArchivedGeoDataSnapshot::validate_offsets(), called from validate(), which bounds-checks every geosite/geoip slice (with overflow-safe arithmetic) and confirms the name lists are sorted for binary search. Adds a regression test with an out-of-bounds category offset. Assisted-by: Claude:claude-fable-5
…ith 0 Both h3 OpenStreams/Connection close() impls ignored the h3::error::Code and closed the QUIC connection with application code 0, so the peer's CONNECTION_CLOSE always carried 0 and lost the protocol-level reason (e.g. H3_INTERNAL_ERROR). Map the code onto the u32 close code (saturating) instead. Assisted-by: Claude:claude-fable-5
hex (const-hex) is only used in a #[cfg(test)] block in proto/addr.rs, so it does not belong in the normal dependency set. Assisted-by: Claude:claude-fable-5
The normal 'server started, listening on ...' line was logged at warn. The forward.rs equivalent was already fixed; this was the last one. Assisted-by: Claude:claude-fable-5
Two concurrency defects in the quiche driver: 1. out_queue was unbounded. wait_for_data always re-armed a stream's send back-channel after buffering a chunk, so a flow-controlled peer let the driver drain the bounded handle channel into an unbounded out_queue -- a single slow peer could OOM the process. Track the queued byte count and, once it reaches MAX_PENDING_OUT, park the back-channel receiver instead of re-arming it; write_stream re-arms once the queue drains below the cap, so the local writer sees back-pressure. Also cap queued outbound datagrams (dropping oldest). 2. Inbound re-flush had a lost-wakeup. Overflow buffered in pending_in was only re-flushed when some event happened to wake the worker; a handle draining its recv channel produced no such event, so on a quiescent pure-upload stream the tail bytes and FIN could stall until idle timeout. QuicheRecv now nudges the driver (FlushInbound) when it drains a previously-full channel. Adds a backend-generic bulk-transfer loopback test (large payload, slow reader) exercising the buffering paths on both quinn and quiche. Assisted-by: Claude:claude-fable-5
TuicOutbound::handle_udp allocated a per-session child cancel token but never used it, and its outer loop only broke on the global context token. So when a UDP session's bridge task exited on its own (the local UDP channel closed), handle_udp kept spinning — logging every 30s, pinning the udp_session cache entry, and never sending a Dissociate. A long-lived client churning UDP associations leaked one task slot and one assoc id per dead session until the u16 assoc space was exhausted. handle_udp now awaits the bridge task (or global shutdown), then cancels the child token, removes the udp_session entry, and sends drop_udp. tuic-client's SOCKS5 associate handler no longer abort()s the outbound task — that skipped the teardown; it now detaches it so handle_udp can send the Dissociate as the closed local stream drives it to exit. Adds a regression test asserting handle_udp returns (and the cache drains) after the local stream closes; it hangs against the old loop. Assisted-by: Claude:claude-fable-5
With the masquerade feature, spawn_h3_router spawns run_masquerade per
connection, parked on select!{ cancel.cancelled(), go.notified() }. A
pure-TUIC peer never fires go, and the per-connection teardown only
cancels the acceptor/UDP child tokens — not the cancel token handed to
run_masquerade (only global shutdown/kick cancels it). So every normal
peer disconnect left the parked task alive, holding a clone of the
connection handle until process exit: one leaked task + handle per
closed connection.
Add a conn.closed() branch to the parked select so the task is bound to
the connection's lifetime and returns on disconnect. The h3 path is
unaffected (a stream fires go first); the masquerade integration test
still passes.
Assisted-by: Claude:claude-fable-5
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
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.
Overview
Second-round full-project code review of the wind/tuic workspace, plus fixes for the highest-value findings. The review report is committed as
CODE_REVIEW_ROUND2.md; this PR then fixes 22 issues across security, correctness, concurrency, and tests. Each fix is its own commit.Most of the previous round's 99 findings were already resolved; this round confirmed 73 new ones and fixes the load-bearing subset.
Fixes by theme
Fail-open security downgrades
is_private_ipnow canonicalizes IPv4-mapped IPv6 (::ffff:10.0.0.1) and adds the CGNAT range, closing adrop_private/loopback SSRF-guard bypass.localhost/privatekeywords now require a token boundary —proxy privatetracker.orgno longer parses as theprivateRFC1918 range + hijack.rejectrule silently vanishing was fail-open).typenow warns instead of silently becoming a direct outbound (typo → traffic bypasses the tunnel).QUIC quiche driver (high-severity concurrency)
out_queue: a flow-controlled peer could make the driver drain the handle channel into an unbounded queue → single slow peer OOM. Now back-pressures at a byte cap; outbound datagrams are capped too.pending_in(and the following FIN) could stall on a quiescent pure-upload stream.QuicheRecvnow nudges the driver when it drains a full channel.TUIC session/task lifecycle leaks (high-severity)
handle_udpnow returns when a UDP session's local stream closes — freeing its task, releasing the assoc id, dropping the cache entry, and sendingDissociate— instead of spinning forever until global shutdown. tuic-client's SOCKS5 associate handler no longerabort()s it (which skipped that teardown).conn.closed()), so a normal peer disconnect no longer leaks the parkedrun_masqueradetask + connection handle per connection.Config / correctness
wind-dns: container#[serde(default)]so partial[dns]tables parse; defaultDnsModeback toSystemto match the documented (backward-compatible) behaviour.tuic-server: honor the configuredstream_timeout(was hardcoded toDuration::ZERO, silently disabling half-close reaping).wind:--work_dirnow takes effect; shutdown drain timeout no longer causes a non-zero error exit.wind-acme: TLS private keys written0600on Unix.wind-geodata: validate slice offsets when opening a cache so a corrupt file can't OOB-panic on every query.tuic-core:From<io::Error>maps toProtoError::Ioinstead of panicking underdebug_assertions.wind-quic: forward the h3 close code instead of always closing with 0.GeoIP/GeoSite wiring
wind-acl:AclEngineBuilder::geodata(..)wires the database intoAclEnginesoGEOIP/GEOSITErules actually match (previously a silent no-op / fail-open);build()warns if geo rules are present without a database.Tests (were silently vacuous)
test_server_client_integration,test_ipv6_server_client_integration) discarded relay results and wrapped each phase inlet _ = timeout(...)— they had no assertions and passed even if the relay was fully broken. They now assert relay success. Adding the assertions surfaced two real IPv6 client bugs, both fixed here:[::1]was announced as an invalid server name.Testing
Full workspace compiles (
cargo check --workspace --all-targets). Added/verified regression tests, all green:wind-quicloopback + new backend-generic bulk-transfer test (quinn and quiche).wind-testnewudp_association_is_released_when_local_stream_closes— verified it hangs against the pre-fix loop, confirming it's a real guard.wind-aclnewgeodata_routingtests (geo rules match with data, fail-open without).wind-geodatanew out-of-bounds-offset rejection test.wind-dnsserde-default/default-mode tests.tuic-testsintegration (quinn + quiche + IPv6) and masquerade suites.Reviewer notes / follow-ups (not in this PR)
tuic-serverstill routes via wind-core'sAclRouter, not the now-geodata-capablewind-acl::AclEngine. Switching it over + loading the DB from config is a tracked follow-up.stream_senderrors, drop-without-finish emitting FIN instead of RESET) are left for focused, paired-test work — they'd change the hot inbound channel type and are risky to batch.CODE_REVIEW_ROUND2.md.🤖 Generated with Claude Code