Improve TLS ClientHello SNI parsing (research mode)#669
Merged
Conversation
Addresses the tls.c gaps raised in #654/#655 for the opt-in SNI extraction path (jni_sni / is_play). parse_tls_header: - Return status codes instead of void so the caller can tell an incomplete TLS record (buffer more) apart from a complete record with no SNI or a non-TLS payload. TLS_PARSE_INCOMPLETE is returned whenever the record length exceeds the bytes present. - Fix the off-by-one in parse_server_name_extension: the entry-header bound was `pos + 3 < data_len`, which dropped a server name ending exactly at the buffer boundary. It is now `pos + 3 <= data_len`. ip.c: - Fix a variable-shadowing bug: the TCP session found for a 443 flow was stored in a shadowed local, so the outer `cur` stayed NULL. As a result checkedHostname was never set and the block decision re-ran on every packet. The found session is now assigned to the single `cur`. - Reassemble a ClientHello that spans multiple TCP segments. When the first segment yields an incomplete record, buffer the payload per session (bounded by TLS_SNI_MAX_BUFFER) and re-parse as later segments arrive, instead of losing the SNI for good. The block decision is postponed while reassembling so it is not made on partial evidence. The buffer is allocated only for a split ClientHello and freed as soon as the record completes or the cap is reached, keeping the cost bounded and confined to research mode. tcp.c / netguard.h: - Initialise checkedHostname at session creation (previously read from uninitialised malloc memory) and add the reassembly buffer fields, freed in clear_tcp_data. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fqmytQ1CBbvpXKrweEm7z
A blocked SNI verdict on an established 443 session only dropped the deciding segment; retransmissions then passed because the decision is once-per-session. Send an RST to the client instead, mirroring handle_tcp's handling of blocked new sessions. Also stop allocating the 16 KiB reassembly buffer on zero-length segments (the handshake ACK), deferring without buffering instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Continuation segments were appended to the reassembly buffer blindly, so a retransmitted segment (e.g. when the engine is slow to ACK) would duplicate bytes and corrupt the buffer, losing the SNI. Track the next expected sequence number: ignore pure retransmissions, append only the fresh tail of a partial overlap, and abandon reassembly on a forward gap rather than parse a buffer with a hole. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the opt-in research-mode TLS ClientHello SNI extraction path used by the native NetGuard engine, primarily to (1) correctly distinguish incomplete TLS records from non-SNI cases, and (2) add bounded TCP-segment reassembly so SNI isn’t lost when a ClientHello spans multiple segments.
Changes:
- Updated
parse_tls_headerto return explicit status codes (success / incomplete / no SNI / invalid) and fixed an SNI list boundary off-by-one. - Added bounded per-session buffering to reassemble split ClientHello payloads (research mode only), and ensured session state is initialized and freed correctly.
- Fixed TCP session lookup shadowing so
checkedHostnamegating works as intended.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/jni/netguard/tls.h | Exposes FQDN_MAX and new parse_tls_header return codes/status API. |
| app/src/main/jni/netguard/tls.c | Implements new return-code semantics; fixes SNI parsing boundary condition. |
| app/src/main/jni/netguard/ip.c | Implements per-session ClientHello buffering/retry and avoids re-deciding every packet. |
| app/src/main/jni/netguard/tcp.c | Initializes new TCP session fields and frees buffered TLS data on teardown. |
| app/src/main/jni/netguard/netguard.h | Adds TCP session fields for TLS reassembly buffer + length. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // post-quantum key shares. The buffer is per-session, allocated only for a | ||
| // split ClientHello, and freed as soon as the record is complete or the cap is | ||
| // reached, so the battery/memory cost stays bounded and confined to is_play. | ||
| #define TLS_SNI_MAX_BUFFER 16384 |
Comment on lines
355
to
+359
| char* packetdata = data; | ||
| // While a ClientHello is still being reassembled across TCP segments we | ||
| // let the segment through but postpone the block decision until the SNI | ||
| // is available (or we give up), so it is not made on partial evidence. | ||
| int defer_sni = 0; |
Comment on lines
+393
to
+395
| if (cur != NULL && cur->tcp.checkedHostname == 0) { | ||
| char hostname[FQDN_MAX + 1] = ""; | ||
| int rc; |
Comment on lines
+430
to
+433
| if (strnlen(hostname, sizeof(hostname)) > 0) { | ||
| log_android(ANDROID_LOG_DEBUG, "Seen SNI: %s", hostname); | ||
| packetdata = hostname; | ||
| } |
This reverts commit 0a10948. Not worth the extra native code: the corruption it guards against (a retransmission during the few-ms reassembly window) is rare and already degrades safely — the TLS structure checks reject the buffer and the decision falls back to IP/DNS evidence. Blind best-effort append is the documented scope of research-mode reassembly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Addresses the
tls.cgaps raised in the #655 comment (from #654) for the opt-in, research-mode SNI extraction path (jni_sni/is_play). This is not a change to DNS-based blocking — per the discussion on #655, DNS attribution stays system-wide.The SNI path had three concrete defects and a missing capability:
parse_tls_headersilently gave up and the SNI was lost.curvariable shadowing. The TCP session found for a port-443 flow was stored in a shadowed local, so the outercurstayedNULL. ConsequentlycheckedHostnamewas never set and the address decision re-ran on every packet.checkedHostname.ng_mallocdoes not zero memory, and the field was never initialised at session creation, so the per-session "already checked" gate read garbage.parse_server_name_extension(tls.c:66): the entry-header boundpos + 3 < data_lendropped a server name ending exactly at the buffer boundary.Changes
tls.c/tls.hparse_tls_headernow returns a status code (>=0hostname length,TLS_PARSE_INCOMPLETE,TLS_PARSE_NO_SNI,TLS_PARSE_INVALID) instead ofvoid, so the caller can distinguish "record not fully received — buffer more" from "complete, no SNI" and "not TLS".TLS_PARSE_INCOMPLETEis returned whenever the declared record length exceeds the bytes present.<→<=).FQDN_MAXhoisted to the header so the caller can size its buffer.ip.ccur(removes the shadowing), so the decision block sees it andcheckedHostnameis honoured.TLS_SNI_MAX_BUFFER= 16 KiB, one max TLS record) and re-parse as later segments arrive. The block decision is postponed while reassembling so it is never made on partial evidence. The buffer is allocated only for a split ClientHello and freed as soon as the record completes or the cap is reached — keeping the cost bounded and confined to research mode, in line with the battery-first constraint flagged in the issue.tcp.c/netguard.hcheckedHostname = 0at session creation and add thetls_data/tls_lenreassembly fields, freed inclear_tcp_data(all TCP teardown paths route through it, so no leak).Testing
The full Android/NDK build cannot run in this environment (the NDK is not installed and the required Gradle 9.6.1 distribution host is blocked by egress policy), so
tls.cwas validated with a host-side harness compiled-Wall -Wextra(zero warnings):TLS_PARSE_INCOMPLETE(drives reassembly);ip.clogic) → SNI found exactly when the record completes;TLS_PARSE_INVALID(never buffered);TLS_PARSE_NO_SNI.CI will exercise the full native build.
Notes / scope
🤖 Generated with Claude Code
Generated by Claude Code