Skip to content

Fix memory-safety and hang bugs in native packet engine - #751

Open
kasnder wants to merge 1 commit into
masterfrom
agent/native-c-safety-fixes
Open

Fix memory-safety and hang bugs in native packet engine#751
kasnder wants to merge 1 commit into
masterfrom
agent/native-c-safety-fixes

Conversation

@kasnder

@kasnder kasnder commented Aug 22, 2026

Copy link
Copy Markdown
Member

Found during a correctness audit of app/src/main/jni/netguard/ (all inherited from the NetGuard import unless noted). 50 insertions, 18 deletions across 7 C files; no behavior change for well-formed packets.

Memory safety

  • dhcp.c:84 — DHCP reply options were written ~57 KB past the 500-byte response allocation: (uint8_t *)(response + sizeof(struct dhcp_packet)) advances in struct-sized units (~240 B each). Reachable by any co-installed app with a single UDP datagram to port 67 (sport 68 / dport 67 → check_dhcp) — no special permission needed. This is the headline fix in this PR.
  • ip.c / tcp.c — the TCP data offset (doff) was never validated before deriving payload pointer/length from it: doff < 5 underflows tcpoptlen to e.g. 236, and an oversized doff wraps the derived length. Both SNI memcpys into the reassembly buffer were already bounded (datalen < TLS_SNI_MAX_BUFFER, copy = min(datalen, space)), so this was never a 65 KB write into the 16 KB buffer. The actual defect is an out-of-bounds read: the bad offset lets data/datalen point past the end of the get_mtu()-sized tun read buffer, so parse_tls_header (and, in tcp.c, the queued bytes) can walk off the end of that buffer. Both sites now reject impossible data offsets.
  • ip.c — added the same guard for IPv4's ihl: (uint8_t)((ihl - 5) * 4) underflows to 236 when ihl == 0, pushing payload past the packet; the downstream length checks are size_t so they wrap too and pass. Now rejected next to the existing tot_len check.

Hangs / correctness

  • tcp.c — SYN option parsing made no progress for an option with length 0 (kind ∉ {0,1}) → infinite loop on the VPN event thread. (optlen is int, so an over-long option already made len > optlen go negative and exit the loop on its own — the hang was real only for len == 0.) Loop always terminates now and stays inside the declared options region.
  • icmp.c — ICMPv6 echo replies used pseudo-header length bytes - sizeof(ip6_hdr) (underflow, host order); now htonl(bytes), matching the query path.
  • pcap.c — rollover flushed the FILE* and truncated the underlying fd, then re-seeked with lseek(fileno(pcap_file), ...), which never syncs the FILE*'s own buffered offset — ftell() kept reporting the pre-rollover size and the file re-truncated on every later write. Now fseek(pcap_file, ...) on the FILE* itself.

Hardening

  • vsnprintf instead of vsprintf into fixed 1024-byte stack buffers (log/exit formatting).
  • SOCKS5 strings received from Java are copied with bounds checks (warn on truncation).
  • Seed rand() once at init so TCP ISNs aren't identical across boots.

Reachability

Other than the DHCP heap overflow above, everything in this PR is only reachable from packets the kernel writes into the tun device on behalf of a local app (a malformed doff, TCP option, or ihl has to come from a real socket send). That needs CAP_NET_RAW/root to forge directly, so these are defensive hardening against a corrupted/malicious kernel path or future refactors, not exploitable by an unprivileged co-installed app today. The DHCP overflow is the one item here that is reachable right now, by any co-installed app, with a single UDP datagram — treat it as the headline fix.

Verification

  • NDK clang -fsyntax-only over all engine TUs: clean (only pre-existing const-qualifier warnings).
  • ./gradlew assembleGithubDebug (native C + Rust + Kotlin): BUILD SUCCESSFUL.
  • git diff --check: clean.

Known follow-ups (deliberately not in this PR)

  • A correct walk of IPv6 extension headers (to reach TCP/UDP inside chained-extension IPv6 packets) was dropped from this PR after review found the first attempt unsafe — wrong RFC 8200 length unit, ESP/AH have no walkable next-header field, and non-first fragments would be misparsed as L4 data. Filed separately for a correct implementation.
  • Session-list mutations in handle_events() run outside ctx->lock while jni_get_stats() iterates under it (narrow use-after-free window) — needs a small locking design, not a drive-by fix.
  • DNS-over-TCP: only isolated frames get their length prefix rewritten after blanking (coalesced/split reads keep original bytes; header counts still zeroed).

All found in an audit of the C engine and inherited from the NetGuard
import unless noted:

- dhcp.c: the DHCP reply options were written ~57KB past the 500-byte
  response allocation ("response + sizeof(struct dhcp_packet)" advances
  in struct-sized units). Reachable by any co-installed app with a single
  UDP datagram to port 67 -- no special permission needed. This is the
  headline fix.
- ip.c, tcp.c: validate the TCP data offset before deriving the payload
  pointer/length from it. A doff below 5 (or option bytes past the
  segment) previously wrapped datalen to ~65K; the read this enabled was
  out of bounds past the end of the get_mtu() tun read buffer, not an
  overflow of the (already length-checked) SNI reassembly buffer. Only
  reachable via a packet the kernel writes into the tun for a local app,
  so it needs CAP_NET_RAW/root -- defensive hardening, not exploitable
  today.
- ip.c: the IPv4 header-length underflow this doff fix mirrors --
  "(ihl - 5) * 4" wraps to 236 when ihl == 0, and the length checks
  further down are size_t so they wrap too and pass -- gets the same
  ihl < 5 guard next to the existing tot_len check. Same reachability
  caveat as above.
- tcp.c: a SYN option with length 0 could spin the event-loop thread
  forever; option walking now always progresses and stays inside the
  declared options region. (optlen is signed, so an over-long option
  already exited via the negative-length check; only the len == 0 case
  actually hung.) Same reachability caveat as above.
- icmp.c: ICMPv6 echo replies computed a bogus pseudo-header length
  (bytes - sizeof(ip6_hdr)) in host order; use htonl(bytes), matching
  the query path.
- pcap.c: rolling over the PCAP file skipped fflush before ftruncate,
  and re-seeked with lseek(fileno(...)) afterwards, which never syncs
  the FILE*'s own buffered offset -- ftell() kept reporting the
  pre-rollover size and the file re-truncated on every later write.
  fflush before ftruncate, then fseek() (not lseek on the fd) on the
  FILE* itself.
- Hardening: vsnprintf for log/exit formatting, bounded SOCKS5 string
  copies from Java, seeded rand() for ISNs.

Not included: a walk of IPv6 extension headers was dropped from this
change after review found it unsafe (wrong RFC 8200 unit, ESP/AH have no
walkable next-header field, and non-first fragments would be misparsed
as L4 data) -- tracked separately for a correct implementation.

No behavior change for well-formed packets: every new check only rejects
input that was previously misparsed or impossible.
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.

1 participant