Skip SNI research mode for tunnelled flows - #744
Merged
Conversation
SNI research mode reassembles a ClientHello on the ng_session that handle_tcp creates for a 443 flow. A flow routed through the WireGuard tunnel never gets one: handle_ip writes the packet to the WG bridge and returns before handle_tcp runs. So the reassembly guard always saw cur == NULL and no SNI was ever collected, while every later segment re-ran is_address_allowed() and a UID lookup, because the once-per-session shortcut lives inside a cur != NULL branch those flows never reach. Resolve the routing verdict before the 443 decision sites and, when the flow tunnels, take the ordinary non-research path for it: decide once on the SYN by IP, then allowed = 1 — exactly how WireGuard behaves without research mode. Nothing changes when WireGuard is off, or for flows that per-app routing keeps direct, which still collect SNI as before. The tunnel_uid resolution is factored into resolve_tunnel_uid() and shared with the routing fork, which reuses the early answer rather than resolving and re-storing it a second time. Also stop the blocking-mode summary from implying research mode still works: it now says SNI extraction does not apply to remotely routed traffic whenever SNI and a configured remote VPN are both on. Refs #735
- AGENTS.md's privacy-preservation principle claimed SNI parsing runs the same whether WireGuard is on or off, which this PR makes false. Update it to describe the actual behaviour (research mode collects nothing for tunnelled flows, surfaced in the Research summary). - Move the SNI/WireGuard caveat from the blocking_mode summary to the Research (log_logcat) preference summary, since that is the control that actually toggles sni_enabled; blocking_mode's summary never mentioned SNI at all. - resolve_tunnel_uid() now takes an optional out_uid parameter so the SNI-attribution fallback in handle_ip can reuse a UID the routing resolution already looked up, instead of paying for a third lookup (flow-cache miss -> session table -> procfs/Binder) on the same packet.
kasnder
marked this pull request as ready for review
August 20, 2026 22:35
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
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.
Fixes #735 by making research mode cheap and honest under the tunnel, rather than building SNI-under-tunnel.
The problem
SNI research mode reassembles a ClientHello on the
ng_sessionthathandle_tcpcreates for a 443 flow. A flow routed through WireGuard never gets one —handle_ipwrites the packet to the WG bridge and returns beforehandle_tcpruns. So onmaster, withsni_enabledand WireGuard both on:cur != NULL && cur->tcp.checkedHostname == 0never holds,parse_tls_headeris never called, and research mode collects nothing;if (!defer_sni && (cur == NULL || ...))and pays acreate_packet+is_address_allowedJNI upcall and a UID lookup — per packet, forever, because thecheckedHostname = 1shortcut sits inside thecur != NULLbranch those flows never reach.The change
ip.cresolves the routing verdict before the 443 decision sites. If the flow tunnels,sni_activeis cleared and the packet takes the ordinary non-research path: decide once on the SYN by IP,allowed = 1afterwards — exactly how WireGuard behaves without research mode. That removes the per-packet upcall and stops research mode pretending to work.tunnel_uidresolution (flow cache → session table → authoritative lookup, with the fail-closed fallback) is factored intoresolve_tunnel_uid()and shared with the routing fork, which reuses the early answer instead of resolving and re-storing it a second time.ActivitySettingsappends a one-line note to the blocking-mode summary whensni_enabledand a configuredwg_enabledare both on, so the UI stops implying extraction still applies to tunnelled traffic. One new English string; no new preference or toggle.Not done here
Making SNI work through the tunnel needs per-flow
checkedHostname+ reassembly state keyed likeroute_flow_store, with buffer lifetime on a cache rather than a session — and a blocked verdict there still could notwrite_rst. That is a feature, not this bug; worth a separate issue if it is wanted.Verification
Not built. This container has no Android SDK (
android-envexits 3), so:app:compileGithubDebugJavaWithJavac,:app:assembleGithubDebug(the one that actually compilesip.c) and:app:lintGithubDebugwere not run. Draft until someone runs them — the C change is unverified by a compiler. Reviewed by hand against the surrounding code instead.Generated by Claude Code