Repository navigation
fix(toolkit): scapy cuts 4in6 at each Payload Length and skips ICMP quotes (#1623, #1627) - #1631
Merged
Conversation
…uotes (#1623, #1627) - #1623: ipv4_reassembly handed on bytes() of the IPv4 layer, which Scapy ends with the frame's trailer however deep the layer, so IPv4 carried in IPv6 ran past each Payload Length above it. It now takes the octets _within_datagrams() bounds, and gives no input when they cut the header. - #1627: IPerror (protocol 41) and IPerror6 (next header 4 or 41) bind full IPv6 and IP layers, so _tcp_carrier found TCP inside an ICMP error's quote. Its walk now stops at either (_quoted). ipv4_reassembly finds its layer by the same walk (_first_ipv4) instead of packet['IP'], which reached into ICMPv6 quotes and into a Redirected Header's packet field. New tests/toolkit/test_scapy_ipv6_payload_and_icmp_quote_1623_1627_unit.py compares scapy and dpkt with the default engine over 29 IPv4 and 21 TCP cases; it fails without the fix, and each guard is load-bearing.
Owner
Author
|
Verdict: GOOD TO GO at Sonnet cross-review, on a 53-case IPv4 probe and a 51-case TCP probe under scapy 2.7.0 and 2.8.0. It confirmed 7 of 21 cases differing before and 1 after, the 42 failures with Non-blocking notes:
Not checked: scapy 2.6.0 (the floor), and the author's own 31-case probe. I re-ran the new test file at this head: 6 passed, 110 subtests. |
JarryShaw
added
the
review: good-to-go
Contributor
|
Coverage: 89.66% (unit tier, Python 3.14,
Per-file detail: the |
JarryShaw
removed
the
review: good-to-go
This was referenced Oct 14, 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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please follow the guide below
You will be asked some questions, please read them carefully and answer honestly
Put an
xinto all the boxes [ ] relevant to your pull request (like that [x])Use Preview tab to see how your pull request will actually look like
Searched for similar pull requests
Followed the coding style (
make pylint,make mypy,make isort)make testpasses, and a test case covers the changeAdded a changelog entry under
docs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
Tick the commit type your subject line carries.
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingrelease— bumps the version or rolls up a distributionchore— anything elseDescription of your pull request and other information
#1623. Scapy files a frame's trailer as padding at the end of the packet, so
bytes()of IPv4 carried in IPv6 ran past each Payload Length.ipv4_reassemblynow takes_within_datagrams(ipv4)(pcapkit/toolkit/scapy.py:544) and gives no input when those octets cut the header (:547). The issue's two rows now match the default engine: payloads 16, 8 (were 22, 14), and 10, 4 with the datagrampartial(were 16, 8,complete).#1627.
IPerror(protocol 41) andIPerror6(next header 4 or 41) bind fullIPv6/IPlayers, so_tcp_carrierfound TCP in an ICMP error's quote. The walk now stops at either (_quoted,:337,:482).ipv4_reassemblyfinds its layer by the same walk (_first_ipv4,:365):packet['IP']reached IPv4 in ICMPv6 quotes and in a Redirected Header's packet field.Measured on 3.14 (scapy 2.7.0): scapy differed from the default engine in 7 of 21 #1622 IPv4 cases and 20 of 31 probe cases; afterwards only #1609's 33-header case and the out-of-scope rows below. dpkt agrees throughout.
New
tests/toolkit/test_scapy_ipv6_payload_and_icmp_quote_1623_1627_unit.py: 29 IPv4 and 21 TCP cases, scapy and dpkt against the default engine. Revertingpcapkit/gives 42 failures; each of the 5 guards fails it alone. Also green on 3.11 (scapy 2.8.0).Not fixed here, identical before and after:
ipv6_reassemblystill takes a Fragment header from below the first IPv6 (6in6, ICMP quotes);Issues for the first two are being filed.
Closes #1623
Closes #1627