TCP reassembly silently resolves conflicting retransmissions last-write-wins and still reports COMPLETE

[JarryShaw]

When two segments claim the same sequence range but carry different bytes, TCP reassembly silently keeps the later one and still reports the datagram as complete. Nothing in the returned Datagram records that the two disagreed.

Reproduction, on c28ffc287

from pcapkit.foundation.reassembly.tcp import TCP
from pcapkit.foundation.reassembly.data.tcp import Packet

def seg(num, seq, payload, *, ack=1000):
    return Packet(bufid=('192.0.2.1', 12345, '198.51.100.2', 443), dsn=seq, ack=ack,
                  num=num, syn=False, fin=False, rst=False, len=len(payload),
                  first=seq, last=seq + len(payload) - 1,
                  header=b'hdr', payload=bytearray(payload))

r = TCP(strict=True)
r(seg(1, 100, b'AAAAAAAA'))    # original
r(seg(2, 100, b'BBBBBBBB'))    # same range, different bytes
r(seg(3, 108, b'CCCC'))        # stream continues
for d in r.fetch():
    print(d.completed, bytes(d.payload), d.index)

True b'BBBBBBBBCCCC' (1, 2, 3)

b'AAAAAAAA' is gone without a trace, and completed is True.

Mechanism

pcapkit/foundation/reassembly/tcp.py:166:

else:           # if fragment partially overlaps existing payload
    RAW[PSN - ISN:PSN - ISN + info.len] = info.payload

The slice assignment is unconditional — the incoming payload is written over whatever occupied that range, with no comparison against the bytes already there. The hole-descriptor list is then updated from the segment's own bounds, so the overwritten range remains hole-free and completion is derived as COMPLETE. The mirrored branch at :176 (RAW = info.payload + RAW[-GAP:]) does the same for a segment that reaches back before the current ISN.

Why it is worth recording

A conforming stack retransmits identical bytes, so a disagreement means one of two things, and both matter to anyone using this for analysis:

• a broken or buggy sender, which the caller would want to know about;
• deliberately overlapping segments, which is the classic TCP-overlap IDS-evasion technique — the whole point of that attack is that different reassemblers resolve the conflict differently, so a tool that silently picks one and reports "complete" gives an answer that looks authoritative and isn't.

Last-write-wins is a defensible choice (it is roughly what Linux does), but making it silently and still claiming completeness is what turns it into a defect: there is no way for a caller to tell a clean stream from a contested one.

Possible directions

Not proposing a specific fix, since the right answer depends on whether the reassembler wants to grow a notion of "contested":

• compare before overwriting, and record the conflicting ranges on the Datagram (a new field, so additive);
• surface it as a ProtocolWarning when strict=True, leaving the resolution unchanged;
• keep first-write-wins instead, which is what some stacks do, and document the choice either way.

Found while reviewing #435, which changes completed to a Completion enum but does not touch this path — git diff origin/main over reassembly/tcp.py shows no change to either overlap branch, and :166/:176 are byte-identical on main. Filed separately rather than folded into that PR.

[JarryShaw]

Still reproduces on fa128959e, but three details in the report above have gone
stale and one new measurement bears directly on the design choice.

The reproduction needs updating. Packet now takes a required timestamp
argument, so the snippet as written dies with
TypeError: Packet.__init__() missing 1 required positional argument: 'timestamp'.
Adding timestamp=0.0 makes it run, and the behaviour is unchanged:

completed=complete  payload=b'BBBBBBBBCCCC'  index=(1, 2, 3)

The line numbers have moved. The unconditional slice assignment is now at
pcapkit/foundation/reassembly/tcp.py:194, and the mirrored reach-back branch at
:204 — not :166 and :176.

strict makes no difference, which matters for one of the options on the
table. Measured both ways:

strict=True :  completed=complete  payload=b'BBBBBBBBCCCC'  index=(1, 2, 3)
strict=False:  completed=complete  payload=b'BBBBBBBBCCCC'  index=(1, 2, 3)

That is because strict does not mean "be strict about anomalies" in this
codebase. It is documented as "if return all datagrams (including those not
implemented)" and stored as self._flag_s in
pcapkit/foundation/reassembly/reassembly.py:424; tcp.py refers to it only in
comments about whether an incomplete buffer is still reported. So "warn only
under strict=True" would be repurposing an existing flag that currently
controls something unrelated — worth knowing before choosing it, since it would
change the meaning of a public keyword rather than add a new one.

Nothing else here is a new finding; this comment is only to keep the issue
accurate for whoever picks it up.

[JarryShaw]

Design decision recorded, and the resolution question is answered by the spec rather than by preference. Two parts.

1. Surfacing: record the conflict on the Datagram

Chosen by the repo owner. Compare before overwriting, and add a field to Datagram naming the conflicting sequence ranges. Additive, so existing callers are unaffected, and it is the only option that lets a caller ask a datagram after the fact whether it was contested.

The strict=True gate is off the table, measured: strict means "if return all datagrams (including those not implemented)" — self._flag_s at pcapkit/foundation/reassembly/reassembly.py:424 — and makes no difference to this path either way. Gating on it would repurpose a public keyword that controls something unrelated.

There is direct precedent for surfacing rather than silently resolving: Zeek raises a rexmit_inconsistency event, documented as "Generated when Zeek detects a TCP retransmission inconsistency." So a mainstream analysis tool treats a conflicting retransmission as a reportable event in its own right.

2. Resolution: RFC 9293 specifies first-write-wins, so this becomes a conformance fix

RFC 9293 §3.10 (which obsoletes RFC 793) is explicit:

When a segment overlaps other already received segments, we reconstruct the segment to contain just the new data and adjust the header fields to be consistent.

"Just the new data" means the already-received bytes are kept and the overlapping portion of the arriving segment is discarded — first-write-wins. §3.10.7.4 points the same way: trim off any portion outside the window and process further only if the segment then begins at RCV.NXT, i.e. the duplicate prefix is trimmed from the incoming segment, never written over what is already buffered.

pcapkit/foundation/reassembly/tcp.py:194 does the opposite. The unconditional RAW[PSN - ISN:PSN - ISN + info.len] = info.payload overwrites already-buffered bytes with the arriving ones, and :204 mirrors it for the reach-back case. So the current behaviour is not RFC-conformant, which changes this from "a defensible choice made silently" into a straightforward conformance defect.

Correcting my own framing in the issue body

The body above says "Last-write-wins is a defensible choice (it is roughly what Linux does)". I never verified the Linux claim and it should not have been stated. Worse, it pointed a reader toward keeping the current behaviour as the sensible default when the spec says the opposite. Retracted. What I have actually verified is the RFC 9293 text quoted above and the existence of Zeek's event; I have not checked any endpoint stack's source, nor Snort's or Wireshark's reassembly policy, so nothing here rests on those.

What this implies, including the cost

Switching to first-write-wins changes reassembled payload bytes for any capture containing a conflicting overlap — the reproduction in the body would yield b'AAAAAAAACCCC' instead of b'BBBBBBBBCCCC'. That is a behaviour change, not an additive one, and it is worth being explicit about rather than burying: it is justified because the current output is non-conformant, and because a capture that hits this path is by definition anomalous. A conforming sender retransmits identical bytes, in which case first and last are the same and nothing changes at all.

So the shape of the fix is: compare before writing; keep the already-buffered bytes; discard the conflicting portion of the arriving segment; record the conflicting range on the Datagram; leave completed alone, since with the range recorded a caller can now tell a clean stream from a contested one.