From 719b7f439c1c8c76d0573b34daa329061c9ad8a6 Mon Sep 17 00:00:00 2001 From: srdusr <99972264+srdusr@users.noreply.github.com> Date: Fri, 29 May 2026 22:56:00 +0200 Subject: Fix IPv4/IPv6 fragment continuation data being decoded as fake transport headers A real correctness bug, not just missing visibility: a non-first fragment's payload is pure continuation data with no TCP/UDP/ICMP header in it at all, but it was being handed to the transport parsers unconditionally, which could misread arbitrary payload bytes as port numbers, sequence numbers, etc. and print a plausible-looking but entirely fake decode. IPv4 gained identification/more_fragments/fragment_offset fields; a nonzero offset now stops summarize_packet before transport dispatch, reporting the fragment instead. IPv6 expresses fragmentation as an extension header instead, so the fix lives in walk_ipv6_extension_headers(): it already walked past Fragment headers, but never checked the offset before continuing on as if a transport header followed - the same bug, reached through a different path. Fixed with an ESP-style hard stop on a nonzero offset. Caught and fixed a second bug while writing the first fix, before it ever ran: the fragment's Identification field was a loop-local variable, discarded the moment the walk continued past a *first* fragment (offset zero) to keep decoding the real payload underneath -- every later return reported no fragment id even though one applied. Fixed by hoisting it to a variable that persists across iterations, the same category of mistake as an earlier IGMPv3 bug. Fuzzed afterward regardless (fuzz_ipv4, fuzz_ipv6, fuzz_summarize, ~16.8M combined runs) - clean. Live-verified with a real 4000-byte ping to the local gateway over the actual 1500-MTU interface: both directions fragmented into 3 pieces each, the first decoded normally with a fragmentation note, and the continuation fragments correctly showed only fragment metadata, no fake ICMP decode attempted. --- tests/test_ipv6.cpp | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) (limited to 'tests/test_ipv6.cpp') diff --git a/tests/test_ipv6.cpp b/tests/test_ipv6.cpp index 1cce85b..2d75e9a 100644 --- a/tests/test_ipv6.cpp +++ b/tests/test_ipv6.cpp @@ -129,6 +129,43 @@ TEST_CASE("walk_ipv6_extension_headers walks the fixed-size Fragment header") { CHECK(result.payload[0] == 0xCA); } +TEST_CASE("walk_ipv6_extension_headers reports the fragment id even for the first fragment") { + // Same shape as the test above (fragment offset 0 - the first + // fragment), but this time checking that the walk continues on to + // TCP *and* still surfaces the Identification field, which a + // caller needs to correlate this with the fragments that follow. + std::vector payload = {static_cast(kProtoTcp), 0x00, + 0x00, 0x00, 0x00, 0x00, 0x30, 0x39, // id = 0x3039 + 0xCA, 0xFE}; + auto result = walk_ipv6_extension_headers(kNextHeaderFragment, payload); + CHECK_FALSE(result.is_non_first_fragment); + REQUIRE(result.fragment_id.has_value()); + CHECK(*result.fragment_id == 0x3039); + CHECK(result.final_next_header == kProtoTcp); + REQUIRE(result.payload.size() == 2); // walk continued past the fragment header to real payload +} + +TEST_CASE("walk_ipv6_extension_headers stops at a non-first fragment rather than walking into " + "continuation data") { + // Fragment offset field (13 bits, packed into the top of bytes[2:3]) + // set to a nonzero value - 8 in units of 8 bytes, i.e. byte offset + // 64 into the original datagram. offset_res_m = 8 << 3 = 0x0040. + std::vector payload = { + static_cast(kProtoTcp), 0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x2A, + 0xDE, 0xAD, 0xBE, 0xEF, // pure continuation data - NOT a TCP header + }; + auto result = walk_ipv6_extension_headers(kNextHeaderFragment, payload); + CHECK(result.is_non_first_fragment); + REQUIRE(result.fragment_id.has_value()); + CHECK(*result.fragment_id == 0x2A); + // final_next_header still names TCP (that's what the reassembled + // datagram eventually is), but the payload past it is untouched + // continuation data - callers must not decode it as TCP. + CHECK(result.final_next_header == kProtoTcp); + REQUIRE(result.payload.size() == 4); + CHECK(result.payload[0] == 0xDE); +} + TEST_CASE("walk_ipv6_extension_headers applies AH's 4-byte-unit length formula") { // AH: next_header(1)=TCP, payload_len(1)=1 -> total len (1+2)*4=12 bytes. std::vector payload(12, 0); -- cgit v1.2.3