Repository navigation
fix(ipv6): parse unrecognised extension headers generically so the chain survives #891
Description
Describe the bug
When an IPv6 extension header fails to parse, @beholder substitutes a Raw — but IPv6's own chain-walk then reads .next off it, which Raw does not carry:
ipv6.py:329 info = next_.info # next_ is whatever _import_next_layer returned
ipv6.py:338 proto = info.next # AttributeError when next_ is the Raw fallback
The consequence is not confined to the extension header. The AttributeError escapes IPv6.read before IPv6's own info is returned, so the @beholder one level up turns the whole IPv6 packet into Raw — source, destination, hop limit and flow label lost along with the extension header.
Reproduction
Measured on main at 21b121c0e, and on PR #889's head, with PYTHONSAFEPATH=1 and pcapkit.__file__ asserted to the tree under test. A well-formed Mobility Header (an IPv6 extension header — Mobility_Header = 135 is in pcapkit/const/ipv6/extension_header.py:42) whose status byte is unassigned:
Ethernet/IPv6 assigned status -> payload=IPv6 protochain=Ethernet:IPv6:MH
Ethernet/IPv6 unassigned status -> payload=Raw protochain=Ethernet:Internet_Protocol_version_6
IPv6 alone unassigned status -> AttributeError: 'Raw' object has no attribute 'next'
Note both IPv6 and MH are absent from the degraded protochain.
It is not specific to that enum. On unmodified main, two unrelated MH failure modes raise the identical AttributeError: a truncated MH (6 bytes) and an absurd Header Len of 0xff. So any extension-header parse failure reaches it.
Expected behavior
A failed extension header degrades to Raw in place, leaving the IPv6 header's own parsed fields intact and the rest of the chain walked as far as it can be — which is what @beholder exists to provide. At minimum the walk should stop cleanly on a Raw rather than raising AttributeError, since Raw is a value _import_next_layer is documented to return.
System information
- Python 3.14.7, CPython, checkout at
21b121c0e.
Additional context
Surfaced while reviewing #889, which stops four mh.py helper enums from minting on an unassigned byte. #889 does not introduce this — it makes it far more reachable, turning a malformed-packet-only path into one a well-formed packet with an unrecognised status byte takes. It is out of scope there and #889 documents the real cost instead.
This also corrects a claim I made on #877 when recommending those enums raise: I said the cost was one MH message's parse. On the IPv6 path it is the whole IPv6 packet. See issues/877 for the correction.
Metadata
Metadata
Assignees
Labels
Projects
- StatusShow more project fieldsDone
Activity
JarryShaw commentedon Sep 29, 2026
One design decision needed before this is implemented, because "degrade in place" can mean three different things and they are not equivalent.
The mechanism is settled and measured:
ipv6.py:329takesinfo = next_.info,:338doesproto = info.next, and aRawfallback's info carries no.next, so theAttributeErrorescapesIPv6.readbefore IPv6's own info is returned and the outer@beholderreplaces the whole packet. Confirmed pre-existing — a truncated extension header and an absurdHeader Lenboth reach it on unmodifiedmain.What is not settled is what should happen instead. Three candidates:
(1) Stop the chain walk cleanly on a
Raw. Treat aRawfrom_import_next_layeras end-of-chain: keep IPv6's own parsed fields, attach theRawas the final layer, stop walking. A capture with a bad Mobility Header would then readEthernet:IPv6:Rawinstead ofEthernet:Internet_Protocol_version_6, with source, destination, hop limit and flow label intact.(2) Let the walk continue past it. Would need
Rawto carry a usablenext, which it cannot — the whole reason it is aRawis that the header did not parse, so the next-header byte is not trustworthy. I do not think this is viable and mention it only to rule it out explicitly.(3) Leave the wholesale degrade, and only stop it being an
AttributeError. Catch it, log it, and let the packet becomeRawas it does today — cheaper, but it keeps losing the IPv6 header fields, which is the actual damage.My recommendation is (1). It is what
@beholderexists to provide — a bad layer costs that layer — and it is the only option where an operator inspecting a capture still sees the addresses. It does change observable output for malformed captures (protochaingains aRawtail where the packet used to vanish into one), so it wants abreakinglabel and a test pinning the new shape.One consequence you should weigh, because it cuts the other way: #889 added a test asserting today's wholesale-degrade behaviour precisely so that fixing this would break it and force a look. Taking (1) means that test gets re-pointed rather than deleted — which is correct, but it means the shape lands in two places and the changelog entry for #889 will describe behaviour the library no longer has by then.
Labelled
needs: decision. Nothing dispatched until you rule.JarryShaw commentedon Sep 29, 2026
But i think IPv6 extension headers share the same first few fields (including length) formats? Then we can actually have a RawExtension fallback that carries the unparsed payload as if a Raw protocol then next to the actual next one.
JarryShaw commentedon Sep 29, 2026
Good idea, and mostly right — but I measured the layouts and there are three exceptions that decide the design. Field order taken from the schemas on
main:HOPOPTnext,len,optionsIPv6_Optsnext,len,optionsIPv6_Routenext,length,type,seg_leftMHnext,length,type,reserved,chksumHIPnext,len,pkt,ver, …AHnext,len,reserved,spi,seqIPv6_Fragnext,reserved,flags,id— no length at allESPnext, no length — has neitherShim6So
nextat offset 0 holds for seven of the nine, which is what makes yourRawExtensionidea work — the walk can keep going, which is strictly better than my option (1) of stopping. But the length needed to skip to that next header is not uniform:IPv6_Fragis fixed at 8 octets — byte 1 is reserved, not a length. A generic reader must special-case 44.AH'slenis in 4-octet units minus 2, not the 8-octet-units-minus-1 that HOPOPT/Opts/Route/MH/HIP use. Same field position, different arithmetic.ESPhas neither field — its next-header byte is inside the encrypted payload, so the chain genuinely ends there and no fallback can continue it.So the shape I would build, if you agree:
RawExtensionparsesnextat offset 0 and computes its own length from a per-protocol rule (8-octet units for the five, fixed 8 forFrag, 4-octet units forAH), carries the remainder as opaque bytes likeRaw, and handsnextback soipv6.py:338's walk continues to the real next header.ESPstays a terminal — it already is, and cannot be otherwise. That gets an operator the IPv6 header, a marker for the unparsable extension, and every layer after it, which none of my three options delivered.Two things to decide, since they are yours not mine:
RawExtensiontrust a length it cannot validate? If the bad header is bad because its length field is garbage, skipping by that length lands mid-packet and the next "header" is noise. Safer is to stop the walk when the computed length would overrun the remaining payload — so it continues where it can and stops where it cannot, rather than fabricating layers.protochainoutput changes either way — a bad Mobility Header currently makes the whole packetRaw, and would become something likeEthernet:IPv6:RawExtension:UDP. That is the point, but it isbreakingfor anyone matching on today's shape, and it means re-pointing the test fix(mh): stop the four RFC-inline helper enums from minting on unassigned bytes #889 added to pin the current behaviour.Confirm the shape and I will dispatch it. Keeping
needs: decisionuntil you do, since point 1 changes the implementation materially.JarryShaw commentedon Sep 29, 2026
Let's re-read IPv6 extension header's RFC documentation? Maybe some of these values are not considered as extension headers? Maybe there have been a standardised format of how legit extension headers should contain/look like so that we can have our RawExtension (name subject to your recommendation based on the library's naming convention and future facing considerations).
But if we really cannot find the trustworthy source or shared format for them, then we fall back to original recommendation on (1) - stop with a Raw containing whatever is remained.
JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim:
Right question to ask — my table was a census of this library's schemas, which is evidence about our code rather than about what a legitimate extension header is. Investigating against the specifications, with the fallback you named as the default if the answer comes back "no trustworthy shared format".
Four things being established, each to be answered with quoted text rather than summary:
IPv6_Frag's fixed 8 octets (RFC 8200 §4.5),AH's Payload Length unit and bias (RFC 4302, to be read rather than recalled), andESP's next-header byte living inside the encrypted payload (RFC 4303). PlusMobility_Header,HIP,Shim6,BIT_EMUand whether 253/254 are extension headers at all or merely reserved protocol numbers.RawExtensionis buildable. If not, your fallback applies.Name deferred to the same investigation, as you asked — against the library's own conventions and where it should live (
protocols/internet/beside the other extension headers, orprotocols/misc/besideRaw).Keeping
needs: decisionuntil the findings are in, since they decide which design you are choosing between.JarryShaw commentedon Sep 29, 2026
Findings in, and your instinct was right — there is a trustworthy shared format, so the fallback is stricter than the evidence requires. It is RFC 6564, not RFC 8200 §4.8.
1. IANA says all twelve are extension headers, so that question is settled and not in our favour. The Protocol Numbers registry has an explicit column, and the
Yrows are an exact element-for-element match withpcapkit/const/ipv6/extension_header.py— nothing extra, nothing missing. Unsurprising once you look: the module is generated from that column. (The CSV isprotocol-numbers-1.csv; the unsuffixed URL 404s.)2. But the RFCs do narrow it, and the format requirement is a real MUST. RFC 6564, A Uniform Format for IPv6 Extension Headers, Standards Track, Updates 2460, RFC 2119 keywords declared. §4, verbatim:
And §5 answers the retroactivity question outright, verbatim:
Cite RFC 6564 §4, not RFC 8200 §4.8 — I had pointed at the wrong section. RFC 8200 §4.8 restates the layout without an RFC 2119 keyword ("they need to use the following format") and its field description carries a copy-paste defect, describing the generic length field as "Length of the Destination Options header".
3. So the answer to your decisive question is "yes, with a closed six-entry exception table":
HOPOPT0,IPv6_Route43,IPv6_Opts60,MH135,HIP139,Shim6140(octet1 + 1) * 8IPv6_Frag44AH51(octet1 + 2) * 4— RFC 4302 §2.2 flags its own deviationESP50,BIT_EMU147,253,254The four terminals are terminal for stated reasons, not by inference: RFC 8200 §4.5 says "the Encapsulating Security Payload (ESP) is not considered an extension header" and RFC 4303 puts its Next Header in a trailer after a variable-length encrypted payload with no length field anywhere; RFC 9801 defines 147 as a payload type ("the payload is an emulated bit-stream") with no header layout at all, making IANA's
Ya filing artefact; and RFC 3692 assigns 253/254 for local experiment with no wire format by design.Recommendation: build it. Six conformers today, plus every extension header standardised from April 2012 onward by the MUST — which is precisely the population a generic fallback exists to serve. Two need a documented per-value length rule, four terminate into a plain
Raw. That is enumerable and spec-cited, not heuristic. So: stop the walk, but only at those four values.Name:
RawExtension, inpcapkit/protocols/internet/raw_extension.py, subclassingInternet— your name, kept. The argument for the location is thatRawis defined by refusing to continue the chain:protocolraisesUnsupportedCall,lengthis hardcoded0,_nextis alwaysNoPayload().RawExtension's whole purpose is the opposite — to yield a real next-header value and a real octet length. It belongs beside the extension headers it stands in for. It should still followRaw's precedent of raisingUnsupportedCallfrom__index__, having no single protocol number.Two things still yours to say before I dispatch: whether it should refuse to skip when the computed length would overrun the remaining payload (I would — a garbage length field is the likeliest reason the header failed to parse, and skipping by it fabricates layers), and confirmation that
protochainchanging fromEthernet:Internet_Protocol_version_6toEthernet:IPv6:RawExtension:UDPisbreakingand re-points the test #889 added to pin today's behaviour.JarryShaw commentedon Sep 29, 2026
yes, it should, or actually i think the current existing workflow or house convention already handles/covers that.
yes confirmed - the
RawExtensionname is still up to your recommendations, as well as where it should be housed.JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim:
You are right that a convention already exists, and it is worth naming precisely because it does not quite cover this case. The house pattern is warn-and-clip: emit a
SchemaWarningsaying what was declared against what remains, then read what remains.The gap: clipping a payload and clipping a skip-distance are different operations. Clipping is right when the declared length only governs how much of this header to read — you take what is there and carry on. But
RawExtension's length also decides where the next header starts, and a clipped skip-distance points at whatever happens to sit at the end of the buffer, which is not a header. Continuing from there fabricates a layer out of trailing bytes, which is worse than stopping: a wrongprotochainentry reads as a parsed fact.So the implementation will follow the convention's warning and diverge on the action: emit a
SchemaWarningin the established wording, then stop the walk rather than clip-and-continue —RawExtensionabsorbs the remaining octets and reports no next header. The chain ends honestly at the bad header instead of inventing what follows it. I will say so in the docstring, citing the convention so the divergence reads as deliberate rather than as an oversight.Name and location, per your deferral:
RawExtension, inpcapkit/protocols/internet/raw_extension.py, subclassingInternet.Rawis defined by refusing to continue a chain —protocolraisesUnsupportedCall,lengthis hardcoded0,_nextis alwaysNoPayload().RawExtensionexists to do the opposite for a conforming header, so it belongs beside the extension headers it substitutes for rather than besideRaw. It will followRaw's precedent of raisingUnsupportedCallfrom__index__, having no single protocol number of its own.needs: decisionremoved,breakingandwipapplied, worker dispatched. Scope: the six RFC 6564 conformers walk generically,IPv6_Fraguses a constant 8 andAHuses(len + 2) * 4, andESP/BIT_EMU/253/254terminate into a plainRawas they do today.JarryShaw commentedon Sep 29, 2026
Confused tho. Now that we confirmed that IPv6 extension headers should have the uniformed format, why dont we make RawExtension dynamic, meaning, it reads and parses according to the shared format then its property is still able to return the necessary information as index, next, length, protocol (derived from the actual Enum value), etc.?
RawExtensionname may sound ambiguous - it's not directly pointing to IPv6 extension headers.1 remaining item
[-]fix(ipv6): a Raw fallback for a failed extension header makes the whole IPv6 packet degrade to Raw[/-][+]fix(ipv6): parse unrecognised extension headers generically so the chain survives[/+]JarryShaw commentedon Sep 29, 2026
What if we make it something like
IPv6_Extand have every other known implemented extension headers subclass it. So that we can sort of have the shared logic and common methodology that's being used in the library's house convention.JarryShaw commentedon Sep 29, 2026
Measured your
IPv6_Extproposal. Build it — but the rationale you gave for it is the weakest one available, and that changes what the base should contain.There is substantial shared logic: ~348 duplicated source lines collapsing to ~55, a net ~290 removed. But it is not the RFC 6564 header format. It is the extension-mode contract — the
extension=keyword, theself._extfflag, the threeUnsupportedCallguard properties, and theif extension: return infoshort-circuit. AST-normalised comparison of the class bodies:The format you named is shared by only 5 of 7, and it is the one thing I would not build the base around.
IPv6_Fraghas no length field at all (constant 8),AHuses(len + 2) * 4per RFC 4302, andESPis terminal. Worse, the 5 that do share it disagree on the schema field name —leninHOPOPT/IPv6_Opts/HIP,lengthinIPv6_Route/MH. A base designed around a length field two members do not have starts by papering over a rename. A base designed around the extension-mode contract covers all 7 and fixes three live inconsistencies.Two corrections to things I told you earlier, both mine:
Shim6is not implemented at all.pcapkit/protocols/internet/shim6.pydoes not exist; there is a 0-byteNotImplemented/shim6.py, excluded from the wheel byMANIFEST.in:90, with a TODO atinternet/__init__.py:12. So there are 7 implemented extension headers, not 8, and the generic-conformer set is 5, not 6. I had listedShim6in the exception table I gave the worker; corrected. It is actually a good test case for the generic parser — a registered IANA header with no class behind it.AHsubclassingIPsecis not an obstacle, as I implied it was.IPsecis 48 lines providing exactly one member,id(), which bothAHandESPoverride anyway. All four parenting shapes linearise with zero MRO conflict, verified live.AH(IPsec, IPv6_Ext)is the right one — re-parentingIPsecitself would dragESPinto a base whose contract it cannot honour.Breakage is near-nil and I checked rather than assumed: 0 MRO conflicts, 0
protochainstring changes (chains derive fromalias→ concrete class name, which does not move), 0 dispatch-table changes (Internet.__proto__holdsModuleDescriptorstring pairs, resolved lazily), 0 pickling impact, 0 realisinstance/issubclasscall sites inpcapkit/. One concrete casualty:tests/protocols/internet/test_ah_unit.py:111patchesIPsec.__post_init__, which stops intercepting once__post_init__hoists — that test needs rewriting. None of the 7 classes, schemas or data models is generated, so regeneration is unaffected.Two PRs, and the ordering matters more than the split. The #891 fix creates the 8th member of this family —
IPv6_GenericExtis precisely a subclass of the base you are proposing. DesigningIPv6_Extagainst 7 members and then retrofitting the 8th is worse than letting the 8th exist first. Also the proof obligations are opposite: #891 must ship a test that fails without it, while the refactor's correctness argument is "the diff only moves lines and the suite is unchanged" — merge them and that argument is unavailable. I will file the refactor as its own issue once #891 lands, unless you would rather have it first, in which case refactor-only goes first and the generic parser lands as a subclass afterwards. What I would argue against is one merged PR either way.Name clash to settle:
IPv6_Extcollides with the in-flightipv6_generic_ext.pyand with theExtensionHeaderenum inpcapkit/const/ipv6/extension_header.py. SuggestIPv6_Extfor the base and keepingIPv6_GenericExtfor the concrete generic parser — base names the family, subclass names the mechanism. Unverified and worth settling before either lands: whether mypy/pylint in CI accept two identically-parameterised generic bases onAH.JarryShaw commentedon Sep 29, 2026
If we double inherit
AH, we should do it forESPas well i think. Like all IPv6 extension headers should get the same base class ofIPv6_Extwhich contains the shared contracts for extension headers - despite some of these headers can be standalone protocols by themeless. Or to make it simpler, we only useIPv6_Extas base class for those headers that cannot stand alone?JarryShaw commentedon Sep 29, 2026
Measured both options. Neither boundary is "standalone vs not" — and
ESPturns out to be a different case than you or I assumed.The contract markers, counted per file:
7 of 8 implement the contract in full.
ESPstores the flag and then ignores it.esp.py:1276doesself._extf = extensionand nothing ever reads it — compareah.py:75,87,99, threeif self._extf:guards. ESP also has zeroUnsupportedCallguard properties and noif extension: returnshort-circuit. SoESP(..., extension=True)today records the flag and behaves exactly as if it wereFalse. That is a latent defect independent of this refactor, and I am filing it separately.So your option 2 — "only headers that cannot stand alone" — is the weaker one, which is the opposite of what I would have guessed. It would exclude
AH,MHandHIP, all three of which are arguably standalone protocols and all three of which implement the contract completely. That cuts the base's coverage from 7 to 4 while removing nothing structural. The line you are reaching for is not "can it stand alone" but "does it participate in extension mode", and those two do not coincide.On the symmetry instinct — right in principle, and the facts do not support it here.
They are not symmetric:
AHhonours the contract andESPdoes not. GivingESPthe base would inherit the threeUnsupportedCallguards and theif extension: returnit currently lacks — which is to say it would fixESP, not merely re-parent it. That is a behaviour change, and smuggling it inside a refactor whose whole correctness argument is "the diff only moves lines" destroys that argument. Same reason I argued against merging #891 with the refactor.Recommendation:
IPv6_Extcovers the 7 that implement the contract today,AHviaAH(IPsec, IPv6_Ext)as you suggested.ESPstays out of the refactor and gets its own decision: should ESP support extension mode? If yes, it joins the base in that change and the dead_extfis fixed by construction. If no, theextension=keyword should be removed from its signature rather than accepted and discarded.That keeps the refactor provably behaviour-preserving and puts the ESP question where it can be answered on its merits instead of as a side effect.
JarryShaw commentedon Sep 29, 2026
If per IANA, ESP is an extension header, then it MUST honour the same contracts - that's an implementation defect. And thus we should have ESP double inherit as AH then.
JarryShaw commentedon Sep 29, 2026
Ruling recorded, verbatim, and the condition is satisfied — I checked IANA directly:
IANA's
protocol-numbers-1.csv, theIPv6 Extension Headercolumn:Yfor both. And this library already agrees —pcapkit/const/ipv6/extension_header.py:33listsESP = 50in its own IANA-derived extension-header registry. So the inconsistency is internal: the registry says ESP is an extension header while theESPclass acceptsextension=, stores it atesp.py:1276, and never reads it. That is a sharper statement of the defect than I made earlier.So your ruling stands and it resolves the tension I raised. Consequences:
IPv6_Extcovers 8, not 7 — the seven that implement the contract today plusESP, withESP(IPsec, IPv6_Ext)mirroringAH(IPsec, IPv6_Ext).Unchanged: #891 itself stays the narrow fix (the generic parser so the chain survives), and the
IPv6_Extbase remains a separate follow-up filed after #891 lands.One incidental from the same CSV, consistent with the earlier Shim6 finding:
140,Shim6,Shim6 Protocol,Y,[RFC5533]is a registered extension header with no implementation here, which is exactly the case the generic parser is for.JarryShaw commentedon Sep 29, 2026
Correcting the record on this issue, because a ruling you gave rests on a premise I got wrong.
The exception table I gave you and the implementer said:
and described those as keeping "degrading to a plain
Rawexactly as they do today". You then ruled, verbatim:That is correct about the wire format and wrong about this codebase. Measured on
origin/main: only ESP is implemented terminally — it parses cleanly toEthernet:IPv6:ESP. For 147 (BIT_EMU), 253 and 254, "as they do today" means the whole IPv6 packet collapses — which is the defect this issue exists to fix, not a benign terminal state.The mechanism, verified myself rather than relayed:
Enum_ExtensionHeadermembers —BIT_EMU: 147,Use_for_experimentation_and_testing_253,…_254— so they enter the extension-header walk atpcapkit/protocols/internet/ipv6.py:315.Internet.__proto__entry (measuredNonefor each), so_lookup_next_layerreturnsRaw.Rawhas nonext, soproto = info.nextat:338raisesAttributeError: 'Raw' object has no attribute 'next'.Result on PR #904 as it stands: code 140 is fixed, and 147/253/254 still produce
payload=Raw,protochain=Ethernet:Internet_Protocol_version_6— the exact #891 signature quoted in this issue's own body. Worse, my framing led the implementer to document that as intended, atipv6_generic_ext.py:51-54andipv6.py:445-446.So the table had a real consequence: it told the implementer to preserve the bug for three of the twelve codes, and they faithfully did. This issue's own Expected behavior — "At minimum the walk should stop cleanly on a
Rawrather than raisingAttributeError" — is unmet for those three.What I am briefing unless you rule otherwise: make the walk break when the layer it just decoded has no
next, rather than special-casing a code list. That satisfies the terminal semantics you ruled for, fixes all three codes, and needs no table at all — which is the right shape, since a table of codes is exactly what was wrong here. The two docstrings get corrected to say the walk stops at these headers rather than that the packet degrades.Corrected table for the record, with the implemented-versus-conforming distinction that mine elided:
mainHOPOPT,IPv6_Route,IPv6_Opts,MH,HIP(octet1 + 1) * 8IPv6_FragAH(octet1 + 2) * 4(RFC 4302)ESPShim6(140)BIT_EMU(147),253,254fix(ipv6): parse unrecognised extension headers generically (#891)
fix(ipv6): parse unrecognised extension headers generically (#891)
fix(ipv6): parse unrecognised extension headers generically (#891) (#904