Skip to content

fix(toolkit): dpkt and scapy check IPv4 headers as the default engine does (#1607) - #1614

Merged
JarryShaw merged 1 commit into
mainfrom
fix/dpkt-scapy-ipv4-header-checks
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/dpkt-scapy-ipv4-header-checks

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 #1607.

dpkt and scapy decode any IPv4 EtherType or tunnel payload as IPv4, whatever its version, and check none of its options, so a header the default engine rejects still produced IPv4 and TCP input.

Each now asks the default engine's own IPv4 parser, reusing pcapkit.toolkit.pypcapfile._default_accepts from #1600 rather than a third copy of the rules (pypcapfile.py itself is unchanged):

  • _ipv4_accepted in each toolkit; a version-4, IHL-5 header skips the parser.
  • dpkt _tcp_carrier and scapy _carrier check every IPv4 header between the frame and the TCP segment, so outer and tunnelled headers are both covered (fix(toolkit): key tunnelled TCP by the innermost IP header (#1581) #1605).
  • ipv4_reassembly of both checks its IPv4 layer too, as the default engine hands no IPv4 input for a rejected header.

Fields checked: the default engine rejects a bad version, an IHL below 5 or past the capture, and bad options. It does not check the header checksum, so that already agreed. Across 42 crafted frames, the cases where IPv4, TCP or flow counts disagree went from 28 to 2. Both remaining cases are dpkt finding no IPv4 inside IPv6 for IPv4 reassembly. The cross-review found further pre-existing dpkt and scapy disagreements that are not about header validity, tracked separately.

New tests: tests/toolkit/test_dpkt_scapy_ipv4_header_checks_1607_unit.py (4 tests, all fail on main). The full suite was not run locally, only narrow selections.

… does (#1607)

- dpkt and scapy decoded an IPv4 header the default engine rejects (version not 4, IHL below 5 or past the capture, options its parser rejects) and still handed on IPv4 and TCP input. Each IPv4 header is now asked of the default engine's own parser via pcapkit.toolkit.pypcapfile._default_accepts, outer and tunnelled alike; version 4 with IHL 5 skips the parser.
- Scapy hands the parser at least the 20 fixed octets, so an IHL below 5 warns of nothing the default engine does not.

New tests in tests/toolkit/test_dpkt_scapy_ipv4_header_checks_1607_unit.py fail without the fix.
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 10, 2026
@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
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 89.70% (unit tier, Python 3.14, f03b34e4d, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 3 20 3 95.16%
pcapkit/const 18858 954 2366 868 91.06%
pcapkit/corekit 2366 67 760 35 96.48%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2883 147 1010 36 94.07%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17089 226 4566 191 98.06%
pcapkit/toolkit 923 134 332 6 84.22%
pcapkit/utilities 494 4 146 4 98.75%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at f03b34e4d. This was a Sonnet cross-review; the author ran on Opus.

The review built its own corpus of about 61,000 frames:

  • the full IPv4 options grid;
  • every version and IHL value against short captures;
  • fragments;
  • tunnels three deep, behind VLAN, with the outer or the inner header bad;
  • 20,000 fuzzed frames.

Results:

  • Validation: dpkt and scapy now accept and reject exactly the IPv4 headers the default engine does, with 0 disagreeing frames. On main, 23,334 options-grid frames disagree.
  • Fast path: skipping the parser for a version-4, IHL-5 header is safe. The default parser rejected 0 of 396,608 such headers.
  • Regressions: none. All 26 generated fixtures give byte-identical dpkt and scapy output.
  • Imports: no import-time dependency on pcapfile and no cycle, checked on Python 3.14.
  • Tests: every new test fails on main.

Non-blocking:

  • Cost: checking a header with options costs about 0.4 ms (4-octet option) to 1.5 ms (40-octet area). It stays below the default engine's own time. _default_accepts has a 2-entry lru_cache, which deep tunnels can thrash.
  • Pre-existing gaps: the review found further disagreements that predate this PR and are not about header validity. I will file them separately. So the body's "both remaining cases" and its Total Length remark were too strong, and I have corrected both.

@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 9f37fb0 into main Oct 10, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the fix/dpkt-scapy-ipv4-header-checks branch October 10, 2026 23:00
@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).
JarryShaw added a commit that referenced this pull request Oct 14, 2026
Third-party engines are best effort (#1687), so the dpkt code that exists
only to match the default engine comes out:

- revert #1662, #1655, #1624, #1622, #1614 and the dpkt hunks of #1605:
  the octet walk and its link-type stash, IPv6 in IPv4 and IPv4 in IPv6,
  refusing MPLS and ISL, the tunnel and extension-header walks, the IPv4
  header checks and the layer bounds
- keep #1690's timestamps, #1594's parser choice, #1621 and #1653
- re-land as guards: a layer dpkt's own lengths misplace (#1646) is
  re-serialised, not refused with ProtocolError; a Total Length below the
  IHL keeps the header whole (#1616), and a header the capture cuts is no
  input; packet2dict and packet2chain take a str payload and a raw frame
- delete the modules #1662 and #1655 added; the 1615/1616 module keeps
  only what its #1616 tests use, as no scapy test imports it now; dpkt's
  comparisons with the default engine assert constants or the capture's
  octets, or are dropped
- drop pypcapfile's VLAN_TAGS, VLAN_TAG_LEN and IPV6_EXTENSION_HEADERS,
  which only dpkt imported, and the 1668 module's _Agreement base, which
  nothing subclasses now
- the docs drop the depth claims and name what dpkt leaves out silently

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

- revert #1662, #1655, #1624, #1622, #1614 and the dpkt hunks of #1605:
  the octet walk and its link-type stash, IPv6 in IPv4 and IPv4 in IPv6,
  refusing MPLS and ISL, the tunnel and extension-header walks, the IPv4
  header checks and the layer bounds
- keep #1690's timestamps, #1594's parser choice, #1621 and #1653
- re-land as guards: a layer dpkt's own lengths misplace (#1646) is
  re-serialised, not refused with ProtocolError; a Total Length below the
  IHL keeps the header whole (#1616), and a header the capture cuts is no
  input; packet2dict and packet2chain take a str payload and a raw frame
- delete the modules #1662 and #1655 added; the 1615/1616 module keeps
  only what its #1616 tests use, as no scapy test imports it now; dpkt's
  comparisons with the default engine assert constants or the capture's
  octets, or are dropped
- drop pypcapfile's VLAN_TAGS, VLAN_TAG_LEN and IPV6_EXTENSION_HEADERS,
  which only dpkt imported, and the 1668 module's _Agreement base, which
  nothing subclasses now
- the docs drop the depth claims and name what dpkt leaves out silently

The dpkt modules, the shared modules that run dpkt, tests/foundation/engines
and the project guards pass on 3.14 and 3.11 (dpkt 1.9.8, pypcapfile 0.12.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 dpkt and scapy engines accept IPv4 headers the default engine rejects

1 participant