ip4defrag: overlapping fragment sets never reassemble — broken currentOffset arithmetic plus a silent fragment drop #2

Open
opened 2026-08-26 09:51:38 +00:00 by claude · 3 comments
Collaborator

Severity: high · ip4defrag/defrag.go:298 (build), ip4defrag/defrag.go:220-248 (insert)

ip4defrag advertises BSD-Right overlap handling (insert's doc comment: "we are inserting fragment based on their offset, latest first. This is sometimes called BSD-Right"). It does not implement it. Two separate defects mean an overlapping fragment set is always discarded, and the fragment list is left poisoned so the datagram can never complete.

For a monitoring tool this is an availability bug with an offensive use: one extra small fragment injected into a fragmented flow makes the analyser lose the entire datagram, silently, with a benign-looking error rather than a signal.

Defect 1 — the overlap branch advances currentOffset by the wrong quantity

} else if frag.FragOffset*8 < currentOffset {
        startAt := currentOffset - frag.FragOffset*8
        ...
        final = append(final, frag.Payload[startAt:]...)
        currentOffset = currentOffset + frag.FragOffset*8   // <-- wrong
}

Compare the non-overlapping branch four lines above, which is correct:

final = append(final, frag.Payload...)
currentOffset = currentOffset + frag.Length - 20

After splicing an overlapping fragment, currentOffset must become that fragment's end, i.e. frag.FragOffset*8 + (frag.Length - 20). Instead it becomes currentOffset + frag.FragOffset*8 — the running offset plus the fragment's start, which is not a meaningful quantity in any coordinate system. The two coincide only when currentOffset == frag.Length-20, by accident.

Every subsequent fragment is then compared against a bogus currentOffset and reported as a hole.

Defect 2 — insert() silently drops a fragment while still counting it

if fragOffset >= f.Highest {
        f.List.PushBack(in)
} else {
        for e := f.List.Front(); e != nil; e = e.Next() {
                frag, _ := e.Value.(*layers.IPv4)
                if in.FragOffset == frag.FragOffset { return nil, nil }   // duplicate
                if in.FragOffset < frag.FragOffset {
                        f.List.InsertBefore(in, e)
                        break                                             // inserted
                }
        }
}
// ... unconditionally:
f.Current = f.Current + fragLength
if f.Highest < fragOffset+fragLength { f.Highest = fragOffset + fragLength }

If fragOffset < f.Highest but the fragment's offset is larger than every offset already in the list, the loop runs off the end without inserting anything — and then Current and Highest are updated as though it had been. The fragment's bytes are counted but do not exist. Current is now permanently inflated relative to what the list can tile, so f.Highest == f.Current can only ever be satisfied by a set that does not actually tile, which build() then rejects as a hole. The list is never freed except by an explicit DiscardOlderThan.

This also makes fragment handling order-dependent in a way nothing documents: the same three fragments reassemble or do not depending purely on arrival order.

Reproduction

An ordinary overlapping set — A=[0,24), B=[8,40), C=[56,64), all fully formed, no truncation, all IHL=5:

=== B. overlap arithmetic: real (untruncated) overlapping set ===
  frag1 -> nil, err=<nil>
  frag2 -> nil, err=<nil>
  frag3 -> nil, err=defrag: building - hole found

Trace: A sets currentOffset = 24. B at byte 8 takes the overlap branch and sets currentOffset = 24 + 8 = 32 instead of 8 + 32 = 40. C at byte 56 is then > 32 → "hole found".

The silent drop (A=[0,16), X=[8,16), C=[16,24), sent in that order):

=== C. silent fragment drop (counted, never stored) ===
  frag1 -> nil, err=<nil>
  frag2 -> nil, err=<nil>     <-- X counted in Current/Highest, never in the list
  frag3 -> nil, err=<nil>     <-- accounting can no longer balance; never completes

Impact

  • Analyser blinding. A rival appends one small overlapping fragment to a fragmented exchange — theirs or, since (SrcIP, DstIP, Id, Protocol) are all readable off the wire, someone else's — and the whole datagram vanishes from the analyser's view. It costs one packet and needs no host to accept it.
  • Order dependence. Reordering on a real network is enough to make legitimate fragmented traffic fail to reassemble, non-deterministically.
  • State retention. Every poisoned list is retained until an explicit DiscardOlderThan; see #4.
  • This is also the reason issue #1's PoC has a send-order requirement: the crafted overlapping fragment must arrive after a higher-offset fragment or defect 2 swallows it before build() can be reached.

Fix

// build(), overlap branch
currentOffset = frag.FragOffset*8 + frag.Length - uint16(frag.IHL)*4

and in insert(), make the fall-through case explicit rather than silent:

inserted := false
for e := f.List.Front(); e != nil; e = e.Next() {
        frag, _ := e.Value.(*layers.IPv4)
        if in.FragOffset == frag.FragOffset {
                return nil, nil
        }
        if in.FragOffset < frag.FragOffset {
                f.List.InsertBefore(in, e)
                inserted = true
                break
        }
}
if !inserted {
        f.List.PushBack(in)   // it belongs at the end; counting it is now honest
}

Both belong under a test that asserts a known-good overlapping set reassembles to the BSD-Right result the doc comment promises — there is currently no such test.


Verified against b7d9dbd on Go 1.24.4. PoC: defrag_more.

**Severity: high** · `ip4defrag/defrag.go:298` (`build`), `ip4defrag/defrag.go:220-248` (`insert`) `ip4defrag` advertises BSD-Right overlap handling (`insert`'s doc comment: *"we are inserting fragment based on their offset, latest first. This is sometimes called BSD-Right"*). It does not implement it. Two separate defects mean an overlapping fragment set is **always** discarded, and the fragment list is left poisoned so the datagram can never complete. For a monitoring tool this is an availability bug with an offensive use: one extra small fragment injected into a fragmented flow makes the analyser lose the entire datagram, silently, with a benign-looking `error` rather than a signal. ## Defect 1 — the overlap branch advances `currentOffset` by the wrong quantity ```go } else if frag.FragOffset*8 < currentOffset { startAt := currentOffset - frag.FragOffset*8 ... final = append(final, frag.Payload[startAt:]...) currentOffset = currentOffset + frag.FragOffset*8 // <-- wrong } ``` Compare the non-overlapping branch four lines above, which is correct: ```go final = append(final, frag.Payload...) currentOffset = currentOffset + frag.Length - 20 ``` After splicing an overlapping fragment, `currentOffset` must become that fragment's **end**, i.e. `frag.FragOffset*8 + (frag.Length - 20)`. Instead it becomes `currentOffset + frag.FragOffset*8` — the running offset plus the fragment's *start*, which is not a meaningful quantity in any coordinate system. The two coincide only when `currentOffset == frag.Length-20`, by accident. Every subsequent fragment is then compared against a bogus `currentOffset` and reported as a hole. ## Defect 2 — `insert()` silently drops a fragment while still counting it ```go if fragOffset >= f.Highest { f.List.PushBack(in) } else { for e := f.List.Front(); e != nil; e = e.Next() { frag, _ := e.Value.(*layers.IPv4) if in.FragOffset == frag.FragOffset { return nil, nil } // duplicate if in.FragOffset < frag.FragOffset { f.List.InsertBefore(in, e) break // inserted } } } // ... unconditionally: f.Current = f.Current + fragLength if f.Highest < fragOffset+fragLength { f.Highest = fragOffset + fragLength } ``` If `fragOffset < f.Highest` but the fragment's offset is larger than every offset already in the list, the loop runs off the end **without inserting anything** — and then `Current` and `Highest` are updated as though it had been. The fragment's bytes are counted but do not exist. `Current` is now permanently inflated relative to what the list can tile, so `f.Highest == f.Current` can only ever be satisfied by a set that does not actually tile, which `build()` then rejects as a hole. The list is never freed except by an explicit `DiscardOlderThan`. This also makes fragment handling **order-dependent** in a way nothing documents: the same three fragments reassemble or do not depending purely on arrival order. ## Reproduction An ordinary overlapping set — `A=[0,24)`, `B=[8,40)`, `C=[56,64)`, all fully formed, no truncation, all `IHL=5`: ``` === B. overlap arithmetic: real (untruncated) overlapping set === frag1 -> nil, err=<nil> frag2 -> nil, err=<nil> frag3 -> nil, err=defrag: building - hole found ``` Trace: `A` sets `currentOffset = 24`. `B` at byte 8 takes the overlap branch and sets `currentOffset = 24 + 8 = 32` instead of `8 + 32 = 40`. `C` at byte 56 is then `> 32` → "hole found". The silent drop (`A=[0,16)`, `X=[8,16)`, `C=[16,24)`, sent in that order): ``` === C. silent fragment drop (counted, never stored) === frag1 -> nil, err=<nil> frag2 -> nil, err=<nil> <-- X counted in Current/Highest, never in the list frag3 -> nil, err=<nil> <-- accounting can no longer balance; never completes ``` ## Impact - **Analyser blinding.** A rival appends one small overlapping fragment to a fragmented exchange — theirs or, since `(SrcIP, DstIP, Id, Protocol)` are all readable off the wire, someone else's — and the whole datagram vanishes from the analyser's view. It costs one packet and needs no host to accept it. - **Order dependence.** Reordering on a real network is enough to make legitimate fragmented traffic fail to reassemble, non-deterministically. - **State retention.** Every poisoned list is retained until an explicit `DiscardOlderThan`; see #4. - This is also the reason issue #1's PoC has a send-order requirement: the crafted overlapping fragment must arrive *after* a higher-offset fragment or defect 2 swallows it before `build()` can be reached. ## Fix ```go // build(), overlap branch currentOffset = frag.FragOffset*8 + frag.Length - uint16(frag.IHL)*4 ``` and in `insert()`, make the fall-through case explicit rather than silent: ```go inserted := false for e := f.List.Front(); e != nil; e = e.Next() { frag, _ := e.Value.(*layers.IPv4) if in.FragOffset == frag.FragOffset { return nil, nil } if in.FragOffset < frag.FragOffset { f.List.InsertBefore(in, e) inserted = true break } } if !inserted { f.List.PushBack(in) // it belongs at the end; counting it is now honest } ``` Both belong under a test that asserts a known-good overlapping set reassembles to the BSD-Right result the doc comment promises — there is currently no such test. --- *Verified against `b7d9dbd` on Go 1.24.4. PoC: `defrag_more`.*
Owner

Can this be exploited? Is there a way to reproduce this issue over the wire? Like someone would send the malicious packets to a service that's being dumped, then Daisy would parse it with gopacket. If not, close the issue.

Can this be exploited? Is there a way to reproduce this issue over the wire? Like someone would send the malicious packets to a service that's being dumped, then Daisy would parse it with gopacket. If not, close the issue.
Author
Collaborator

Correction to the reproduction in the issue body, and a third defect it missed.

While writing the fix I found my example was badly chosen. The set I used — A=[0,24), B=[8,40), C=[56,64) — has a genuine hole at [40,56), so "hole found" was the correct answer for it. It did not demonstrate the defect. The accounting happened to balance by coincidence, which is what let it reach the overlap branch at all.

A set that genuinely tiles tells a much worse story. A=[0,24), B=[8,40), C=[40,48) covers [0,48) contiguously with an overlap at [8,24), and every real IP stack reassembles it:

A=[0,24) B=[8,40) C=[40,48)  -- tiles [0,48) with an overlap at [8,24)
expected reassembly: 48 bytes

  order A,B,C  -> never completed (no error, no datagram)
  order A,C,B  -> never completed (no error, no datagram)
  order B,A,C  -> never completed (no error, no datagram)
  order C,B,A  -> never completed (no error, no datagram)
  order C,A,B  -> never completed (no error, no datagram)
  order B,C,A  -> never completed (no error, no datagram)

No arrival order works, and there is no error — the caller gets nil, nil forever and the fragment list is retained until an explicit DiscardOlderThan.

The root cause is upstream of both defects in the issue body

f.Current = f.Current + fragLength
...
if f.FinalReceived && f.Highest == f.Current {
        return f.build(in)
}

f.Current is a sum of fragment lengths. An overlapping set counts the shared bytes once per fragment that carries them, so Current overshoots Highest permanently and the equality can never hold. For the set above: Current = 24+32+8 = 64, Highest = 48.

So build() is never even called, which means the currentOffset arithmetic defect described in the issue body is real but normally unreachable — it only executes when the sum balances by accident, as in my original example. Same for the silent-drop defect: it makes things worse, but it is not what blocks the common case.

Ranking the three, most to least important:

  1. The build trigger is arithmetically incapable of firing for an overlapping set. (Not in the original issue body.)
  2. The currentOffset advance in the overlap branch is wrong — reached once (1) is fixed.
  3. insert() silently drops a fragment that belongs after every stored element while still counting it.

Fixed in fix/issue-2-ip4defrag-overlap

  • Trigger on f.Current >= f.Highest and let build() decide whether the fragments tile; a hole becomes "not complete yet" (nil, nil) rather than a permanent error, since more fragments may still arrive.
  • currentOffset = frag.FragOffset*8 + frag.Length - uint16(frag.IHL)*4 — advance to the fragment's end.
  • Append, rather than drop, a fragment that sorts after every stored element.

After the fix, all six orders produce the same 48 bytes with first-fragment-wins resolution:

  order A,B,C  -> REASSEMBLED 48 bytes: "AAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBCCCCCCCC"
  ... (identical for all six)

Covered by TestDefragOverlapping (all six orders) and TestDefragFragmentPastListTail. Both fail on main.

One consequence worth flagging for review: fixing this means overlapping sets now do reassemble, so the first-wins resolution policy becomes security-relevant. Linux ≥ 4.19 drops overlapping IPv4 fragments outright rather than resolving them. If matching Linux is the goal, rejecting the datagram may be the better behaviour than reassembling it first-wins — that is a policy call, not a bug fix, so I did not make it here. Worth deciding before merging.

PoC: ovl2 on branch pentest/2026-08-poc.

**Correction to the reproduction in the issue body, and a third defect it missed.** While writing the fix I found my example was badly chosen. The set I used — `A=[0,24)`, `B=[8,40)`, `C=[56,64)` — has a genuine hole at `[40,56)`, so "hole found" was the *correct* answer for it. It did not demonstrate the defect. The accounting happened to balance by coincidence, which is what let it reach the overlap branch at all. A set that genuinely tiles tells a much worse story. `A=[0,24)`, `B=[8,40)`, `C=[40,48)` covers `[0,48)` contiguously with an overlap at `[8,24)`, and every real IP stack reassembles it: ``` A=[0,24) B=[8,40) C=[40,48) -- tiles [0,48) with an overlap at [8,24) expected reassembly: 48 bytes order A,B,C -> never completed (no error, no datagram) order A,C,B -> never completed (no error, no datagram) order B,A,C -> never completed (no error, no datagram) order C,B,A -> never completed (no error, no datagram) order C,A,B -> never completed (no error, no datagram) order B,C,A -> never completed (no error, no datagram) ``` **No arrival order works, and there is no error** — the caller gets `nil, nil` forever and the fragment list is retained until an explicit `DiscardOlderThan`. ### The root cause is upstream of both defects in the issue body ```go f.Current = f.Current + fragLength ... if f.FinalReceived && f.Highest == f.Current { return f.build(in) } ``` `f.Current` is a **sum of fragment lengths**. An overlapping set counts the shared bytes once per fragment that carries them, so `Current` overshoots `Highest` permanently and the equality can never hold. For the set above: `Current = 24+32+8 = 64`, `Highest = 48`. So `build()` is never even called, which means the `currentOffset` arithmetic defect described in the issue body is real but normally **unreachable** — it only executes when the sum balances by accident, as in my original example. Same for the silent-drop defect: it makes things worse, but it is not what blocks the common case. Ranking the three, most to least important: 1. **The build trigger is arithmetically incapable of firing for an overlapping set.** (Not in the original issue body.) 2. The `currentOffset` advance in the overlap branch is wrong — reached once (1) is fixed. 3. `insert()` silently drops a fragment that belongs after every stored element while still counting it. ### Fixed in `fix/issue-2-ip4defrag-overlap` - Trigger on `f.Current >= f.Highest` and let `build()` decide whether the fragments tile; a hole becomes "not complete yet" (`nil, nil`) rather than a permanent error, since more fragments may still arrive. - `currentOffset = frag.FragOffset*8 + frag.Length - uint16(frag.IHL)*4` — advance to the fragment's end. - Append, rather than drop, a fragment that sorts after every stored element. After the fix, all six orders produce the same 48 bytes with first-fragment-wins resolution: ``` order A,B,C -> REASSEMBLED 48 bytes: "AAAAAAAAAAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBCCCCCCCC" ... (identical for all six) ``` Covered by `TestDefragOverlapping` (all six orders) and `TestDefragFragmentPastListTail`. Both fail on `main`. One consequence worth flagging for review: fixing this means overlapping sets now **do** reassemble, so the first-wins resolution policy becomes security-relevant. Linux ≥ 4.19 drops overlapping IPv4 fragments outright rather than resolving them. If matching Linux is the goal, rejecting the datagram may be the better behaviour than reassembling it first-wins — that is a policy call, not a bug fix, so I did not make it here. Worth deciding before merging. *PoC: `ovl2` on branch `pentest/2026-08-poc`.*
Author
Collaborator

Independent verification — both defects present in gopacket/gopacket v1.7.0

Confirmed by reading the fork's source directly. Both defects are verbatim:

// build(), overlap branch — advances by the fragment's START, not its end
currentOffset = currentOffset + frag.FragOffset*8

// insert() — the loop can fall through without inserting, but the
// counters below are updated unconditionally
f.Current = f.Current + fragLength

The send-order dependency this issue describes is confirmed from the other direction too: reproducing #1's root cause B requires the higher-offset fragment C to arrive before the crafted overlapping fragment B, exactly as predicted here. Send them in offset order and B hits the silent-drop path and build() is never reached. That interaction is worth keeping cross-referenced — it is the difference between #1 reproducing and not.

A third path to "never reassembles", distinct from both defects here

While verifying #1 I found a way to make a fragment set permanently un-reassemblable that involves neither the overlap branch nor the silent drop, and needs no overlapping fragment at all.

insert() computes fragLength := in.Length - 20 while securityChecks() uses ip.Length - uint16(ip.IHL)*4. Put IP options on any non-last fragment and Current and Highest are computed on different bases, so they can never converge — build() is simply never called:

=== IP options on a non-last fragment defeat reassembly ===
  first fragment IHL=5 (0 opt bytes) REASSEMBLED len(Payload)=32
  first fragment IHL=6 (4 opt bytes) NEVER REASSEMBLED
  first fragment IHL=7 (8 opt bytes) NEVER REASSEMBLED
  first fragment IHL=8 (12 opt bytes) NEVER REASSEMBLED

Two ordinary contiguous fragments, no overlap, every field RFC-valid, 4 bytes of Router Alert. A real stack reassembles it without complaint.

This matters for the fix proposed here: correcting the currentOffset arithmetic and making the fall-through explicit does not close it, because the set never reaches build() in the first place. It needs the insert() length fix from #1:

fragLength := in.Length - uint16(in.IHL)*4

Suggest the test this issue asks for ("a known-good overlapping set reassembles to the BSD-Right result") be joined by one asserting that a contiguous set with IHL > 5 on a non-last fragment reassembles too. That case has no test today and is the cheapest of the three to exploit.

On the impact framing

A rival appends one small overlapping fragment to a fragmented exchange […] and the whole datagram vanishes from the analyser's view.

Confirmed in principle, with one practical caveat from live testing: on a path where a middlebox reassembles (a Linux bridge with bridge-nf-call-iptables=1, or any conntracking hop), an overlapping set is normalised or dropped before it reaches a downstream capture, so the injected fragment never arrives. The IP-options variant above does not have that limitation — Router Alert's copy-on-fragment bit is set, so the option survives a reassemble/re-fragment cycle and the evasion works through the middlebox. Worth noting when assessing reachability.


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

## Independent verification — both defects present in `gopacket/gopacket v1.7.0` Confirmed by reading the fork's source directly. Both defects are verbatim: ```go // build(), overlap branch — advances by the fragment's START, not its end currentOffset = currentOffset + frag.FragOffset*8 // insert() — the loop can fall through without inserting, but the // counters below are updated unconditionally f.Current = f.Current + fragLength ``` The send-order dependency this issue describes is confirmed from the other direction too: reproducing #1's root cause B **requires** the higher-offset fragment C to arrive before the crafted overlapping fragment B, exactly as predicted here. Send them in offset order and B hits the silent-drop path and `build()` is never reached. That interaction is worth keeping cross-referenced — it is the difference between #1 reproducing and not. ### A third path to "never reassembles", distinct from both defects here While verifying #1 I found a way to make a fragment set permanently un-reassemblable that involves **neither** the overlap branch nor the silent drop, and needs no overlapping fragment at all. `insert()` computes `fragLength := in.Length - 20` while `securityChecks()` uses `ip.Length - uint16(ip.IHL)*4`. Put IP options on any **non-last** fragment and `Current` and `Highest` are computed on different bases, so they can never converge — `build()` is simply never called: ``` === IP options on a non-last fragment defeat reassembly === first fragment IHL=5 (0 opt bytes) REASSEMBLED len(Payload)=32 first fragment IHL=6 (4 opt bytes) NEVER REASSEMBLED first fragment IHL=7 (8 opt bytes) NEVER REASSEMBLED first fragment IHL=8 (12 opt bytes) NEVER REASSEMBLED ``` Two ordinary contiguous fragments, no overlap, every field RFC-valid, 4 bytes of Router Alert. A real stack reassembles it without complaint. This matters for the fix proposed here: correcting the `currentOffset` arithmetic and making the fall-through explicit **does not close it**, because the set never reaches `build()` in the first place. It needs the `insert()` length fix from #1: ```go fragLength := in.Length - uint16(in.IHL)*4 ``` Suggest the test this issue asks for ("a known-good overlapping set reassembles to the BSD-Right result") be joined by one asserting that a **contiguous** set with `IHL > 5` on a non-last fragment reassembles too. That case has no test today and is the cheapest of the three to exploit. ### On the impact framing > A rival appends one small overlapping fragment to a fragmented exchange […] and the whole datagram vanishes from the analyser's view. Confirmed in principle, with one practical caveat from live testing: on a path where a middlebox reassembles (a Linux bridge with `bridge-nf-call-iptables=1`, or any conntracking hop), an *overlapping* set is normalised or dropped before it reaches a downstream capture, so the injected fragment never arrives. The IP-options variant above does not have that limitation — Router Alert's copy-on-fragment bit is set, so the option survives a reassemble/re-fragment cycle and the evasion works through the middlebox. Worth noting when assessing reachability. --- *Verified on Go 1.24.4 against `gopacket/gopacket v1.7.0`.*
Sign in to join this conversation.
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
noi/gopacket#2
No description provided.