noi/gopacket

Fork 0

ip4defrag: build() emits a reassembled IPv4 layer whose Length field lies about its own Payload #6

New issue

Open

opened 2026-08-26 09:53:27 +00:00 by claude · 1 comment

claude commented

2026-08-26 09:53:27 +00:00

Collaborator

Severity: medium · ip4defrag/defrag.go:283-320 (fragmentList.build)

build() assembles final from whatever bytes each fragment actually carries, but sets the output header's Length from f.Highest, which was accumulated from the declared ip.Length of each fragment. When any fragment was captured short — snaplen truncation, or a deliberately short frame — the two disagree, and build() returns success.

The caller receives a *layers.IPv4 that looks like a normal, fully reassembled datagram and is internally inconsistent.

Reproduction

Two non-overlapping fragments. The first declares 1480 payload bytes and carries 4:

=== D. Length/Payload divergence laundered into a 'valid' datagram ===
  frag1 -> nil, err=<nil>
  frag2 -> REASSEMBLED: declared Length=1488  len(Payload)=12  payload="AAAACCCCCCCC"

No error, no Truncated flag on the result, no signal of any kind. Length=1488, payload 12 bytes — a 124× overstatement.

Note this needs no overlap at all, so it is not blocked by the fixes for #1 or #2 — it goes through build()'s ordinary contiguous path.

Impact

The truncation is introduced by the capture, which is normal and expected. What ip4defrag does is launder it: an inconsistency that a consumer could have detected on the raw fragment (ip.Length vs len(ip.Payload), plus Truncated metadata) is re-emitted as a fresh, apparently-valid datagram with the metadata gone.

Downstream consequences, in increasing order of severity:

Any byte/volume accounting that trusts Length over-reports by up to two orders of magnitude — cheap traffic-graph poisoning.
Any re-serialisation (writing a filtered pcap, generating a block rule, building a replay snippet) produces a header whose Total_Length does not match the frame.
Any consumer that slices on the declared length — payload[:ip.Length-ip.IHL*4], the single most natural thing to write — panics. gopacket's own ip4defrag is not a Decoder, so nothing on this path is inside NewPacket's recover().

Fix

Set the output length from what was actually assembled, and refuse to return a datagram that does not match its own accounting:

if int(f.Highest) != len(final) {
        return nil, fmt.Errorf("defrag: assembled %d bytes, accounting says %d", len(final), f.Highest)
}
out := &layers.IPv4{
        ...
        Length: uint16(len(final)) + uint16(in.IHL)*4,
        ...
}

The stricter and better fix is the one in #1: reject a fragment whose payload is shorter than it claims in securityChecks(), so a short capture never enters a fragment list at all. With that in place this divergence cannot arise. The assertion above is still worth keeping as a backstop, since it is the invariant build() is supposed to maintain.

Verified against b7d9dbd on Go 1.24.4. PoC: defrag_more, case D.

**Severity: medium** · `ip4defrag/defrag.go:283-320` (`fragmentList.build`) `build()` assembles `final` from whatever bytes each fragment actually carries, but sets the output header's `Length` from `f.Highest`, which was accumulated from the **declared** `ip.Length` of each fragment. When any fragment was captured short — snaplen truncation, or a deliberately short frame — the two disagree, and `build()` returns success. The caller receives a `*layers.IPv4` that looks like a normal, fully reassembled datagram and is internally inconsistent. ## Reproduction Two **non-overlapping** fragments. The first declares 1480 payload bytes and carries 4: ``` === D. Length/Payload divergence laundered into a 'valid' datagram === frag1 -> nil, err=<nil> frag2 -> REASSEMBLED: declared Length=1488 len(Payload)=12 payload="AAAACCCCCCCC" ``` No error, no `Truncated` flag on the result, no signal of any kind. `Length=1488`, payload 12 bytes — a **124× overstatement**. Note this needs no overlap at all, so it is not blocked by the fixes for #1 or #2 — it goes through `build()`'s ordinary contiguous path. ## Impact The truncation is introduced by the capture, which is normal and expected. What `ip4defrag` does is **launder it**: an inconsistency that a consumer could have detected on the raw fragment (`ip.Length` vs `len(ip.Payload)`, plus `Truncated` metadata) is re-emitted as a fresh, apparently-valid datagram with the metadata gone. Downstream consequences, in increasing order of severity: - Any byte/volume accounting that trusts `Length` over-reports by up to two orders of magnitude — cheap traffic-graph poisoning. - Any re-serialisation (writing a filtered pcap, generating a block rule, building a replay snippet) produces a header whose `Total_Length` does not match the frame. - Any consumer that slices on the declared length — `payload[:ip.Length-ip.IHL*4]`, the single most natural thing to write — panics. gopacket's own `ip4defrag` is not a `Decoder`, so nothing on this path is inside `NewPacket`'s `recover()`. ## Fix Set the output length from what was actually assembled, and refuse to return a datagram that does not match its own accounting: ```go if int(f.Highest) != len(final) { return nil, fmt.Errorf("defrag: assembled %d bytes, accounting says %d", len(final), f.Highest) } out := &layers.IPv4{ ... Length: uint16(len(final)) + uint16(in.IHL)*4, ... } ``` The stricter and better fix is the one in #1: reject a fragment whose payload is shorter than it claims in `securityChecks()`, so a short capture never enters a fragment list at all. With that in place this divergence cannot arise. The assertion above is still worth keeping as a backstop, since it is the invariant `build()` is supposed to maintain. --- *Verified against `b7d9dbd` on Go 1.24.4. PoC: `defrag_more`, case D.*

claude added the

labels

2026-08-26 09:53:29 +00:00

claude referenced this issue

2026-08-26 09:53:50 +00:00

ip4defrag: the "fragment will overrun" security check is vacuous — uint16 wraparound in securityChecks #7

claude referenced this issue

2026-08-26 12:29:23 +00:00

ip4defrag: the "fragment will overrun" security check is vacuous — uint16 wraparound in securityChecks #7

claude commented

2026-08-26 12:29:42 +00:00

Author

Collaborator

Independent verification — present in `gopacket/gopacket v1.7.0`; one caller-side mitigation worth documenting

The defect is present verbatim in the maintained fork: build() assembles final from captured bytes but sets the output header's Length from f.Highest, accumulated from each fragment's declared ip.Length. Confirmed by source inspection.

The analysis in this issue is correct, and the "laundering" framing is the right one — the inconsistency is detectable on the raw fragment (ip.Length vs len(ip.Payload), plus Truncated) and is re-emitted with that metadata gone.

A mitigation that happens to work, and why it shouldn't be relied on

The consumer audited alongside these issues turns out to be immune to this one, by accident rather than design. Rather than passing the *layers.IPv4 from DefragIPv4 downstream, it re-serialises the reassembled datagram back into a synthetic frame before decoding:

opts := gopacket.SerializeOptions{FixLengths: true, ComputeChecksums: true}
gopacket.SerializeLayers(buf, opts, ip, payload)

FixLengths: true recomputes Total_Length from the payload actually present, so the lying Length never escapes the defragmentation step. The consumer also derives its byte accounting from len(reassembled) rather than the header field, so the traffic-graph poisoning described here does not land either.

Two things follow:

This is worth adding to the issue as a known workaround for callers who cannot wait for a library fix — re-serialise with FixLengths instead of consuming the returned layer directly. It is a two-line change and it closes the whole class.
It is not a substitute for the fix. It only works because that caller happened to need a re-framed packet for unrelated reasons. A caller that does the natural thing — read out.Length, or slice out.Payload[:out.Length-uint16(out.IHL)*4] — gets the full impact, including the panic this issue predicts in its third bullet. The fix belongs in build().

Concurring on the "no overlap required" point

Confirming this independently, because it affects remediation sequencing: this reaches build()'s ordinary contiguous path, so neither the fix for #1 nor the fix for #2 closes it. Of the four ip4defrag issues, this is the one most likely to be assumed fixed by the others and left open.

Suggested addition to the proposed fix

The proposed if int(f.Highest) != len(final) check is right. Worth pairing it with propagating truncation rather than only rejecting it — if any contributing fragment had Truncated set, the assembled result should carry that flag, so a caller that wants the bytes anyway can still tell they are incomplete. Right now the only options the fix offers are "valid datagram" or "error", and a capture with snaplen truncation is a legitimate, expected input rather than an attack.

Verified against gopacket/gopacket v1.7.0 on Go 1.24.4.

## Independent verification — present in `gopacket/gopacket v1.7.0`; one caller-side mitigation worth documenting The defect is present verbatim in the maintained fork: `build()` assembles `final` from captured bytes but sets the output header's `Length` from `f.Highest`, accumulated from each fragment's *declared* `ip.Length`. Confirmed by source inspection. The analysis in this issue is correct, and the "laundering" framing is the right one — the inconsistency is detectable on the raw fragment (`ip.Length` vs `len(ip.Payload)`, plus `Truncated`) and is re-emitted with that metadata gone. ### A mitigation that happens to work, and why it shouldn't be relied on The consumer audited alongside these issues turns out to be **immune** to this one, by accident rather than design. Rather than passing the `*layers.IPv4` from `DefragIPv4` downstream, it re-serialises the reassembled datagram back into a synthetic frame before decoding: ```go opts := gopacket.SerializeOptions{FixLengths: true, ComputeChecksums: true} gopacket.SerializeLayers(buf, opts, ip, payload) ``` `FixLengths: true` recomputes `Total_Length` from the payload actually present, so the lying `Length` never escapes the defragmentation step. The consumer also derives its byte accounting from `len(reassembled)` rather than the header field, so the traffic-graph poisoning described here does not land either. Two things follow: 1. **This is worth adding to the issue as a known workaround** for callers who cannot wait for a library fix — re-serialise with `FixLengths` instead of consuming the returned layer directly. It is a two-line change and it closes the whole class. 2. **It is not a substitute for the fix.** It only works because that caller happened to need a re-framed packet for unrelated reasons. A caller that does the natural thing — read `out.Length`, or slice `out.Payload[:out.Length-uint16(out.IHL)*4]` — gets the full impact, including the panic this issue predicts in its third bullet. The fix belongs in `build()`. ### Concurring on the "no overlap required" point Confirming this independently, because it affects remediation sequencing: this reaches `build()`'s ordinary contiguous path, so neither the fix for #1 nor the fix for #2 closes it. Of the four ip4defrag issues, this is the one most likely to be assumed fixed by the others and left open. ### Suggested addition to the proposed fix The proposed `if int(f.Highest) != len(final)` check is right. Worth pairing it with propagating truncation rather than only rejecting it — if any contributing fragment had `Truncated` set, the assembled result should carry that flag, so a caller that wants the bytes anyway can still tell they are incomplete. Right now the only options the fix offers are "valid datagram" or "error", and a capture with snaplen truncation is a legitimate, expected input rather than an attack. --- *Verified against `gopacket/gopacket v1.7.0` on Go 1.24.4.*