Skip to content

protocols: floor CALIPSO, MPL and REG_INFO's wire-derived lengths at zero - #460

Merged
JarryShaw merged 2 commits into
mainfrom
fix-455-length-underflow
Sep 18, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix-455-length-underflow

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Summary

Closes #455.

Three more wire-derived length expressions shared the unguarded-underflow shape #438/#449 fixed elsewhere: a crafted or truncated option drives a subtraction negative, and the negative length reaches struct as a format like '-1s'.

The three sites (re-derived against da2422728, since #449 shifted the line numbers the issue quoted):

  • pcapkit/protocols/schema/internet/hopopt.py:432 and pcapkit/protocols/schema/internet/ipv6_opts.py:432 -- CALIPSOOption.pad, pkt['len'] - 8 - pkt['cmpt_len'] * 4
  • pcapkit/protocols/schema/internet/hopopt.py:696 and pcapkit/protocols/schema/internet/ipv6_opts.py:701 -- MPLOption.pad, pkt['len'] - 2 - <Seed-ID length>
  • pcapkit/protocols/schema/internet/hip.py:733 -- RegInfoParameter.reg_info, pkt['len'] - 2

Two distinct symptoms:

  • CALIPSO and MPL's PaddingField reaches struct.calcsize with a negative length and raises a bare struct.error: bad char in struct format.
  • RegInfoParameter.reg_info is a ListField, whose while length > 0 loop returns an empty list on a negative length with no exception at all -- a malformed REG_INFO parses "successfully" to reg_type=().

The fix follows the precedent PR #449 set for smf_i_dpd_id_len: a named function per module (since hopopt.py and ipv6_opts.py don't import from each other) that raises FieldValueError from pcapkit.utilities.exceptions when the computed length would go negative.

  • calipso_pad_len and mpl_opt_pad_len, added to both hopopt.py and ipv6_opts.py, replacing the identical duplicated lambdas each file carried for CALIPSOOption.pad and MPLOption.pad.
  • reg_info_list_len, added to hip.py, mirroring registration_type_list_len (added by protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449 for the sibling REG_REQUEST/REG_RESPONSE/REG_FAILED parameters, which read a single lifetime octet where REG_INFO reads min_lifetime + max_lifetime, hence the different offset).

Test plan

  • tests/protocols/internet/test_ipv6_extension_unit.py: added _assert_calipso_option_rejects_underflowing_length and _assert_mpl_option_rejects_underflowing_length, each exercised against both HOPOPT and IPv6_Opts. Confirmed failing on the pre-fix tree with struct.error: bad char in struct format (from pcapkit/corekit/fields/field.py:109), and passing after the fix with FieldValueError.
  • tests/protocols/internet/test_hip_unit.py: added test_hip_reg_info_parameter_rejects_underflowing_length, mirroring the existing test_hip_registration_parameters_reject_underflowing_length (protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449/A wire-derived length underflows into a struct format string, raising bare struct.error on untrusted input #438). Confirmed failing on the pre-fix tree with AssertionError: FieldValueError not raised (i.e. the silent reg_type=() case), and passing after the fix.
  • tests/protocols/test_option_roundtrip_unit.py: ran explicitly per the coordination note in the issue -- 7 passed, 299 subtests passed, no EXPECTED_FAILURES entry flipped (CALIPSO/MPL have none; REG_INFO's existing entry is an unrelated RECONSTRUCT/tuple-vs-list gap, untouched by this change).
  • mypy clean on all three modified schema modules.
  • Full suite, same fixture state both sides (examples/generators/make_samples.py regenerated once): baseline da2422728 collects 994 tests; with this branch's 5 new tests, 999 collected, 982 passed, 17 skipped, 0 failed.

🤖 Generated with Claude Code

…zero (#455)

Three more wire-derived length expressions shared the unguarded-underflow
shape #438/#449 fixed elsewhere -- a crafted or truncated option drives the
subtraction negative and it reaches struct as a format like '-1s'.

- CALIPSOOption.pad (hopopt.py, ipv6_opts.py): `len - 8 - cmpt_len * 4` could
  go negative and hit struct.calcsize as a bare struct.error. Added
  calipso_pad_len, raising FieldValueError below zero, replacing the
  duplicated lambda in both files.
- MPLOption.pad (hopopt.py, ipv6_opts.py): same crash shape for
  `len - 2 - <Seed-ID length>`. Added mpl_opt_pad_len analogously.
- RegInfoParameter.reg_info (hip.py): `len - 2` fed a ListField, whose
  `while length > 0` loop silently returns [] on a negative length instead
  of raising -- a malformed REG_INFO parsed "successfully" with no
  diagnostic. Added reg_info_list_len, mirroring #449's
  registration_type_list_len for REG_REQUEST/RESPONSE/FAILED.

Follows #449's precedent for smf_i_dpd_id_len: a named function per module,
since hopopt.py and ipv6_opts.py don't import from each other.

Tests cover both symptoms distinctly and are confirmed failing on the
pre-fix tree (struct.error for CALIPSO/MPL, silent reg_type=() for
REG_INFO). Full suite: 982 passed, 17 skipped (999 collected, baseline 994
at da24227); mypy clean.
Comment thread pcapkit/protocols/schema/internet/hopopt.py
Comment thread pcapkit/protocols/schema/internet/hip.py
@JarryShaw

Copy link
Copy Markdown
Owner Author

Standing in for Copilot on this one (out of tokens). Reviewed at head fe98da602bb23537763896ec3b24072d7b03e46b, 0 commits behind origin/main (da2422728) per git rev-list --count HEAD..origin/main. All code read via git show <ref>:<path> against a fetched refs/pull/460/head confirmed equal to the stated head sha, never the ambient working tree.

CI

All 23 checks settled, no red: 21 SUCCESS (Analyze, CodeQL, deploy-pages, Compat Python {3.10..3.15}, Python {3.10..3.15}, Integration Python {3.10..3.15}), 2 SKIPPED (Docs test gate, Gate (full suite, Python 3.14)), 0 pending, 0 failing.

The three claimed sites, verified at this sha

  • pcapkit/protocols/schema/internet/hopopt.py:435 and ipv6_opts.py:435 -- CALIPSOOption.pad (the PR body says :432; that's actually the lambda pkt: pkt['cmpt_len'] > 0, line two above it -- left an inline nit, doesn't affect the fix).
  • hopopt.py:696 and ipv6_opts.py:701 -- MPLOption.pad. Matches the PR body exactly.
  • hip.py:733 -- RegInfoParameter.reg_info. Matches the PR body exactly.

Reproduction, both directions (schema files reverted to da2422728, PR's tests kept)

  • CALIPSO/MPL, pre-fix: the issue's repro (HOPOPT(bytes.fromhex('3b0007000000000000000000000000'), 16)) raises bare struct.error: bad char in struct format at pcapkit/corekit/fields/field.py:109, confirmed live. Post-fix: FieldValueError: HOPOPT: invalid CALIPSO option length: 0, confirmed live.
  • RegInfoParameter, pre-fix: the new test_hip_reg_info_parameter_rejects_underflowing_length fails with AssertionError: FieldValueError not raised -- i.e. it silently parsed rather than crashing or raising, exactly the claimed second symptom. Post-fix: passes, FieldValueError: HIP: invalid parameter length: 0.
  • All 5 new tests: 5 failed against pre-fix schema files, all 5 pass against the PR's schema files.

Precedent and offset claims

Test counts, measured myself

  • tests/protocols/test_option_roundtrip_unit.py at this sha: 7 passed, 299 subtests passed, matching the PR's claim exactly. hip-parameter/REG_INFO's entry is a RECONSTRUCT/"unsupported type <class 'tuple'>" gap at pcapkit/protocols/schema/schema.py:624, grouped with 15 other parameters under the same tuple-vs-list issue -- unrelated to length underflow, and the file isn't even in this PR's diff, so it can't have flipped.
  • Baseline at da2422728 (measured myself, not taken on faith): pytest --collect-only994 tests. At fe98da602: 999 collected, full run → 982 passed, 17 skipped, 0 failed. All three numbers match the PR body exactly.
  • mypy pcapkit/protocols/schema/internet/{hopopt,ipv6_opts,hip}.py at this sha: clean, 0 errors. Full-project mypy pcapkit at this sha: 124 errors in 40 files (496 source files checked) -- same count the task's calibration gives for the da2422728 baseline, and none of the 124 are in the three modified files (the two ipv6_opts.py/hopopt.py hits in that list are the unrelated pcapkit/protocols/internet/{hopopt,ipv6_opts}.py, not the schema/internet/ ones this PR touches). Net new mypy errors: 0.

The four out-of-scope hip.py ListField candidates

Assessed each individually rather than taking the "identical shape" framing on faith, and it holds up -- all four genuinely underflow the same way, confirmed by construction against main (da2422728, unmodified by this PR):

Parameter Field Line (main) Consumed before list Crafted Length=0 result
NATTraversalModeParameter modes 454 reserved: PaddingField(length=2) no exception, mode_id=()
TransportFormatListParameter formats 808 (none -- first field, yet still -2) no exception, tf_type=()
ESPTransformParameter suites 826 reserved: PaddingField(length=2) no exception, suite_id=()
HIPTransportModeParameter mode 941 port: UInt16Field() no exception, mode_id=()

Each is a ListField(length=lambda pkt: pkt['len'] - 2, ...) against the same unconstrained wire-controlled Parameter.len: UInt16Field(), so each hits the identical while length > 0 silent-empty-list path this PR fixes for RegInfoParameter. TransportFormatListParameter.formats is odd in a different way worth separating out -- it subtracts 2 despite being the very first field in the schema with nothing structurally consumed before it -- but that's a question about whether -2 is the right constant, not about whether the expression can go negative; it can, on the same wire-controlled len.

Separately, I checked the two lines that don't belong to this list of four: hip.py:420 (HIPTransformParameter.suites) and hip.py:635 (HITSuiteListParameter.suites) use a bare length=lambda pkt: pkt['len'], no subtraction, so they cannot underflow the same way -- confirming that if any earlier characterization conflated those two line numbers with the four real candidates above, that conflation was wrong, but the four candidates themselves are real and share the exact shape.

Left this as an out-of-scope inline note rather than a blocker, per the PR's stated scope (the three sites #455 names) -- a follow-up issue mirroring this fix would be the right vehicle for the four.

Verdict

Every claim in the PR body checked out against direct reproduction and my own measurements, no test or type-check regressions, CI fully green, one-commit-per-file diff matches the stated intent. Nothing here blocks merge.

GOOD TO MERGE at fe98da602bb23537763896ec3b24072d7b03e46b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Three more wire-derived lengths underflow: CALIPSO and MPL padding crash in struct, RegInfoParameter silently returns empty

1 participant