From 98c2da472dd1608adc4b1c1cd62b85688168ccff Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:30:05 +0200 Subject: [PATCH 1/2] Fix IPv6 extension header walk in handle_ip() 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 --- .github/workflows/test.yml | 7 + app/CMakeLists.txt | 1 + app/src/main/jni/netguard/ip.c | 51 +----- app/src/main/jni/netguard/ip6_ext.c | 136 +++++++++++++++ app/src/main/jni/netguard/ip6_ext.h | 103 ++++++++++++ app/src/main/jni/netguard/netguard.h | 4 - app/src/test/native/ip6_ext_test.c | 243 +++++++++++++++++++++++++++ 7 files changed, 499 insertions(+), 46 deletions(-) create mode 100644 app/src/main/jni/netguard/ip6_ext.c create mode 100644 app/src/main/jni/netguard/ip6_ext.h create mode 100644 app/src/test/native/ip6_ext_test.c diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index de2de996..d59bc137 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -73,6 +73,13 @@ jobs: app/src/test/native/dns_frame_test.c app/src/main/jni/netguard/dns_frame.c /tmp/dns_frame_test + - name: Run IPv6 extension header walk host tests + run: | + 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 + - name: Run unit tests run: ./gradlew testFdroidDebugUnitTest --offline diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index e1d31bf4..284f5855 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -12,6 +12,7 @@ add_library( netguard src/main/jni/netguard/netguard.c src/main/jni/netguard/session.c src/main/jni/netguard/ip.c + src/main/jni/netguard/ip6_ext.c src/main/jni/netguard/policy.c src/main/jni/netguard/tls.c src/main/jni/netguard/tcp.c diff --git a/app/src/main/jni/netguard/ip.c b/app/src/main/jni/netguard/ip.c index ae316d80..1f80b8fb 100644 --- a/app/src/main/jni/netguard/ip.c +++ b/app/src/main/jni/netguard/ip.c @@ -19,6 +19,7 @@ #include "netguard.h" #include "tls.h" +#include "ip6_ext.h" #include int max_tun_msg = 0; @@ -267,27 +268,6 @@ int check_tun(const struct arguments *args, return 0; } -// https://en.wikipedia.org/wiki/IPv6_packet#Extension_headers -// http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml -int is_lower_layer(int protocol) { - // No next header = 59 - return (protocol == 0 || // Hop-by-Hop Options - protocol == 60 || // Destination Options (before routing header) - protocol == 43 || // Routing - protocol == 44 || // Fragment - protocol == 51 || // Authentication Header (AH) - protocol == 50 || // Encapsulating Security Payload (ESP) - protocol == 60 || // Destination Options (before upper-layer header) - protocol == 135); // Mobility -} - -int is_upper_layer(int protocol) { - return (protocol == IPPROTO_TCP || - protocol == IPPROTO_UDP || - protocol == IPPROTO_ICMP || - protocol == IPPROTO_ICMPV6); -} - // SNI extraction disabled by default: connecting to tracker IPs to read TLS // ClientHello leaks the user's IP address to the tracker server. // Can be enabled at runtime via jni_sni() for research purposes. @@ -363,31 +343,18 @@ void handle_ip(const struct arguments *args, struct ip6_hdr *ip6hdr = (struct ip6_hdr *) pkt; - // Skip extension headers - uint16_t off = 0; - protocol = ip6hdr->ip6_nxt; - if (!is_upper_layer(protocol)) { - log_android(ANDROID_LOG_WARN, "IP6 extension %d", 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; - log_android(ANDROID_LOG_WARN, "IP6 extension %d", protocol); - - off += (8 + ext->ip6e_len); - ext = (struct ip6_ext *) (pkt + off); - } - if (!is_upper_layer(protocol)) { - off = 0; - protocol = ip6hdr->ip6_nxt; - log_android(ANDROID_LOG_WARN, "IP6 final extension %d", protocol); - } - } + // Skip extension headers. ip6_skip_ext_headers() (ip6_ext.c) owns the + // walk -- RFC 8200 Hdr Ext Len arithmetic, hard bounds checks against + // `length`, and deciding which header types are walkable -- so it can + // be unit-tested on the host; see ip6_ext.h for the exact contract. + size_t payload_off; + if (!ip6_skip_ext_headers(pkt, length, &protocol, &payload_off)) + log_android(ANDROID_LOG_WARN, "IP6 extension %d not walkable", protocol); saddr = &ip6hdr->ip6_src; daddr = &ip6hdr->ip6_dst; - payload = (uint8_t *) (pkt + sizeof(struct ip6_hdr) + off); + payload = (uint8_t *) (pkt + payload_off); // TODO checksum } else { diff --git a/app/src/main/jni/netguard/ip6_ext.c b/app/src/main/jni/netguard/ip6_ext.c new file mode 100644 index 00000000..4708e57a --- /dev/null +++ b/app/src/main/jni/netguard/ip6_ext.c @@ -0,0 +1,136 @@ +/* + This file is part of NetGuard. + + NetGuard is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + NetGuard is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with NetGuard. If not, see . + + Copyright 2015-2019 by Marcel Bokhorst (M66B) +*/ + +#include "ip6_ext.h" + +#include + +/* + * Extension header "Next Header" values that are not IPPROTO_* upper-layer + * protocols and cannot be walked further, but that this engine still wants + * to name accurately in logs/decisions rather than lump in with a generic + * "unknown". Not all libc's define every one of these (IPPROTO_NONE and + * IPPROTO_DSTOPTS in particular vary), so they are given literal values + * straight from the IANA protocol-numbers registry rather than relying on + * to supply them. + */ +#define IP6_EXT_HOPOPTS 0 +#define IP6_EXT_ROUTING 43 +#define IP6_EXT_FRAGMENT 44 +#define IP6_EXT_ESP 50 +#define IP6_EXT_AH 51 +#define IP6_EXT_DSTOPTS 60 +#define IP6_EXT_NONE 59 /* "No Next Header" */ + +/* + * Defensive cap on the number of extension headers walked for a single + * packet. Legitimate chains are one or two headers long; this exists only + * to bound a maliciously (or corruptly) long chain, not to accommodate + * real traffic -- the `length` bound below already makes the loop + * terminate, but a packet can still carry many minimum-size (8-byte) + * extension headers, so an explicit cap keeps the walk cheap regardless. + */ +#define MAX_IP6_EXT_HEADERS 8 + +static int is_upper_layer_protocol(uint8_t protocol) { + return protocol == IPPROTO_TCP || + protocol == IPPROTO_UDP || + protocol == IPPROTO_ICMP || + protocol == IPPROTO_ICMPV6; +} + +int ip6_skip_ext_headers(const uint8_t *pkt, size_t length, + uint8_t *protocol_out, size_t *payload_off_out) { + if (pkt == NULL || protocol_out == NULL || payload_off_out == NULL) + return 0; + + if (length < IP6_EXT_FIXED_HDR_LEN) { + /* Defensive only: handle_ip() already rejects a packet shorter than + * the fixed IPv6 header before ever calling this. Report a value + * that can never be mistaken for TCP/UDP/ICMP and never point + * payload_off_out past the buffer. */ + *protocol_out = IP6_EXT_NONE; + *payload_off_out = length; + return 0; + } + + /* The fixed IPv6 header's Next Header field is byte 6 (after the 4-byte + * Version/Traffic Class/Flow Label and the 2-byte Payload Length). */ + uint8_t next = pkt[6]; + size_t off = IP6_EXT_FIXED_HDR_LEN; + + for (int hops = 0; ; hops++) { + if (is_upper_layer_protocol(next)) { + *protocol_out = next; + *payload_off_out = off; + return 1; + } + + /* Headers that cannot be walked at all: report and stop right where + * we are, without trying to read a length field that (for Fragment) + * does not mean what it would for the walkable headers, or (for + * ESP/No-Next-Header/anything unrecognised) simply is not there to + * find plaintext structure behind. */ + if (next == IP6_EXT_FRAGMENT || next == IP6_EXT_ESP || next == IP6_EXT_NONE || + !(next == IP6_EXT_HOPOPTS || next == IP6_EXT_ROUTING || + next == IP6_EXT_DSTOPTS || next == IP6_EXT_AH)) { + *protocol_out = next; + *payload_off_out = off; + return 0; + } + + if (hops >= MAX_IP6_EXT_HEADERS) { + /* Chain too long -- bail out where we are rather than keep + * walking an attacker-controlled sequence of headers. */ + *protocol_out = next; + *payload_off_out = off; + return 0; + } + + /* Every walkable header (Hop-by-Hop, Routing, Destination Options, + * AH) starts with a 1-byte Next Header and a 1-byte length field; + * need both in bounds before reading either. */ + if (off + 2 > length) { + *protocol_out = next; + *payload_off_out = off; + return 0; + } + + uint8_t hdr_next = pkt[off]; + uint8_t hdr_len = pkt[off + 1]; + + /* AH's length is in 4-octet units, not including the first 8 + * octets, per RFC 4302 section 2.2: (Hdr Ext Len + 2) * 4. Every + * other walkable header uses RFC 8200's 8-octet units, not + * including the first 8 octets: 8 * (Hdr Ext Len + 1). */ + size_t advance = (next == IP6_EXT_AH) + ? ((size_t) hdr_len + 2) * 4 + : 8 * ((size_t) hdr_len + 1); + + if (advance > length || off > length - advance) { + /* Declared length runs past the end of the buffer. */ + *protocol_out = next; + *payload_off_out = off; + return 0; + } + + off += advance; + next = hdr_next; + } +} diff --git a/app/src/main/jni/netguard/ip6_ext.h b/app/src/main/jni/netguard/ip6_ext.h new file mode 100644 index 00000000..78dcd304 --- /dev/null +++ b/app/src/main/jni/netguard/ip6_ext.h @@ -0,0 +1,103 @@ +/* + This file is part of NetGuard. + + NetGuard is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + NetGuard is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with NetGuard. If not, see . + + Copyright 2015-2019 by Marcel Bokhorst (M66B) +*/ + +#ifndef IP6_EXT_H +#define IP6_EXT_H + +/* + * Pure IPv6 extension header walk, extracted out of handle_ip() (ip.c) so + * it can be unit-tested on the host without pulling in JNI/Android + * dependencies. This header and its implementation (ip6_ext.c) depend only + * on libc ( for the IPPROTO_* constants) -- no netguard.h, + * no JNI, no struct ip6_hdr/ip6_ext from , so the fixed + * header size below is duplicated rather than taken from sizeof(struct + * ip6_hdr) (the two must and do agree: RFC 8200 defines the fixed IPv6 + * header as exactly 40 bytes, with no options). + * + * See https://www.rfc-editor.org/rfc/rfc8200 (section 4) and + * https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml + */ + +#include +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/* Fixed IPv6 header length in bytes (RFC 8200 section 3). */ +#define IP6_EXT_FIXED_HDR_LEN 40 + +/* + * Walks the IPv6 extension header chain that starts right after the fixed + * 40-byte IPv6 header, looking for an upper-layer protocol this engine can + * actually parse (TCP, UDP, ICMP, ICMPv6). + * + * pkt/length: the full IP packet buffer, exactly as received from the + * tun device. length must be >= IP6_EXT_FIXED_HDR_LEN (the caller + * already rejects shorter packets before calling this). + * + * On return, *protocol_out and *payload_off_out are always set, and + * *payload_off_out is always a valid offset into pkt (<= length): + * + * - Return 1: an upper-layer protocol was found. *protocol_out is that + * protocol (TCP/UDP/ICMP/ICMPv6) and *payload_off_out is the offset of + * its header -- exactly what the pre-extraction code intended to + * compute. + * + * - Return 0: no upper-layer protocol was found before the walk had to + * stop. *protocol_out is the extension-header type (or other next- + * header value) the walk stopped on, and *payload_off_out is the + * offset of that header (i.e. right after everything successfully + * walked). *protocol_out in this case is guaranteed not to collide + * with TCP/UDP/ICMP/ICMPv6, so a caller that only special-cases those + * four values downstream (as handle_ip does) treats a stopped walk + * exactly like an unparseable/unknown protocol -- it is never + * misread as a real transport header. Reasons to stop: + * - No Next Header (59): a legitimate, clean end of the chain. + * - Fragment (44): ip6e_len is a reserved field for this header, + * not a length, so it cannot be walked; the packet may also not + * be first-fragment, so there may be no upper-layer header here + * at all. + * - ESP (50): the payload is encrypted; nothing after it is + * parseable in plaintext. + * - An unrecognised/non-walkable next-header value. + * - The declared header length would run past `length`, or there + * are not even enough bytes left to read the 2-byte extension + * header itself (truncated packet). + * - MAX_IP6_EXT_HEADERS extension headers were walked without + * reaching an upper-layer protocol (a defensively small cap + * against a maliciously long or looping chain). + * + * Hop-by-Hop (0), Routing (43) and Destination Options (60) are walked + * using the standard 8-octet-unit encoding: 8 * (Hdr Ext Len + 1) bytes. + * Authentication Header (51) is walked using its own 4-octet-unit + * encoding: (Hdr Ext Len + 2) * 4 bytes (RFC 4302 section 2.2) -- AH only + * authenticates, it does not encrypt, so the headers after it (including + * the real transport header, in AH transport mode) remain in plaintext + * and are worth walking into. + */ +int ip6_skip_ext_headers(const uint8_t *pkt, size_t length, + uint8_t *protocol_out, size_t *payload_off_out); + +#ifdef __cplusplus +} +#endif + +#endif /* IP6_EXT_H */ diff --git a/app/src/main/jni/netguard/netguard.h b/app/src/main/jni/netguard/netguard.h index cf0fe6ce..c0d6d7e1 100644 --- a/app/src/main/jni/netguard/netguard.h +++ b/app/src/main/jni/netguard/netguard.h @@ -419,10 +419,6 @@ void check_tcp_socket(const struct arguments *args, const struct epoll_event *ev, const int epoll_fd); -int is_lower_layer(int protocol); - -int is_upper_layer(int protocol); - void handle_ip(const struct arguments *args, const uint8_t *buffer, size_t length, const int epoll_fd, diff --git a/app/src/test/native/ip6_ext_test.c b/app/src/test/native/ip6_ext_test.c new file mode 100644 index 00000000..71371caa --- /dev/null +++ b/app/src/test/native/ip6_ext_test.c @@ -0,0 +1,243 @@ +/* + * Host unit tests for the IPv6 extension header walk extracted into + * app/src/main/jni/netguard/ip6_ext.{h,c}. + * + * Plain C test program with a tiny assert-based harness (no test framework + * dependency), so it builds and runs with the system compiler on any host + * -- see .github/workflows/test.yml. Mirrors app/src/test/native/dns_frame_test.c. + * + * Background: the original inline walk in handle_ip() (ip.c) had several + * compounding defects -- wrong Hdr Ext Len arithmetic (treated as octets + * instead of 8-octet units), no bounds checking against the packet length, + * a loop-termination bug that meant the walk essentially never reached an + * upper-layer protocol at all (see the "negative control" comment below), + * and a handful of header types (Fragment, ESP, AH, Mobility) that either + * cannot be walked the same way or cannot be walked at all. These tests + * pin down the fixed behaviour so none of that can silently regress. + */ + +#include +#include +#include +#include + +#include "ip6_ext.h" + +static int failures = 0; + +#define CHECK(cond, msg) \ + do { \ + if (!(cond)) { \ + fprintf(stderr, "FAIL: %s (%s:%d)\n", (msg), __FILE__, __LINE__); \ + failures++; \ + } \ + } while (0) + +#define IPPROTO_TCP_ 6 +#define IPPROTO_UDP_ 17 + +/* Fixed 40-byte IPv6 header (RFC 8200 section 3): only byte 6 (Next Header) + * matters to ip6_skip_ext_headers(), the rest is zeroed. */ +static void set_ip6_next(uint8_t *pkt, uint8_t next) { + memset(pkt, 0, IP6_EXT_FIXED_HDR_LEN); + pkt[6] = next; +} + +/* Writes a minimal walkable extension header (Hop-by-Hop/Routing/Destination + * Options/AH all share this 2-byte-prefix shape) at `off`: byte 0 is its + * Next Header, byte 1 is Hdr Ext Len. The rest of the header (its actual + * options/data, sized by the caller's chosen advance) is left zeroed. */ +static void set_ext_header(uint8_t *pkt, size_t off, uint8_t next, uint8_t hdr_len) { + pkt[off] = next; + pkt[off + 1] = hdr_len; +} + +/* 1. Plain IPv6 packet, no extension headers -- the common case. */ +static void test_no_extension_headers(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 20]; + set_ip6_next(pkt, IPPROTO_TCP_); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 1, "no extension headers: succeeds"); + CHECK(protocol == IPPROTO_TCP_, "no extension headers: protocol is TCP"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN, "no extension headers: payload right after fixed header"); +} + +/* 2. Single Hop-by-Hop header (Hdr Ext Len 0 -> 8 bytes), then TCP. */ +static void test_single_hopbyhop_then_tcp(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 8 + 20]; + set_ip6_next(pkt, 0 /* Hop-by-Hop */); + set_ext_header(pkt, IP6_EXT_FIXED_HDR_LEN, IPPROTO_TCP_, 0); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 1, "single hop-by-hop: succeeds"); + CHECK(protocol == IPPROTO_TCP_, "single hop-by-hop: protocol is TCP"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN + 8, "single hop-by-hop: payload after the 8-byte header"); +} + +/* 3. Chain of three headers: Hop-by-Hop -> Routing -> Destination Options -> TCP. */ +static void test_chain_of_three_headers(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 8 + 8 + 8 + 20]; + set_ip6_next(pkt, 0 /* Hop-by-Hop */); + size_t off = IP6_EXT_FIXED_HDR_LEN; + set_ext_header(pkt, off, 43 /* Routing */, 0); + off += 8; + set_ext_header(pkt, off, 60 /* Destination Options */, 0); + off += 8; + set_ext_header(pkt, off, IPPROTO_TCP_, 0); + off += 8; + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 1, "chain of three: succeeds"); + CHECK(protocol == IPPROTO_TCP_, "chain of three: protocol is TCP"); + CHECK(payload_off == off, "chain of three: payload after all three headers"); +} + +/* 4. AH uses 4-octet units, not 8-octet units: (Hdr Ext Len + 2) * 4. */ +static void test_ah_then_udp(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 12 + 8]; + set_ip6_next(pkt, 51 /* AH */); + set_ext_header(pkt, IP6_EXT_FIXED_HDR_LEN, IPPROTO_UDP_, 1); // (1+2)*4 = 12 bytes + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 1, "AH then UDP: succeeds"); + CHECK(protocol == IPPROTO_UDP_, "AH then UDP: protocol is UDP"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN + 12, "AH then UDP: payload after AH's 4-octet-unit length"); +} + +/* 5. Declared header length runs past the end of the buffer: stop cleanly, + * do not read out of bounds, and report the header we couldn't get past. */ +static void test_declared_length_past_end(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 8]; // only room for the header itself + set_ip6_next(pkt, 0 /* Hop-by-Hop */); + set_ext_header(pkt, IP6_EXT_FIXED_HDR_LEN, IPPROTO_TCP_, 250); // 8*(250+1) way past the buffer + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 0, "declared length past end: stops (not a false success)"); + CHECK(protocol == 0, "declared length past end: reports the Hop-by-Hop header it stopped on"); + CHECK(payload_off <= sizeof(pkt), "declared length past end: payload_off never exceeds the buffer"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN, "declared length past end: payload_off at the unwalked header"); +} + +/* 6. Truncated packet: a header start (1 byte) but no room for its own + * 2-byte prefix, let alone a body. Must not read past the buffer. */ +static void test_truncated_header_start(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 1]; + set_ip6_next(pkt, 0 /* Hop-by-Hop */); + pkt[IP6_EXT_FIXED_HDR_LEN] = IPPROTO_TCP_; // only 1 byte available, no Hdr Ext Len byte + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 0, "truncated header start: stops"); + CHECK(protocol == 0, "truncated header start: reports the Hop-by-Hop header it stopped on"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN, "truncated header start: payload_off at the unreadable header"); +} + +/* 7. Fragment header: ip6e_len is a reserved field here, not a length -- + * must not be walked, regardless of whatever garbage sits in that byte. */ +static void test_fragment_stops(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 8 + 20]; + set_ip6_next(pkt, 44 /* Fragment */); + // Whatever "Hdr Ext Len" would say for Fragment must be ignored. + set_ext_header(pkt, IP6_EXT_FIXED_HDR_LEN, IPPROTO_TCP_, 0); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 0, "fragment: stops, not walked"); + CHECK(protocol == 44, "fragment: reports Fragment"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN, "fragment: payload_off right after the fixed header"); +} + +/* 8. ESP: payload is encrypted, nothing after it is parseable. */ +static void test_esp_stops(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 20]; + set_ip6_next(pkt, 50 /* ESP */); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 0, "ESP: stops, not walked"); + CHECK(protocol == 50, "ESP: reports ESP"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN, "ESP: payload_off right after the fixed header"); +} + +/* 9. No Next Header (59): a clean, legitimate end of the chain. */ +static void test_no_next_header_stops(void) { + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 20]; + set_ip6_next(pkt, 59 /* No Next Header */); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 0, "no next header: stops cleanly"); + CHECK(protocol == 59, "no next header: reports 59"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN, "no next header: payload_off right after the fixed header"); +} + +/* 10. A chain long enough to hit MAX_IP6_EXT_HEADERS (8): a real TCP header + * sits right where the 9th extension header would be, so if the cap did + * not fire this would incorrectly report success with protocol == TCP. */ +static void test_cap_stops_long_chain(void) { + enum { walked = 8 }; // must match MAX_IP6_EXT_HEADERS in ip6_ext.c + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 8 * (walked + 1) + 20]; + set_ip6_next(pkt, 0 /* Hop-by-Hop */); + + size_t off = IP6_EXT_FIXED_HDR_LEN; + for (int i = 0; i < walked; i++) { + set_ext_header(pkt, off, 0 /* another Hop-by-Hop */, 0); + off += 8; + } + // The 9th header the walk must never reach: if it were read, it would + // hand the walk straight to TCP. + set_ext_header(pkt, off, IPPROTO_TCP_, 0); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 0, "long chain: cap stops the walk before reaching TCP"); + CHECK(protocol == 0, "long chain: reports Hop-by-Hop, not TCP"); + CHECK(payload_off == off, "long chain: payload_off right before the unreached 9th header"); +} + +int main(void) { + test_no_extension_headers(); + test_single_hopbyhop_then_tcp(); + test_chain_of_three_headers(); + test_ah_then_udp(); + test_declared_length_past_end(); + test_truncated_header_start(); + test_fragment_stops(); + test_esp_stops(); + test_no_next_header_stops(); + test_cap_stops_long_chain(); + + if (failures == 0) { + printf("ip6_ext_test: all tests passed\n"); + return 0; + } + + fprintf(stderr, "ip6_ext_test: %d assertion(s) failed\n", failures); + return 1; +} From 010524a23d101dac8b95cb59e0562c56b592c77a Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:32:28 +0200 Subject: [PATCH 2/2] Test the extension-header length arithmetic with Hdr Ext Len > 0 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 --- app/src/test/native/ip6_ext_test.c | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/app/src/test/native/ip6_ext_test.c b/app/src/test/native/ip6_ext_test.c index 71371caa..d4060225 100644 --- a/app/src/test/native/ip6_ext_test.c +++ b/app/src/test/native/ip6_ext_test.c @@ -102,6 +102,47 @@ static void test_chain_of_three_headers(void) { CHECK(payload_off == off, "chain of three: payload after all three headers"); } +/* 3b. A walkable header carrying options (Hdr Ext Len > 0). This is the case + * that discriminates RFC 8200's 8 * (Hdr Ext Len + 1) from the original + * 8 + Hdr Ext Len: at Hdr Ext Len 0 the two agree, so a chain built only from + * minimum-size headers passes under either formula and proves nothing about + * the arithmetic this fix is mainly about. */ +static void test_hopbyhop_with_options_then_tcp(void) { + /* Hdr Ext Len 1 -> 8 * (1 + 1) = 16 bytes; the buggy form gives 9. */ + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 16 + 20]; + set_ip6_next(pkt, 0 /* Hop-by-Hop */); + memset(pkt + IP6_EXT_FIXED_HDR_LEN, 0, 16); + set_ext_header(pkt, IP6_EXT_FIXED_HDR_LEN, IPPROTO_TCP_, 1); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 1, "hop-by-hop with options: succeeds"); + CHECK(protocol == IPPROTO_TCP_, "hop-by-hop with options: protocol is TCP"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN + 16, + "hop-by-hop with options: advance is 8 * (len + 1), not 8 + len"); +} + +/* 3c. The same discrimination on a longer header of a different type, so the + * arithmetic is pinned rather than fitted to one case. */ +static void test_routing_with_options_then_udp(void) { + /* Hdr Ext Len 3 -> 8 * (3 + 1) = 32 bytes; the buggy form gives 11. */ + uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 32 + 8]; + set_ip6_next(pkt, 43 /* Routing */); + memset(pkt + IP6_EXT_FIXED_HDR_LEN, 0, 32); + set_ext_header(pkt, IP6_EXT_FIXED_HDR_LEN, IPPROTO_UDP_, 3); + + uint8_t protocol; + size_t payload_off; + int ok = ip6_skip_ext_headers(pkt, sizeof(pkt), &protocol, &payload_off); + + CHECK(ok == 1, "routing with options: succeeds"); + CHECK(protocol == IPPROTO_UDP_, "routing with options: protocol is UDP"); + CHECK(payload_off == IP6_EXT_FIXED_HDR_LEN + 32, + "routing with options: advance is 8 * (len + 1), not 8 + len"); +} + /* 4. AH uses 4-octet units, not 8-octet units: (Hdr Ext Len + 2) * 4. */ static void test_ah_then_udp(void) { uint8_t pkt[IP6_EXT_FIXED_HDR_LEN + 12 + 8]; @@ -225,6 +266,8 @@ int main(void) { test_no_extension_headers(); test_single_hopbyhop_then_tcp(); test_chain_of_three_headers(); + test_hopbyhop_with_options_then_tcp(); + test_routing_with_options_then_udp(); test_ah_then_udp(); test_declared_length_past_end(); test_truncated_header_start();