Skip to content

Fix IPv6 extension header walk in handle_ip() - #778

Merged
kasnder merged 2 commits into
masterfrom
fix/ipv6-ext-header-walk
Aug 22, 2026
Merged

Fix IPv6 extension header walk in handle_ip()#778
kasnder merged 2 commits into
masterfrom
fix/ipv6-ext-header-walk

Conversation

@kasnder

@kasnder kasnder commented Aug 22, 2026

Copy link
Copy Markdown
Member

Fixes #769.

User-visible impact today (before this fix)

The extension-header walk in handle_ip() has a loop-termination bug: the while condition tests is_lower_layer(ext->ip6e_nxt) (the header after the one currently being examined) but the code assigns protocol = ext->ip6e_nxt — so protocol can only ever be re-assigned another extension-header type, never the real upper-layer protocol found at the end of the chain. Concretely, even the simplest real-world case — a single Hop-by-Hop header immediately followed by TCP — fails: the loop body never executes at all (ext->ip6e_nxt there already is TCP, which is not "lower layer"), so the walk always falls through to the bail-out branch, which resets off = 0 and protocol = ip6hdr->ip6_nxt (the first extension-header type, e.g. 0/Hop-by-Hop).

Downstream, protocol no longer matches IPPROTO_TCP/UDP/ICMP/ICMPV6, so:

  • sport/dport stay 0 and none of the TCP/UDP/ICMP-specific fast paths apply; the packet instead goes through the generic IP-based is_address_allowed() block decision (so IP/tracker blocking itself still works, just without any port-aware nuance).
  • If the destination is allowed and the flow is not tunnelled through WireGuard, none of handle_icmp/handle_udp/handle_tcp fire (none match protocol), so the packet is silently dropped — never forwarded, and not logged as blocked either (this is a connectivity bug, not a privacy one).
  • If the destination is allowed and the flow is tunnelled through WireGuard, write_wireguard_packet() forwards the full raw packet byte-for-byte regardless of protocol/payload, so WireGuard-routed IPv6 traffic with extension headers is unaffected by this bug today.
  • If the destination is blocked, the packet is correctly dropped either way, just logged with the wrong protocol number.

So: real breakage is confined to direct-routed (non-WireGuard) IPv6 flows that use any IPv6 extension header — they get silently blackholed today. This is on top of the separate, more serious memory-safety issue: the walk has no bounds checking at all against the packet buffer, so a header's declared length can walk ext/payload past the end of the buffer.

What changed

Extracted the walk into app/src/main/jni/netguard/ip6_ext.{h,c} — pure C, no JNI/Android headers, following the dns_frame.c/dns_frame.h extraction precedent (PR #773's HostsBlocklistLogic pattern) — so it can be unit-tested on the host. handle_ip() (ip.c) now calls ip6_skip_ext_headers(pkt, length, &protocol, &payload_off) and uses payload_off directly (fixing the double-add of sizeof(ip6_hdr) that existed in the original bail-out arithmetic, latent because the loop never reached a state where it mattered).

Fixes applied:

  • Hdr Ext Len arithmetic: RFC 8200's 8-octet units, not raw octets — 8 * (len + 1), not 8 + len.
  • Hard bounds checks: every read of a header's own 2-byte prefix is bounds-checked before it happens, and a header's declared length is checked against the remaining buffer before advancing — off never exceeds length.
  • Loop termination: rewritten so protocol/payload_off only ever indicate success when an upper-layer protocol was actually found.
  • Cap on headers walked: MAX_IP6_EXT_HEADERS = 8 bounds a maliciously (or corruptly) long chain.
  • Clean stop states: on any "no parseable L4" outcome, protocol_out is set to the header/type the walk stopped on (never an IPPROTO_TCP/UDP/ICMP/ICMPV6 value), and payload_off_out is always <= length — so a caller that only special-cases those four values (as handle_ip() does) treats a stopped walk exactly like today's "unknown protocol" path, never misreading extension-header bytes as a transport header.

Header types walked vs. stopped, and why:

  • Walked (RFC 8200 8-octet units): Hop-by-Hop (0), Routing (43), Destination Options (60).
  • Walked (RFC 4302 4-octet units, (len + 2) * 4): Authentication Header (51) — AH only authenticates, it doesn't encrypt, so a transport-mode AH header still has plaintext headers behind it worth walking into.
  • Stopped, not walked: Fragment (44) — its Hdr Ext Len-shaped byte is actually a reserved field, not a length, so it cannot be walked the same way, and a non-first fragment has no upper-layer header at all. ESP (50) — payload is encrypted, nothing after it is parseable in plaintext. No Next Header (59) — a clean, legitimate end of chain. Mobility (135) was previously listed as walkable; dropped (out of scope/rare, and not safely walkable the same way).
  • Also fixed: the original is_lower_layer() listed Destination Options (60) twice and never actually needed to (is_upper_layer supersedes it structurally in the new design).

No new preferences or user-facing options — this is a pure correctness/safety fix inside the native packet engine, consistent with the project's "simplicity over configurability" bias.

Testing

Added app/src/test/native/ip6_ext_test.c (mirrors the existing dns_frame_test.c host-test pattern) and a CI step in .github/workflows/test.yml right after the DNS-framing one. Covers: no extension headers (plain IPv6, the common case); a single Hop-by-Hop header then TCP; a chain of three headers (Hop-by-Hop → Routing → Destination Options → TCP); AH's distinct 4-octet-unit length, followed by UDP; a declared header length that runs past the buffer; a truncated packet (header start present, no room for its own length byte); Fragment; ESP; No Next Header; and a chain long enough to hit the 8-header cap (with a real TCP header planted right where an unbounded walk would wrongly reach it).

Built and verified locally:

  • cc -Wall -Wextra -Werror -Iapp/src/main/jni/netguard -o /tmp/ip6_ext_test app/src/test/native/ip6_ext_test.c app/src/main/jni/netguard/ip6_ext.c && /tmp/ip6_ext_test → all 10 tests pass.
  • ./gradlew :app:compileGithubDebugJavaWithJavac → succeeds.
  • ./gradlew assembleGithubDebug → succeeds for all four ABIs (native C + Rust); produced TrackerControl-githubDebug-latest.apk.
  • ./gradlew :app:testGithubDebugUnitTest → 279 tests, 0 failures, 0 errors.

Negative control: re-implemented the old buggy inline walk (same wrong 8 + len arithmetic and the same broken loop-termination logic) under the new function signature and linked it against the same, unmodified test file. Result: 10 of the new assertions fail against it —

FAIL: single hop-by-hop: succeeds
FAIL: single hop-by-hop: protocol is TCP
FAIL: single hop-by-hop: payload after the 8-byte header
FAIL: chain of three: succeeds
FAIL: chain of three: protocol is TCP
FAIL: chain of three: payload after all three headers
FAIL: AH then UDP: succeeds
FAIL: AH then UDP: protocol is UDP
FAIL: AH then UDP: payload after AH's 4-octet-unit length
FAIL: long chain: payload_off right before the unreached 9th header
ip6_ext_test: 10 assertion(s) failed

This confirms the tests genuinely exercise the loop-termination bug (defect 4) — even the single-extension-header and three-header-chain cases, which is the dominant real-world failure mode — not just passing vacuously against the new code.

Out of scope

  • Adding userspace forwarding for Fragment/ESP flows on the direct (non-WireGuard) path — the engine has never had handle_fragment/handle_esp, and building that is unrelated to fixing the walk itself.
  • Reassembling fragmented IPv6 packets to find the upper-layer header on a non-first fragment.

🤖 Generated with Claude Code

kasnder and others added 2 commits August 22, 2026 15:30
The inline extension-header walk in handle_ip() (ip.c) had several
compounding defects: Hdr Ext Len was treated as raw octets instead of
RFC 8200's 8-octet units, there was no bounds checking against the
packet buffer at all, the loop's termination condition could never
actually assign an upper-layer protocol to `protocol` (so the walk
degenerated into a bail-out for essentially any IPv6 packet carrying
an extension header, even a single Hop-by-Hop header before TCP), and
the bail-out path double-added sizeof(ip6_hdr) into the payload offset.

Extract the walk into app/src/main/jni/netguard/ip6_ext.{h,c} (pure C,
no JNI/Android dependency, following the dns_frame.c precedent) so it
is unit-testable on the host, add hard bounds checks and a cap on the
number of headers walked, fix the Hdr Ext Len arithmetic (including
AH's distinct 4-octet-unit encoding), and drop Fragment/ESP/Mobility
from the walkable set (they either cannot be walked at all, or their
length field means something else). handle_ip() now calls into this
module and uses its offset directly, removing the double-add.

Add app/src/test/native/ip6_ext_test.c and a CI step mirroring the
existing dns_frame_test.c host-test precedent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every walkable-chain case in the suite used Hdr Ext Len 0, where RFC 8200's
8 * (len + 1) and the original 8 + len both give 8. Swapping the new
arithmetic back for the old one left the whole suite green, so it did not
actually cover the defect the fix is mainly about.

Add a Hop-by-Hop with Hdr Ext Len 1 (16 bytes, buggy form 9) and a Routing
header with Hdr Ext Len 3 (32 bytes, buggy form 11). With only that one
line reverted, these two now fail and nothing else does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kasnder

kasnder commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

Pushed 010524a2 on top: two extra host tests, because the suite as written could not detect the defect this PR is mainly about.

Every walkable-chain case used Hdr Ext Len 0, and at 0 the RFC 8200 form 8 * (len + 1) and the original 8 + len both give 8. Reverting just that one line in ip6_ext.c and re-running left the whole suite green. (The negative control reported in the PR body replaced the entire old loop, so it failed for bounds and termination reasons that masked the arithmetic being uncovered.)

Added a Hop-by-Hop with Hdr Ext Len 1 (16 bytes; buggy form gives 9) and a Routing header with Hdr Ext Len 3 (32 bytes; buggy form gives 11). With only the arithmetic line reverted, exactly those two now fail:

FAIL: hop-by-hop with options: advance is 8 * (len + 1), not 8 + len
FAIL: routing with options: advance is 8 * (len + 1), not 8 + len
ip6_ext_test: 2 assertion(s) failed

Also verified independently, on top of what the PR body reports:

  • cc -Wall -Wextra -Werror clean, and the suite passes under -fsanitize=address.
  • Two further isolated negative controls: walking AH with 8-octet units instead of 4 fails exactly the AH assertion; disabling the advance > length || off > length - advance bounds check fails exactly the four declared-length-past-end assertions.
  • assembleGithubDebug builds libnetguard.so for all four ABIs, and is_lower_layer/is_upper_layer have no remaining callers anywhere in app/src/main/jni/.
  • The payload_off == length boundary is safe at the call site: the ICMP, UDP and TCP branches in handle_ip() each check length - (payload - pkt) against the header size before dereferencing.

@kasnder
kasnder merged commit 6ab6ec4 into master Aug 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Correctly walk IPv6 extension headers in handle_ip() (ip.c)

1 participant