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. --- include/packeteer/net/ipv4.hpp | 11 +++++++++ include/packeteer/net/ipv6.hpp | 53 ++++++++++++++++++++++++++++++++++++----- include/packeteer/summarize.hpp | 28 ++++++++++++++++++++++ 3 files changed, 86 insertions(+), 6 deletions(-) (limited to 'include') diff --git a/include/packeteer/net/ipv4.hpp b/include/packeteer/net/ipv4.hpp index c6cb656..bcc8fa6 100644 --- a/include/packeteer/net/ipv4.hpp +++ b/include/packeteer/net/ipv4.hpp @@ -27,6 +27,13 @@ struct Ipv4Header { std::uint8_t protocol; Ipv4Address src; Ipv4Address dst; + std::uint16_t identification; // ties fragments of the same original datagram together + bool more_fragments; + // In 8-byte units (RFC 791) - 0 means this is either an unfragmented + // packet or the *first* fragment, either way the one that actually + // carries the transport header. Nonzero means a later fragment: + // pure payload continuation, no TCP/UDP/ICMP header present at all. + std::uint16_t fragment_offset; }; struct Ipv4Packet { @@ -46,6 +53,10 @@ inline std::optional parse_ipv4(std::span bytes header.version = version; header.ihl = ihl; header.total_length = read_be16(bytes, 2); + header.identification = read_be16(bytes, 4); + std::uint16_t flags_and_offset = read_be16(bytes, 6); + header.more_fragments = (flags_and_offset & 0x2000) != 0; + header.fragment_offset = flags_and_offset & 0x1FFF; header.ttl = bytes[8]; header.protocol = bytes[9]; std::copy_n(bytes.begin() + 12, 4, header.src.bytes.begin()); diff --git a/include/packeteer/net/ipv6.hpp b/include/packeteer/net/ipv6.hpp index 8c048e8..d7ea0d5 100644 --- a/include/packeteer/net/ipv6.hpp +++ b/include/packeteer/net/ipv6.hpp @@ -70,6 +70,18 @@ struct Ipv6ExtensionWalkResult { std::uint8_t final_next_header; // a transport protocol, or an extension type we stopped at std::span payload; // bytes after every extension header walked bool stopped_at_esp; // true if ESP was hit - see walk_ipv6_extension_headers() + // True when a Fragment header (RFC 8200 4.5) was walked with a + // nonzero fragment offset - a later fragment, not the first. + // final_next_header still correctly names the eventual transport + // protocol, but `payload` here is pure continuation data with no + // TCP/UDP/ICMPv6 header in it at all; callers must not hand it to + // a transport parser. fragment_id is that header's 4-byte + // Identification field, set whenever a Fragment header was seen + // at all (first fragment or not) - useful for correlating + // fragments of the same original datagram even when this flag is + // false for the first one. + bool is_non_first_fragment; + std::optional fragment_id; }; // Walks Hop-by-Hop, Routing, Destination Options, Fragment, and AH @@ -96,35 +108,64 @@ struct Ipv6ExtensionWalkResult { inline Ipv6ExtensionWalkResult walk_ipv6_extension_headers(std::uint8_t next_header, std::span payload) { constexpr int kMaxExtensionHeaders = 8; + // Persists across loop iterations, unlike a Fragment header's other + // fields: once a Fragment header is seen (even the first one, whose + // walk continues normally afterward), every return from here on + // should still report its Identification field, not lose it the + // moment the loop moves past that one header. + std::optional seen_fragment_id; for (int i = 0; i < kMaxExtensionHeaders; ++i) { if (next_header == kNextHeaderEsp) { - return {next_header, payload, /*stopped_at_esp=*/true}; + return {next_header, payload, /*stopped_at_esp=*/true, false, seen_fragment_id}; } std::size_t ext_len; if (next_header == kNextHeaderFragment) { - if (payload.size() < 8) return {next_header, payload, false}; + if (payload.size() < 8) return {next_header, payload, false, false, seen_fragment_id}; ext_len = 8; } else if (next_header == kNextHeaderAh) { - if (payload.size() < 2) return {next_header, payload, false}; + if (payload.size() < 2) return {next_header, payload, false, false, seen_fragment_id}; ext_len = (static_cast(payload[1]) + 2) * 4; } else if (next_header == kNextHeaderHopByHop || next_header == kNextHeaderRouting || next_header == kNextHeaderDestOptions) { - if (payload.size() < 2) return {next_header, payload, false}; + if (payload.size() < 2) return {next_header, payload, false, false, seen_fragment_id}; ext_len = (static_cast(payload[1]) + 1) * 8; } else { break; // TCP/UDP/ICMPv6/anything else we don't chain through: stop here } - if (payload.size() < ext_len) return {next_header, payload, false}; // truncated: stop + if (payload.size() < ext_len) { + return {next_header, payload, false, false, seen_fragment_id}; // truncated: stop + } + + if (next_header == kNextHeaderFragment) { + // Fragment Offset (13 bits) + Res (2 bits) + M flag (1 bit) + // at bytes[2:4]; Identification (4 bytes) at bytes[4:8]. + std::uint16_t offset_res_m = read_be16(payload, 2); + std::uint16_t fragment_offset = (offset_res_m >> 3) & 0x1FFF; + seen_fragment_id = read_be32(payload, 4); + std::uint8_t this_next_header = payload[0]; + std::span remaining = payload.subspan(ext_len); + if (fragment_offset != 0) { + // A later fragment: `remaining` is pure continuation + // data, not a transport header, regardless of what + // this_next_header names. Stop here rather than + // walking (and definitely rather than letting a + // caller hand this to a transport parser). + return {this_next_header, remaining, false, true, seen_fragment_id}; + } + payload = remaining; + next_header = this_next_header; + continue; + } std::uint8_t this_next_header = payload[0]; payload = payload.subspan(ext_len); next_header = this_next_header; } - return {next_header, payload, false}; + return {next_header, payload, false, false, seen_fragment_id}; } // RFC 5952 canonical text form: lowercase hex, and the longest run of diff --git a/include/packeteer/summarize.hpp b/include/packeteer/summarize.hpp index 4126eb5..4bb2d24 100644 --- a/include/packeteer/summarize.hpp +++ b/include/packeteer/summarize.hpp @@ -319,9 +319,27 @@ inline std::string summarize_packet(std::span bytes, int da out += " | IPv4 (truncated)"; return out; } + bool is_fragment = ip->header.more_fragments || ip->header.fragment_offset != 0; + if (ip->header.fragment_offset != 0) { + // A later fragment: no TCP/UDP/ICMP header is present in + // this packet at all, only continuation payload - handing + // it to a transport parser would misread arbitrary payload + // bytes as port numbers/sequence numbers/etc. Report the + // fragment itself and stop, rather than decode garbage. + char buf[96]; + std::snprintf(buf, sizeof(buf), " | IPv4 %s -> %s ttl=%u proto=%u fragment id=%u offset=%u%s", + ipv4_to_string(ip->header.src).c_str(), ipv4_to_string(ip->header.dst).c_str(), + ip->header.ttl, ip->header.protocol, ip->header.identification, + ip->header.fragment_offset * 8, ip->header.more_fragments ? " (more)" : ""); + out += buf; + return out; + } out += summarize_transport_and_above({"IPv4", ipv4_to_string(ip->header.src), ipv4_to_string(ip->header.dst), ip->header.ttl, ip->header.protocol, ip->payload}); + if (is_fragment) { + out += " (fragmented, id=" + std::to_string(ip->header.identification) + ", more follow)"; + } } else if (version == 6) { auto ip6 = net::parse_ipv6(ip_bytes); if (!ip6) { @@ -341,9 +359,19 @@ inline std::string summarize_packet(std::span bytes, int da src_str.c_str(), dst_str.c_str(), ip6->header.hop_limit, net::kNextHeaderEsp); out += buf; + } else if (walked.is_non_first_fragment) { + char buf[128]; + std::snprintf(buf, sizeof(buf), + " | IPv6 %s -> %s ttl=%u proto=%u fragment id=%u (not first)", + src_str.c_str(), dst_str.c_str(), ip6->header.hop_limit, + walked.final_next_header, *walked.fragment_id); + out += buf; } else { out += summarize_transport_and_above({"IPv6", src_str, dst_str, ip6->header.hop_limit, walked.final_next_header, walked.payload}); + if (walked.fragment_id) { + out += " (fragmented, id=" + std::to_string(*walked.fragment_id) + ")"; + } } } else { out += " | IP version " + std::to_string(version) + " (unsupported)"; -- cgit v1.2.3