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

[kasnder]

Background

PR #751 ("Fix memory-safety and hang bugs in the native packet engine") originally included a hunk that made handle_ip() in app/src/main/jni/netguard/ip.c actually walk IPv6 extension headers to find the upper-layer protocol. It was pulled out of that PR before merge because review found it unsafe. This issue tracks doing the walk correctly.

Current (master) behavior

master's loop at ip.c:362-385 looks like:

uint16_t off = 0;
protocol = ip6hdr->ip6_nxt;
if (!is_upper_layer(protocol)) {
    off = sizeof(struct ip6_hdr);
    struct ip6_ext *ext = (struct ip6_ext *) (pkt + off);
    while (is_lower_layer(ext->ip6e_nxt) && !is_upper_layer(protocol)) {
        protocol = ext->ip6e_nxt;
        off += (8 + ext->ip6e_len);
        ext = (struct ip6_ext *) (pkt + off);
    }
    if (!is_upper_layer(protocol)) {
        off = 0;
        protocol = ip6hdr->ip6_nxt;
    }
}
payload = (uint8_t *) (pkt + sizeof(struct ip6_hdr) + off);

The loop condition tests is_lower_layer(ext->ip6e_nxt) (the next header's type) but the body assigns protocol = ext->ip6e_nxt. So by the time the loop exits, protocol always holds a lower-layer value — the if (!is_upper_layer(protocol)) fallback right after the loop always fires, resets off to 0, and payload lands right after the fixed IPv6 header, on top of whatever extension header is actually there. In other words: master never successfully dispatches a chained-extension IPv6 packet to TCP/UDP today; off is effectively dead code. This is not a regression to fix — it's the reason the PR's naive fix (just changing the loop condition to is_lower_layer(protocol)) looked like a strict improvement while actually introducing three new bugs, below.

Why the straightforward fix is unsafe

Changing the loop condition to is_lower_layer(protocol) makes off live for the first time, which exposes:

1. Wrong unit for the header length. The PR did off += (8 + ext->ip6e_len). Per RFC 8200, Hdr Ext Len is in 8-octet units not counting the first 8 octets, so the correct advance is 8 * (ext->ip6e_len + 1). Any extension header bigger than the minimum 8 bytes lands payload inside the header instead of past it. The PR's added bounds check (off + 8 + ext->ip6e_len > length) inherits the same wrong unit, so it doesn't actually protect against the miscalculation.

2. ESP and AH cannot be walked as struct ip6_ext. is_lower_layer() includes ESP (protocol 50), which has no next-header field at all — the walk would read the SPI's first byte as ip6e_nxt, garbage. AH (protocol 51) does have a next-header field, but its length field is in 4-octet units minus 2, not the struct ip6_ext 8-octet-minus-8 convention, so reusing the same struct/arithmetic misparses it too.

3. Non-first fragments must not be dispatched. Fragment (protocol 44) would now be walked, but a non-first fragment has no L4 header at all — its payload bytes are arbitrary earlier data, and parsing them as a TCP/UDP header creates a bogus session and can emit packets to a random host/port. IPv4 already rejects fragments outright (ip4hdr->frag_off & IP_MF check at ip.c:332-336); IPv6 has no equivalent guard.

Suggested shape for a real fix

• Restrict the walk to the extension header types that are actually struct ip6_ext-shaped and safe to skip: Hop-by-Hop (0), Destination Options (60), Routing (43), and possibly Mobility (135) — using the correct 8 * (ext->ip6e_len + 1) advance.
• Stop the walk (do not dispatch) on ESP (50) or AH (51) — there is no generic, safe way to skip past encrypted/authenticated payloads with the current per-flow model.
• Treat Fragment (44) as a fixed 8-byte header (struct ip6_frag) and only continue/dispatch when it is the first fragment (fragment offset == 0); otherwise bail out the same way IPv4 does for IP_MF/non-zero offset.
• Keep a length check before every dereference of the extension header (bounded by length, using the corrected unit).

Related

• The bounded-but-wrong version was reverted out of Fix memory-safety and hang bugs in native packet engine #751 in favor of leaving master's (already broken but at least inert) behavior in place, plus adding an ihl/doff-style validation guard where relevant elsewhere in ip.c.