toolkit(scapy): scale IPv4 fragment offset to octets before reassembly - #484
Conversation
#483) `pcapkit/toolkit/scapy.py`'s `ipv4_reassembly` passed `ipv4.frag` straight through as `fo`. Scapy reports that field in its on-wire 13-bit form -- 8-octet units per RFC 791 3.1 -- but `IP.reassembly()` (`ip.py`) indexes the datagram buffer with `fo` in octets, dividing by 8 only for the RCVBT bit table. Every other adapter already scales: `dpkt.py` does `ipv4.offset * 8`, `pypcapfile.py` does `ipv4.off * 8`, and `pcap.py`/`pcapng.py` pass pcapkit's own already-scaled `ipv4.offset`. This module's own IPv6 path (`fo= ipv6_frag.offset * 8`, a few lines down) does the same scaling scapy's IPv4 path was missing. Reproduced before the fix, through the public API: `scapy.all.IP(frag=5, flags='MF')` puts raw `5` on the wire (`0x2005`), and feeding a `frag=0`/MF fragment (40 octets of `A`) followed by a `frag=5` final fragment (8 octets of `B`) through `ipv4_reassembly` into `pcapkit.foundation.reassembly.ipv4. IPv4` reports `fo=5` for the second fragment instead of byte offset 40. The reassembler then writes it inside the first fragment's span and returns a completed 13-octet datagram, `b'AAAAABBBBBBBB'`, instead of the full 48 octets. After `fo=ipv4.frag * 8`, the same input reports `fo=40` and reassembles to `b'A'*40 + b'B'*8`. Added a regression test to `tests/toolkit/test_scapy_unit.py`: an `fo` assertion in the existing field-level test, plus a new test that drives the actual reassembler end to end and asserts on the datagram payload, since a bare `fo` check can't distinguish a real fix from one that merely relocates the same bug. Both fail without the `* 8` and pass with it. Does not touch `pcapkit/foundation/reassembly/ip.py` or its data model, which PR #482 owns.
| # datagram's declared total length is computed from the corrupted | ||
| # (unscaled) offset of the final fragment | ||
| self.assertEqual(len(datagram.payload), 48) | ||
| self.assertEqual(bytes(datagram.payload), b'A' * 40 + b'B' * 8) |
There was a problem hiding this comment.
Non-blocking suggestion: this test proves the payload-corruption half of #483's defect (the wrong bytes at the wrong offset), but it doesn't check the other symptom that the same corruption would have produced through #482's new conflict machinery.
I traced IP.reassembly() / IP._detect_conflicts() in pcapkit/foundation/reassembly/ip.py (unchanged by this PR) against this exact fixture under the pre-fix, unscaled fo:
- Fragment 1 (
fo=0, 40-octet payload) setsRCVBTbits for blocks 0-4 (FO // 8throughFO // 8 + (TL - IHL + 7) // 8= 0..5) and writesdatagram[0:40] = b'A' * 40. - Fragment 2, with an unscaled
fo=5instead of the correct40, writes atdatagram[5:13]._detect_conflictsruns before that write, over[5, 13): every one of those positions falls in an already-RCVBT-set block (0 and 1, both fully claimed by fragment 1), andtdlis still-1at that point (fragment 2's ownMF=0update happens after), so thetdl < 0branch of the guard is satisfied everywhere.datagram[pos]is'A'andpayload[index]is'B'for the whole span, so this reports a single conflicting run, i.e.conflict == ((5, 12),)-- a spurious conflict manufactured entirely by the unscaled offset, on data that never actually overlapped on the wire.
So a fix that scaled fo correctly but left some other consumer of the raw value unscaled would still be caught by the existing payload-bytes assertion, but a regression that reintroduced the offset bug would show up two ways: corrupted payload bytes and a non-empty conflict tuple on otherwise non-overlapping fragments. Since this is precisely the interaction #482 and #483 have with each other, asserting self.assertEqual(datagram.conflict, ()) alongside the existing payload assertion would pin that down directly rather than leaving it to be inferred from the payload check.
Not asking for a change before merge -- the existing assertions already conclusively demonstrate the fix works -- just flagging the gap since it's exactly the interaction worth covering here.
Review summaryReviewed at What changed
Checks performed
CIThe 2 skips are VerdictGOOD TO MERGE One non-blocking inline suggestion posted above (strengthening the regression test with a |
…conflict #484's review noted that the #483 regression test checks the reassembled payload but never checks #482's conflict record, even though the defect is precisely an overlap. Measured on main with the ``* 8`` scaling reverted, the datagram comes back: payload b'AAAAABBBBBBBB' (13 octets, not 48) conflict ((5, 12),) so fragment 2 landing inside fragment 1's span is directly observable in conflict, and the fixed code reports (). Asserting the record is empty pins the absence of an overlap rather than only the payload that results from there being none -- a later change could restore the right bytes by another route while still overwriting. Worth being precise about what this does and does not prove: reverting the scaling makes the earlier ``packet2.fo == 40`` assertion fail first, so this new assertion is not independently demonstrated to catch the regression on its own. Its discriminating power is the measurement above, ((5, 12),) against (), rather than a revert-proof. It is defence in depth behind the existing guard, not a replacement for it. Test only; tests/toolkit/test_scapy_unit.py 5 passed.
1 participant
Closes #483
What
pcapkit/toolkit/scapy.py'sipv4_reassemblypassed Scapy's raw 13-bitipv4.fragfield straight through asfo, the byte offset the reassemblerindexes its buffer with. Scapy's
fragis in on-wire 8-octet units per:rfc:
791#section-3.1, not bytes — sofocame out 8x too small for everyfragment except the first.
Every other adapter already gets this right:
dpkt.py:fo=ipv4.offset * 8pypcapfile.py:fo=ipv4.off * 8pcap.py/pcapng.py:fo=ipv4_info.offset, where pcapkit's ownipv4.pyalready scaled it (offset=int(schema.flags['offset']) * 8)fo=ipv6_frag.offset * 8Only the scapy IPv4 path was missing the
* 8.Reproduction (before the fix)
At the field level:
0x2005decodes as flags001(MF) + a 13-bit offset of5— the raw unitcount, not a byte offset of 40.
End to end, through the public API (
pcapkit.toolkit.scapy.ipv4_reassemblyinto
pcapkit.foundation.reassembly.ipv4.IPv4): feeding afrag=0/MFfragment carrying 40 octets of
A, then afrag=5final fragment carrying8 octets of
B, reportsfo=5for the second fragment (expected byteoffset 40). The reassembler writes it inside the first fragment's span
instead of after it, and returns a completed datagram truncated to 13
octets:
b'AAAAABBBBBBBB', instead of the full 48 octets(
b'A'*40 + b'B'*8).After the fix (
fo=ipv4.frag * 8), the second fragment reportsfo=40andthe datagram reassembles to the full 48 octets.
The fix
One line in
pcapkit/toolkit/scapy.py'sipv4_reassembly:with a comment recording the 8-octet-unit fact, mirroring the comment
already present on the IPv6 path in the same file.
Tests
tests/toolkit/test_scapy_unit.py:test_ipv4_and_ipv6_reassemblywith anfoassertion at the field level (mirroring the IPv6 assertion already there).
test_ipv4_reassembly_scales_fragment_offset_through_the_reassembler,which drives the actual reassembler with two crafted fragments and asserts
on the reassembled payload — not just on
foin isolation — since afragment-offset-only check can't distinguish a real fix from one that
merely relocates the same corruption elsewhere.
Both fail without the
* 8scaling (2 != 16and5 != 40respectively)and pass with it.
Scope
Does not touch
pcapkit/foundation/reassembly/ip.py, its data model, ortests/foundation/reassembly/test_ip.py— those belong to #482. Does notchange any other toolkit adapter; none showed the same defect.
Test plan
pytest tests/toolkit/test_scapy_unit.py -v— 5 passed (confirmed thetwo touched assertions fail without the fix, pass with it)
pytest tests/toolkit tests/foundation/reassembly tests/foundation/engines tests/interface -q— 230 passed, 15 skippedpytest tests -q(full suite) — 1035 passed, 17 skippedmypy --config-file mypy.ini pcapkit/toolkit/scapy.py— same 6pre-existing errors as on
origin/main, none introduced by this change(not skipped) in this environment