protocols: floor four more HIP list-length callbacks at zero - #466
Conversation
- NATTraversalModeParameter.modes, TransportFormatListParameter.formats, ESPTransformParameter.suites and HIPTransportModeParameter.mode each computed their ListField length as pkt['len'] - 2 with no lower bound; a peer declaring Length=0 drove that to -2, and ListField's own `while length > 0` loop silently returned an empty list instead of raising, so a malformed parameter parsed "successfully" with no exception at all. - Add a shared two_octet_prefix_list_len() helper, following #460's reg_info_list_len() precedent: same floor-at-zero-and-raise shape, raising FieldValueError. Named for the shared shape (a two-octet reserved/port field ahead of the list) rather than either field's meaning, since the expression is byte-identical across all four sites. - Document the new helper in hip.rst next to its siblings. - Add one regression test per site in test_hip_unit.py, each asserting the parse now raises instead of silently returning an empty list. - TransportFormatListParameter has no such two-octet field on the wire (RFC 7401's TRANSPORT_FORMAT_LIST has nothing between Length and the list), so its previously-benign default (empty-list, Length=0) case now fails at CONSTRUCT instead of RECONSTRUCT; updated its EXPECTED_FAILURES entry in test_option_roundtrip_unit.py to match, with the pre-existing wrong-offset defect noted as out of scope here. Build: brazil-build n/a (public repo); full suite at faf86d2 before this change is 4 failed/995 passed/17 skipped (1016 collected), and 0 failed/999 passed/17 skipped after, both with freshly regenerated fixtures matching the code under test.
) - Correct a regression in the fix itself: TransportFormatListParameter has no two-octet reserved/port field ahead of its list -- RFC 7401 S5.2.11 defines Length as literally "2x number of TF types", nothing else between Length and the list -- unlike NATTraversalModeParameter, ESPTransformParameter and HIPTransportModeParameter, which genuinely do and keep two_octet_prefix_list_len unchanged. Reusing that helper here turned the parameter's legitimate empty-list encoding (Length=0, formats=[]) into a raise, converting a working main case into a failure. - Add transport_format_list_len(): sizes the list at Length exactly, with the same floor-and-raise discipline for a direct, bypassing construction call, even though real wire bytes (an unsigned len) can never underflow it. - The old -2 also silently under-read every non-empty list by two octets on parse; pinned with a new regression test, since it predates this PR and is not limited to the empty-list case. - EXPECTED_FAILURES['hip-parameter/TRANSPORT_FORMAT_LIST'] rejoins the sixteen-entry tuple/list RECONSTRUCT group now that CONSTRUCT/PARSE succeed again, confirmed by re-running the round-trip suite rather than assumed. - Measured separately, not fixed here: item_type=EnumField(length=1) sizes each TF type entry at one octet, but RFC 7401 S5.2.11's own diagram and "Length = 2x number of TF types" both say two, and the high-level maker already computes len=2*len(tf_type) assuming two-octet entries. Confirmed by construction crashing (struct.error) for any real Enum_Parameter value (all HIP parameter type numbers exceed 255) -- a real, separate defect, reported rather than folded in. Build: full suite regenerating fixtures fresh for pre- and post-correction trees (fixture generation itself depends on hip.py); both show 0 failed at the top level because EXPECTED_FAILURES was kept in lockstep with the regression -- the substance is in the new empty-list and full-length tests.
…w' into fix-463-hip-list-length-underflow
…463) - transport_format_list_len()'s own docstring quoted RFC 7401 S5.2.11's "Length = 2x number of TF types" while formats' item_type stayed at EnumField(length=1, ...): an internally inconsistent fix. TF type values are HIP parameter type numbers (2050-4095 per the RFC), which need two octets; item_type=EnumField(length=2, ...) now matches, and matches HIPTransportModeParameter's mode field, the existing two-octet-item precedent in this file. - The two defects were not independent: _make_param_transport_format_list already computes len=2*len(tf_type), so with the length fix alone and a one-octet item_type, a two-entry list's len=4 read back as four entries (two spurious trailing zeros) instead of two -- confirmed before this commit, and confirmed gone after it, for one, two and three entries plus a case with two real TF types over 255. - Add a round-trip test that goes through the public maker (_make_param_transport_format_list) rather than a hand-built schema with a self-consistent len, since a hand-built case cannot expose a maker/schema disagreement. Includes ESP_TRANSFORM (4095) and HIP_TRANSPORT_MODE (7680), both within the RFC's TF type range and both values a one-octet item_type cannot pack (struct.error) -- so the one-octet assumption cannot return silently. - Rewrote the existing full-declared-length test's raw bytes for two-octet entries, and its docstring to name the item-width defect explicitly. Build: full suite green (0 failed, 1012 passed, 17 skipped, 1557 subtests, 1029 collected). EXPECTED_FAILURES unchanged -- the round-trip generator's own TRANSPORT_FORMAT_LIST case uses an empty formats list, unaffected by item width -- confirmed by running tests/protocols/test_option_roundtrip_ unit.py, not by assumption. examples/captures/options-internet.pcap is byte-for-byte unchanged (same reason).
Review of #466 (
|
formats in |
PR head (1fc3860c0) |
main (fa128959e, pre-#463) |
|---|---|---|
[] |
len=0 -> [] |
len=0 -> [] |
[10] |
len=2 -> [10] |
len=2 -> [] (entry lost) |
[10,20] |
len=4 -> [10,20] |
len=4 -> [10,20] (ok by accident) |
[10,20,30] |
len=6 -> [10,20,30] |
len=6 -> [10,20,30,0] (spurious trailing 0) |
[ESP_TRANSFORM(4095), HIP_TRANSPORT_MODE(7680)] |
len=4 -> [4095,7680] |
struct.error: 'B' format requires 0 <= number <= 255 |
This exactly matches Jarry's own pre-measured table — independently reproduced, not assumed.
options-internet.pcap unaffected by the item-width fix specifically (not just "unchanged from main"): generated the fixture from revision 2's hip.py (e62a24095, item_type=length=1) and from 1fc3860c0 into separate scratch dirs via options.generate(dest=...). Both hash c2c9f8dd807d6011745bd5d1c1196275. Confirms the PR's claim that the generator's own TF-list case is empty and so the item-width fix doesn't touch this fixture.
Full suite (PYTHONSAFEPATH=1, forced PYTHONPATH, fixtures regenerated via examples/generators/make_samples.py, .venv/bin/python 3.14.7, pcapkit.__file__ asserted to point at this worktree):
--collect-only: 1029 collected (matches claim).- Full run: 0 failed, 1012 passed, 17 skipped, 1557 subtests passed in 875.5s (matches claim exactly).
tests/protocols/test_option_roundtrip_unit.pyalone: 7 passed, 363 subtests passed (matches claim).mypy pcapkit/protocols/schema/internet/hip.py:Success: no issues found in 1 source file(matches claim).
Independent re-check of the item_type revert (the methodological safeguard the PR itself calls out): edited hip.py:923 from length=2 back to length=1, ran tests/protocols/internet/test_hip_unit.py, restored the line, confirmed git diff clean afterward. Result: exactly two named tests fail —
test_hip_transport_format_list_parameter_parses_the_full_declared_length and test_hip_transport_format_list_parameter_round_trips_through_the_maker (3 sub-tests, struct.error: 'B' format requires 0 <= number <= 255 for the >255 cases) — matching the PR's claim precisely.
CI: gh pr checks 466 — all green (Analyze, CodeQL, all Compat/Integration/Python legs 3.10–3.15, deploy-pages). Gate (full suite, Python 3.14) and Docs test gate show skipping (conditional jobs, not failures — confirmed by final exit code 0, no pending/failing checks remain). pyup.io/safety-ci (the blank-rollup StatusContext entry) reports pass, "No dependencies with known security vulnerabilities."
Findings
1. Two octets per TF type is correct — RFC 7401 §5.2.11 confirmed above, verbatim. transport_format_list_len (hip.py:251-304) and item_type=EnumField(length=2, ...) (hip.py:923) match the RFC exactly, and match HIPTransportModeParameter.mode's existing precedent (hip.py:1054-1057).
2. The other three sites' length callback is correct, but two of the three have a pre-existing item-width bug the PR doesn't touch. two_octet_prefix_list_len (hip.py:219-248) is correctly applied to all three, and the "2 octets consumed by reserved/port" accounting is right. But checking each site's item width against its RFC, as asked:
NATTraversalModeParameter.modes(hip.py:567-570): RFC 5770 §5.4 Figure 6 (raw text) —Mode ID is 16 bits (paired with the 16-bit| Reserved | Mode ID #1 | | Mode ID #2 | Mode ID #3 |Reservedfield in the same 32-bit row). The code declaresitem_type=EnumField(length=1, ...)(hip.py:569) — one octet.ESPTransformParameter.suites(hip.py:939-942): RFC 7402 §5.1.2 (raw text) —Same shape, Suite ID is 16 bits. The code declares| Reserved | Suite ID #1 | | Suite ID #2 | Suite ID #3 |item_type=EnumField(length=1, ...)(hip.py:941) — one octet.HIPTransportModeParameter.mode(hip.py:1054-1057) is the control: RFC 6261 §3.1 also shows 16-bit Mode IDs, and the code correctly usesEnumField(length=2, ...)(hip.py:1056).
Demonstrated through the public makers (not a hand-built schema):
_make_param_nat_traversal_mode(modes=[UDP_ENCAPSULATION])
schema.len = 4, but bytes(schema) is 11 octets, not the 8 the len/padding math assumes
packed.hex() = 0260000400000100000000
unpack(packed).modes == [UDP_ENCAPSULATION, Reserved_0] # 1 entry in, 2 out
_make_param_esp_transform(suites=[AES_CCM_8])
same shape: 1 entry in -> [AES_CCM_8, RESERVED_0] out, 2 in -> 4 out, 3 in -> 6 out
Feeding the corrupted encoding through the full parser is worse than a wrong count:
HIP(fixed_header + packed_nat_traversal_mode_param, ..., extension=True)
# -> AttributeError: 'NATTraversalModeParameter' object has no attribute 'modes'This is the same class of bug #466 fixes for TransportFormatListParameter (RFC-specified 16-bit entries, code using 8-bit EnumField), left in place at two of the three sites the PR calls "genuine" and leaves unmodified. It pre-dates #466: confirmed byte-identical on origin/main (fa128959e) — this PR only changed the length= callback reference for these two sites, never touched item_type. The existing unit tests for both makers (test_hip_unit.py:1034-1038, 1440-1450, 1597-1606) only assert against the constructed schema object's own .modes/.suites Python attribute, never pack it and unpack it — the exact blind spot #466's revision 3 says let the TransportFormatListParameter item-width bug "survive review once already" (per the PR body). This is out of scope for #463 (which is specifically about the -2 length underflow, not item width) and not introduced by #466, so it does not block this PR — but it is real, reproducible, and the same class of defect the reviewer was asked to rule out here.
3. Item_type revert reproduces the claimed regressions exactly — see "What I ran" above. Confirmed independently, not assumed.
4. FieldValueError floor in transport_format_list_len is genuinely unreachable from the wire or the maker. Parameter.len is a UInt16Field (__template__ = 'H', unsigned) — struct.unpack('H', ...) never yields a negative value, so no wire byte sequence can trigger it. _make_param_transport_format_list computes len=2*len(tf_type), always ≥ 0, so the public maker can't trigger it either. Only a direct TransportFormatListParameter(type=..., len=-1, ...) construction bypassing both paths can. The docstring's claim (hip.py:292-298) is accurate. Worth keeping anyway, for the same reason the module's other three guards (registration_type_list_len, reg_info_list_len, two_octet_prefix_list_len) keep theirs: consistency of discipline across the module, at negligible cost.
5. The RECONSTRUCT bucketing is correct and the harness's own guard is satisfied. hip-parameter/TRANSPORT_FORMAT_LIST sits back in the 16-entry RECONSTRUCT/"unsupported type <class 'tuple'>" group (test_option_roundtrip_unit.py:378-389), alongside its siblings including NAT_TRAVERSAL_MODE, ESP_TRANSFORM, HIP_TRANSPORT_MODE. Running the suite (test_round_trip_is_identity_or_a_recorded_gap, test_option_roundtrip_unit.py:836-871) confirms no mismatch: if the code had stopped matching this recorded gap (e.g., if it now returned 'OK' or a different status/detail), the test would fail on assertEqual(outcome.status, gap.status, ...). It passed clean (7/7, 363 subtests), so the guard is doing its job here.
6. Minor, non-blocking: the PR's own new test (test_hip_transport_format_list_parameter_round_trips_through_the_maker) uses Parameter.HIP_TRANSPORT_MODE (7680) as one of its ">255" TF-type cases, but RFC 7401 §5.2.11 states TF type numbers run 2050–4095; 7680 is outside that documented range (still a valid 16-bit HIP parameter type, and the field doesn't enforce the narrower range, so this doesn't affect correctness — just an imprecise choice of test value).
Verdict
Within its stated scope — the pkt['len'] - 2 underflow guard for four sites, and the item-width correction for TransportFormatListParameter that revision 3 adds — every claim in #466 checks out against independent measurement: the RFC citation, the before/after round-trip table, the full-suite numbers, the mypy result, the item_type-revert safeguard, and the RECONSTRUCT bucketing. Finding 2 above (item-width bug in NATTraversalModeParameter.modes/ESPTransformParameter.suites) is real and reproducible, but it pre-dates this PR, is outside #463's stated scope (length underflow, not item width), and is not introduced or worsened by 1fc3860c0 — it belongs in its own follow-up rather than blocking this one.
GOOD TO MERGE, sha 1fc3860c0.
1 participant
Summary
Closes #463.
Four HIP parameters computed their
ListFieldlength aspkt['len'] - 2with no lower bound, on a wire-controlledUInt16Field. A peer declaringLength = 0drove that to-2;ListField's ownwhile length > 0loop returns an empty list for a negative length rather than raising, so the malformed parameter parsed "successfully" to an empty list with no exception at all — silent, and arguably worse than thestruct.erroraBytesFieldsite would raise.Re-derived sites at
faf86d26b(line numbers moved since the issue was filed againstda2422728):pcapkit/protocols/schema/internet/hip.py:504—NATTraversalModeParameter.modespcapkit/protocols/schema/internet/hip.py:858—TransportFormatListParameter.formatspcapkit/protocols/schema/internet/hip.py:876—ESPTransformParameter.suitespcapkit/protocols/schema/internet/hip.py:991—HIPTransportModeParameter.modeHIPTransformParameter.suitesandHITSuiteListParameter.suitesare not affected — they use a barepkt['len']with no subtraction and cannot underflow. (The issue itself flags and rules these two out; not re-touched here.)Revision history on this PR
Revision 1 applied one shared helper,
two_octet_prefix_list_len, to all four sites uniformly. Wrong forTransportFormatListParameter, caught in review.Revision 2 gave
TransportFormatListParameter.formatsits own length callback,transport_format_list_len, sizing the list atLengthexactly (no-2) — RFC 7401 Section 5.2.11 definesLengthas literally "2x number of TF types," with nothing betweenLengthand the list, unlike the other three sites' genuine two-octetreserved/portprefix. This also fixed a second, independent pre-existing bug: the old-2silently under-read every non-empty list by two octets on parse.Revision 3 (current) fixes an inconsistency revision 2 introduced.
transport_format_list_len's own docstring quoted the RFC's "2x number of TF types," which fixes eachTF typeentry at two octets — butformats'item_typestayed atEnumField(length=1, ...). Review caught this before merge: the length fix and the item width are not independent._make_param_transport_format_listalready computeslen=2 * len(tf_type), assuming two-octet entries; withtransport_format_list_lenreturningLengthunchanged and a one-octetitem_type, a maker-built two-entry list (len=4) read back as four entries — two spurious trailing zeros — rather than the two that went in. (Onmain, before any of this PR, the two pre-existing defects partially and only-sometimes cancelled:len - 2with one-octet items happened to read back the right count for exactly two entries, by accident, not by design — three entries already broke it.)The fix now:
two_octet_prefix_list_lenunchanged for the three genuine sites (NATTraversalModeParameter,ESPTransformParameter,HIPTransportModeParameter).TransportFormatListParameter.formatsusestransport_format_list_len(returningLength, unchanged from revision 2) together withitem_type=EnumField(length=2, namespace=Enum_Parameter), matchingHIPTransportModeParameter'smodefield — the established two-octet-item shape already in this file — and matching the RFC diagram (two 16-bitTF typefields per 32-bit row).struct.error: 'B' format requires 0 <= number <= 255that a one-octetitem_typeraised for any real TF type, since valid values (HIP parameter type numbers, per the RFC's own text) run 2050–4095, all over 255._make_param_transport_format_list) rather than a hand-built schema with a self-consistentlen, since a hand-built case cannot expose a maker/schema disagreement — which is exactly the gap that let the one-octet item width through review once already.EXPECTED_FAILURES['hip-parameter/TRANSPORT_FORMAT_LIST']and the generatedoptions-internet.pcap: both unchanged, because the round-trip generator's own case for this parameter uses an emptyformatslist, which the item-width fix does not touch.Following #460's precedent
PR #460 (already on
main) fixed the identical shape forRegInfoParameter.reg_info, replacing the offending lambda with a named function raisingFieldValueError(frompcapkit.utilities.exceptions), following the module's ownmpl_opt_seed_id_lenconvention, and named itreg_info_list_len.This PR follows the same shape.
two_octet_prefix_list_lencovers the three sites whosepkt['len'] - 2is genuinely byte-identical;transport_format_list_lencovers the fourth, which needed both a different length expression and a different item width once the wire format was actually read rather than assumed. Both are documented inhip.rstnext toregistration_type_list_lenandreg_info_list_len.Before/after evidence
Four regression tests in
test_hip_unit.pyfor the length-underflow guard, each failing before withAssertionError: FieldValueError not raisedand passing after. TwoTransportFormatListParameter-specific tests (empty-list-at-Length-zero, full-declared-length survives) plus a third added in revision 3 that round-trips through the maker with real TF types over 255 — that third test (and the full-declared-length test) fails withstruct.error: 'B' format requires 0 <= number <= 255or a wrong entry count against a one-octetitem_type, and passes against the two-octet fix.Full suite,
PYTHONSAFEPATH=1plus explicitPYTHONPATH=<worktree root>(needed for direct script execution ofexamples/generators/make_samples.py, which otherwise resolvespcapkitvia the editable-install finder to a different checkout — confirmed viasys.meta_pathorder and the finder's hardcodedMAPPING), interpreter.venv/bin/python3.14.7,pcapkit.__file__confirmed pointing at this worktree throughout, fixtures regenerated fresh for each side since generation itself depends onhip.py's behavior:faf86d26b, pre-Four more HIP parameters underflow pkt['len'] - 2 and silently return an empty list #463):--collect-only→ 1016 collected; full run → 4 failed (exactly the 4 new regression tests), 995 passed, 17 skipped, 1547 subtests passed.main(fa128959e):--collect-only→ 1029 collected; full run → 0 failed, 1012 passed, 17 skipped, 1557 subtests passed.One methodological note, worth carrying forward: because
EXPECTED_FAILURESwas kept in lockstep with the code at every revision, the top-level pass/fail counts alone could not distinguish a real fix from a disguised regression at any of the three revisions — revision 2's still-live item-width bug also showed 0 failures at the suite level. The dedicatedTransportFormatListParametertests going through the actual public construction API, not the aggregate count, are what prove correctness here.mypyon the touched source file:Success: no issues found in 1 source file.Test plan
AssertionError: FieldValueError not raised) and passes afterTransportFormatListParameter: empty-list-at-Length-zero (no raise), full-declared-length-survives-parse (no silent drop, two-octet entries), and a maker-based round trip covering 0/1/2/3 entries plus two real TF types over 255transport_format_list_len/two_octet_prefix_list_lenunit assertions intest_hip_schema_selectors_and_encrypted_parameter_branchestests/protocols/test_option_roundtrip_unit.pyrun before/after every revision;EXPECTED_FAILURES['hip-parameter/TRANSPORT_FORMAT_LIST']rejoins the sixteen-entry tuple/list group and stays there through the item-width fix, confirmed by running the suite each time, not by predictionexamples/captures/options-internet.pcapconfirmed byte-for-byte unchanged by the item-width fixmypyclean onpcapkit/protocols/schema/internet/hip.py🤖 Generated with Claude Code