Skip to content

fix(toolkit): the scapy adapters stop at the IP datagram's declared length (#1584) - #1589

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1584-scapy-ipv6-trailer
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1584-scapy-ipv6-trailer

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Closes #1584.

Scapy files the octets past an IP layer's declared length as a Padding layer, and bytes() of every layer above it ends with them. pcapkit/toolkit/scapy.py took payloads with bytes(layer.payload), so:

  • IPv6 reassembly (ipv6_reassembly): fragment data and tl included the trailer. The 80-octet datagram from the issue came back 84 octets.
  • TCP reassembly and tracing (tcp_reassembly, tcp_traceflow): the payload, len and last included Ethernet padding and trailers. A padded 60-octet frame holding a 1-octet segment gave 6 octets.

_within_datagrams() cuts at the default engine's boundaries:

  • IPv4: the Total Length, where 0 (TSO) runs to the end of the frame.
  • IPv6: the Payload Length, or for a jumbogram pcapkit's own jumbo_payload_length, because Scapy misreads that field.
  • Tunnels: every enclosing IP layer bounds the data, and the shortest end wins.

A TCP segment gives no record when its datagrams hold fewer than 20 octets of it, or its Data Offset is under 5, as on the default engine.

IPv4 reassembly is unchanged. Its adapter hands the trailer on just as the default engine's adapter does, and foundation/reassembly/ip.py cuts at tl - ihl.

Tests (tests/toolkit/test_scapy_unit.py::ScapyTrailerTests): the adapters match the default engine field for field, with the expected values pinned. Each new test fails without its fix. test_data_with_no_length_to_stop_at_is_taken_whole fails on main only because the helper it calls does not exist there. Coverage of toolkit/scapy.py went from 98% to 99%. I ran narrow selections only, not make test.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 051defa55: small, and limited to malformed input. Cross-reviewed on Sonnet; the author ran Opus.

Needed:

  1. TCP Data Offset below 5 is new to this PR. The payload now starts with header octets, e.g. 87d00000 (checksum and urgent pointer) before the data. main gave a clean payload, and the default engine drops such a segment. Match the default engine.
  2. IPv4 Total Length below the header length. The PR emits an empty record with header=b''. main emitted a full one, and the default engine emits none. Match the default engine.
  3. Tunnels, already broken on main and same function. Bound the segment by the shortest of all enclosing IP layers, not only the nearest. Today the outer datagram's trailer leaks in when the inner length overruns the outer one (default 0 octets, PR 4), and on inner TL 0 inside a sized outer (default 5, PR 9).

Confirmed:

  • Default engine: byte-identical over 43 captures and 3,000 mutated frames. Covers padding, FCS, VLAN/QinQ, IPv6 extension headers, TSO, snaplen cuts, first fragments and well-formed tunnels.
  • Fix counts: 1,180 TCP and 135 IPv6 mutated cases are fixed, with no new divergences except items 1–2.
  • Sample captures: scapy output is unchanged across the 23 samples, trace files included.
  • Jumbograms: scapy 2.6–2.8 read the Jumbo length in native byte order and add the HBH length twice (IPv6.extract_padding). jumbo_payload_length handles option placement and RFC 2675.
  • Tests and versions: the tests pass on scapy 2.6.0, 2.7.0 and 2.8.0. Fail-before is 23 on main.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.56% (unit tier, Python 3.14, 4ce6ee380, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18858 957 2366 871 91.03%
pcapkit/corekit 2334 67 746 35 96.43%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2792 154 984 42 93.43%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16988 230 4540 191 98.02%
pcapkit/toolkit 738 123 250 6 81.68%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

…ength (#1584)

Scapy files the octets past an IP layer's Total Length or Payload Length as
a Padding layer, and bytes() of every layer above it ends with them.

- ipv6_reassembly took the fragment's data, and so its tl, with the frame's
  trailer on the end: an 80-octet datagram reassembled to 84 octets.
- tcp_reassembly and tcp_traceflow did the same to the TCP payload, len and
  last. A segment now ends at the shortest end any enclosing IP layer
  declares, and is dropped, as the default engine drops it, when that holds
  under 20 octets or the Data Offset counts under 5.
- Jumbograms use pcapkit's own Jumbo Payload Length, since Scapy misreads
  it and splits nothing off.
- ipv4_reassembly is unchanged: it hands the trailer on as the default
  engine's adapter does, and the reassembler cuts at tl - ihl.

Tests: the adapters match the default engine on every frame tested, and each
new test fails without its fix.
@JarryShaw
JarryShaw force-pushed the fix/1584-scapy-ipv6-trailer branch from 051defa to 4ce6ee3 Compare October 10, 2026 07:18
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 4ce6ee380. This was a Sonnet delta review; the author ran Opus. The PR merges cleanly into main and pairwise with #1575 and #1594.

  • All three round-1 items now match the default engine byte for byte. That covers Data Offset < 5 (no segment), TL below the header (no record), and the tunnel bound (the shortest enclosing IP layer).
  • No regressions in the 3,000-frame mutation run: 0 frames where main or 051defa55 agreed with the default engine and this head doesn't. 1,388 more TCP/trace records and 135 more IPv6 records agree than on main.
  • The 30 GRE records only trim the trailer. Scapy already reported GRE TCP on main where the default engine gives none, and the PR adds no records.
  • _within_datagrams never cuts a legitimate segment short. The reviewer checked this across 4,370 crafted frames: TL 0 at every tunnel depth, jumbograms in tunnels, IPv6 extension chains, exact and ±N declared lengths, and inner IPv6 in outer fragments. Every record where the default engine has one is identical.
  • Fail-before: all three new tests fail on 051defa55.
  • Tests: scapy toolkit (24), engine (13), trace (12), parity (5) and the agreement module all pass.

Unchanged from main, for the record: scapy's pcap reader truncates frames at 65,535 octets.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw
JarryShaw merged commit 931960f into main Oct 10, 2026
39 checks passed
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
JarryShaw added a commit that referenced this pull request Oct 14, 2026
Third-party engines are best effort (#1687), so the scapy code that
exists only to match the default engine comes out:

- revert #1683, #1663, #1652 and #1631, and the scapy hunks of #1614: the
  link-type and loopback-family walk, the frame and extension-header
  bounds, the IPv6 chain walk over the octets, the stops at GRE, VXLAN,
  PPPoE and the other layers the default engine leaves raw and at ICMP
  quotes, and the IPv4 header checks
- keep #1624's walk (TCP behind AH or an IPv6 extension header in IPv4,
  and the later-fragment stop), #1605's keying by the carrying IP header
  and #1589's trailer cut; the walk calls _walk_extension_headers, which
  the pypcapfile revert keeps
- re-land as guards: a zero-length frame is no IPv4 input and has no
  packet2dict octets (#1676); IPv4 in IPv6 ends at the Payload Length
  (#1623); a packet built by hand is read off its octets (#1676);
  ipv6_reassembly takes only its own IPv6 layer's Fragment header, and
  none in a later IPv4 fragment
- delete the four scapy agreement modules and test_scapy_unit's
  default-engine checks; the guards' tests assert constants
- docs: scapy applies the extension-header bound only behind IPv4 and the
  frame-layer bound nowhere, and its silent gaps are listed
  where gaps are said to be announced

The scapy toolkit and engine modules, the shared modules that run scapy,
and the project and tier guards pass on 3.14 (scapy 2.7.0) and 3.11
(scapy 2.8.0).
JarryShaw added a commit that referenced this pull request Oct 14, 2026
Third-party engines are best effort (#1687), so the scapy code that
exists only to match the default engine comes out:

- revert #1683, #1663, #1652 and #1631, and the scapy hunks of #1614: the
  link-type and loopback-family walk, the frame and extension-header
  bounds, the IPv6 chain walk over the octets, the stops at GRE, VXLAN,
  PPPoE and the other layers the default engine leaves raw and at ICMP
  quotes, and the IPv4 header checks
- keep #1624's walk (TCP behind AH or an IPv6 extension header in IPv4,
  and the later-fragment stop), #1605's keying by the carrying IP header
  and #1589's trailer cut; the walk calls _walk_extension_headers, which
  the pypcapfile revert keeps
- re-land as guards: a zero-length frame is no IPv4 input and has no
  packet2dict octets (#1676); IPv4 in IPv6 ends at the Payload Length
  (#1623); a packet built by hand is read off its octets (#1676);
  ipv6_reassembly takes only its own IPv6 layer's Fragment header, and
  none in a later IPv4 fragment
- delete the four scapy agreement modules and test_scapy_unit's
  default-engine checks; the guards' tests assert constants
- docs: scapy applies the extension-header bound only behind IPv4 and the
  frame-layer bound nowhere, and its silent gaps are listed
  where gaps are said to be announced

The scapy toolkit and engine modules, the shared modules that run scapy,
and the project and tier guards pass on 3.14 (scapy 2.7.0) and 3.11
(scapy 2.8.0).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(toolkit): the scapy engine's IPv6 reassembly includes the frame trailer after the IPv6 payload

1 participant