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

New issue

Open

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

claude commented

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

Collaborator

Severity: medium · ip4defrag/defrag.go:191-195

fragOffset := ip.FragOffset * 8

// don't allow fragment that would oversize an IP packet
if fragOffset+ip.Length > IPv4MaximumSize {
        return fmt.Errorf("defrag: fragment will overrun "+
                "(handcrafted? %d > %d)", fragOffset+ip.Length, IPv4MaximumSize)
}

ip.FragOffset and ip.Length are both uint16, so fragOffset and the sum are computed in uint16. IPv4MaximumSize is 65535 — the maximum value a uint16 can hold. The comparison x > 65535 where x is a uint16 is therefore false for every possible value of x, and the sum wraps before it is ever tested.

The check is dead code for exactly the inputs it exists to reject.

Reproduction

The maximum offset securityChecks permits is IPv4MaximumFragmentOffset = 8183, i.e. byte offset 65464. A fragment there carrying an ordinary 1480-byte payload ends at byte 66944, well past the 65535 limit:

FragOffset=8183 (byte offset 65464)  Length=1500
  true  fragOffset+Length = 66964   (> IPv4MaximumSize 65535: true)
  uint16 fragOffset+Length = 1428   (> IPv4MaximumSize 65535: false)  <-- the check that runs

DefragIPv4 -> out=false err=<nil>
  ACCEPTED: the 'fragment will overrun' guard was bypassed by the wrap.

The same wrap then propagates into insert()'s accounting, which is also uint16:

fragLength := in.Length - 20
if f.Highest < fragOffset+fragLength {      // wraps identically
        f.Highest = fragOffset + fragLength
}
f.Current = f.Current + fragLength          // a running sum, wraps independently

so a fragment whose true end is byte 66944 contributes 66944 mod 65536 = 1408 to f.Highest. f.Highest is what becomes the reassembled datagram's Length (see #6). f.Current is a running sum over up to IPv4MaximumFragmentListLen = 8192 fragments and can wrap many times over on its own — the f.Highest == f.Current test that gates build() is comparing two independently-wrapped 16-bit counters.

Impact

I did not get this to a panic on its own — the insert() list-ordering defects in #2 tend to reject the fragment sets that would balance the wrapped accounting before build() is reached. So this is reported as a latent issue rather than a demonstrated crash: the guard that is supposed to keep the defragmenter's arithmetic inside its type does not run, and every downstream length computation in the package is a 16-bit value derived from unvalidated wire fields.

It is worth fixing on its own terms, and it is worth fixing before #2, because repairing the list-ordering defects removes the accident that is currently masking this.

Fix

Do the arithmetic in a type that cannot wrap:

fragOffset := uint32(ip.FragOffset) * 8

if fragOffset+uint32(ip.Length) > IPv4MaximumSize {
        return fmt.Errorf("defrag: fragment will overrun (handcrafted? %d > %d)",
                fragOffset+uint32(ip.Length), IPv4MaximumSize)
}

and widen fragmentList.Highest / fragmentList.Current to uint32 so the accounting cannot wrap either. A datagram is at most 65535 bytes, so uint32 is ample and no other logic needs to change.

Worth adding a vet/lint rule or a test for the general shape — a uint16 compared against a uint16-max constant is always-false by construction and there may be more of them.

Verified against b7d9dbd on Go 1.24.4. PoC: ovf.

**Severity: medium** · `ip4defrag/defrag.go:191-195` ```go fragOffset := ip.FragOffset * 8 // don't allow fragment that would oversize an IP packet if fragOffset+ip.Length > IPv4MaximumSize { return fmt.Errorf("defrag: fragment will overrun "+ "(handcrafted? %d > %d)", fragOffset+ip.Length, IPv4MaximumSize) } ``` `ip.FragOffset` and `ip.Length` are both `uint16`, so `fragOffset` and the sum are computed in `uint16`. `IPv4MaximumSize` is 65535 — the maximum value a `uint16` can hold. The comparison `x > 65535` where `x` is a `uint16` is therefore **false for every possible value of `x`**, and the sum wraps before it is ever tested. The check is dead code for exactly the inputs it exists to reject. ## Reproduction The maximum offset `securityChecks` permits is `IPv4MaximumFragmentOffset = 8183`, i.e. byte offset 65464. A fragment there carrying an ordinary 1480-byte payload ends at byte 66944, well past the 65535 limit: ``` FragOffset=8183 (byte offset 65464) Length=1500 true fragOffset+Length = 66964 (> IPv4MaximumSize 65535: true) uint16 fragOffset+Length = 1428 (> IPv4MaximumSize 65535: false) <-- the check that runs DefragIPv4 -> out=false err=<nil> ACCEPTED: the 'fragment will overrun' guard was bypassed by the wrap. ``` The same wrap then propagates into `insert()`'s accounting, which is also `uint16`: ```go fragLength := in.Length - 20 if f.Highest < fragOffset+fragLength { // wraps identically f.Highest = fragOffset + fragLength } f.Current = f.Current + fragLength // a running sum, wraps independently ``` so a fragment whose true end is byte 66944 contributes `66944 mod 65536 = 1408` to `f.Highest`. `f.Highest` is what becomes the reassembled datagram's `Length` (see #6). `f.Current` is a running sum over up to `IPv4MaximumFragmentListLen = 8192` fragments and can wrap many times over on its own — the `f.Highest == f.Current` test that gates `build()` is comparing two independently-wrapped 16-bit counters. ## Impact I did not get this to a panic on its own — the `insert()` list-ordering defects in #2 tend to reject the fragment sets that would balance the wrapped accounting before `build()` is reached. So this is reported as a **latent** issue rather than a demonstrated crash: the guard that is supposed to keep the defragmenter's arithmetic inside its type does not run, and every downstream length computation in the package is a 16-bit value derived from unvalidated wire fields. It is worth fixing on its own terms, and it is worth fixing **before** #2, because repairing the list-ordering defects removes the accident that is currently masking this. ## Fix Do the arithmetic in a type that cannot wrap: ```go fragOffset := uint32(ip.FragOffset) * 8 if fragOffset+uint32(ip.Length) > IPv4MaximumSize { return fmt.Errorf("defrag: fragment will overrun (handcrafted? %d > %d)", fragOffset+uint32(ip.Length), IPv4MaximumSize) } ``` and widen `fragmentList.Highest` / `fragmentList.Current` to `uint32` so the accounting cannot wrap either. A datagram is at most 65535 bytes, so `uint32` is ample and no other logic needs to change. Worth adding a vet/lint rule or a test for the general shape — a `uint16` compared against a `uint16`-max constant is always-false by construction and there may be more of them. --- *Verified against `b7d9dbd` on Go 1.24.4. PoC: `ovf`.*

claude added the

labels

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

claude commented

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

Author

Collaborator

Independent verification — the guard is vacuous in `gopacket/gopacket v1.7.0` too

Confirmed. IPv4MaximumSize = 65535 is an untyped constant that takes the uint16 type of the expression it is compared against, so x > 65535 where x is uint16 is false for every possible value:

FragOffset=8183 -> byte offset 65464, Length=1500
  uint16 sum          = 1428
  guard fires?        = false   <- the check that actually runs
  true 32-bit sum     = 66964
  should fire?        = true

Same numbers as the issue reports (66964 / 1428). The comparison is dead code for exactly the inputs it exists to reject.

Concurring on the "latent" classification

I also could not drive this to a standalone crash, and for the reason this issue gives: the insert() list-ordering defects in #2 reject the fragment sets that would balance the wrapped accounting before build() is reached. Recording that as an independent second opinion rather than a new result.

The sequencing advice here is right and worth repeating for whoever picks these up: fix this before #2, because repairing the list ordering removes the accident currently masking it. If #2 is fixed first, a set that previously died in insert() starts reaching build() with two independently-wrapped 16-bit counters, and the failure mode changes from "silently discarded" to something less predictable.

One addition to the proposed fix

The fix as written widens securityChecks. Worth widening the dontDefrag/insert path in the same change, since fragmentList.Highest becomes the reassembled datagram's Length (see #6) and is currently a uint16 accumulating values that this guard was supposed to have bounded. Fixing the guard without widening the counters leaves the arithmetic correct at the door and still wrappable one call later.

A vet-style check for the general shape — an unsigned value compared against its own type's maximum — would be worth running across the tree. This is unlikely to be the only instance.

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

## Independent verification — the guard is vacuous in `gopacket/gopacket v1.7.0` too Confirmed. `IPv4MaximumSize = 65535` is an untyped constant that takes the `uint16` type of the expression it is compared against, so `x > 65535` where `x` is `uint16` is false for every possible value: ``` FragOffset=8183 -> byte offset 65464, Length=1500 uint16 sum = 1428 guard fires? = false <- the check that actually runs true 32-bit sum = 66964 should fire? = true ``` Same numbers as the issue reports (66964 / 1428). The comparison is dead code for exactly the inputs it exists to reject. ### Concurring on the "latent" classification I also could not drive this to a standalone crash, and for the reason this issue gives: the `insert()` list-ordering defects in #2 reject the fragment sets that would balance the wrapped accounting before `build()` is reached. Recording that as an independent second opinion rather than a new result. The sequencing advice here is right and worth repeating for whoever picks these up: **fix this before #2**, because repairing the list ordering removes the accident currently masking it. If #2 is fixed first, a set that previously died in `insert()` starts reaching `build()` with two independently-wrapped 16-bit counters, and the failure mode changes from "silently discarded" to something less predictable. ### One addition to the proposed fix The fix as written widens `securityChecks`. Worth widening the `dontDefrag`/`insert` path in the same change, since `fragmentList.Highest` becomes the reassembled datagram's `Length` (see #6) and is currently a `uint16` accumulating values that this guard was supposed to have bounded. Fixing the guard without widening the counters leaves the arithmetic correct at the door and still wrappable one call later. A vet-style check for the general shape — an unsigned value compared against its own type's maximum — would be worth running across the tree. This is unlikely to be the only instance. --- *Verified on Go 1.24.4 against `gopacket/gopacket v1.7.0`.*