Skip to content

fix(internet)!: refuse or rebuild exactly an MH mobility option changed in one octet (#2684) - #2700

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2684-mh-option-roundtrip
Nov 2, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/2684-mh-option-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Nov 2, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

MH mobility options that did not round-trip after a one-octet change now either rebuild byte for byte or are refused. A refused option is kept as Raw inside IPv6, which rebuilds exactly and logs nothing.

  • Length mismatch: a shared check refuses an option or sub-option whose fields don't take exactly its declared length. This covers MN-ID length 0, LCMP and ANI sub-option lengths, CGA extensions, and retyped options 13→12 and 42→0.
  • Prefix length: options 30, 36, 37 and 55 check it against the field's width only (6 or 8 bits). A parsed, hand-built and keyword value are treated alike, so anything that parses also builds.
  • Text identifiers: Service Selection and MN-ID NAI count the octets they write back, not re-encoded text.
  • QoS: the 2 reserved traffic-class bits are kept, in a new tc_reserved field.

Breaking: QualityOfServiceOption gains the required field tc_reserved.

  • CGA Parameters: the key's DER long-form length is decoded, so a real 1024-bit RSA key now parses. A key or fixed fields that run past the option are refused before they over-read.

The tests include a one-octet mutation sweep over every option code MH.__option__ knows, and it fails on main. Shipped captures parse identically to main, apart from the QoS frame gaining tc_reserved: 0.

Closes #2684

@JarryShaw JarryShaw added this to the PyPCAPKit v1.6 milestone Nov 2, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: running A cross-review is in flight against the current head - no verdict yet labels Nov 2, 2026
@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Nov 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 87a01720f, from a Sonnet cross-review. The shared length check, the text identifiers, tc_reserved and the CGA long form are confirmed. Every cut inside IPv6 still rebuilds through the built-in short read, and the sweep shows no mismatches. Two changes:

  1. The prefix-length exemption lets a caller-built model through for options 30, 36, 37 and 55, which is refactor(protocols)!: refuse fixed-size octets and timestamp counts the makers masked (#2676) #2691's defect class: a hand-built model with a prefix length of 40 or 63 is written, and option 55 accepts 200. Check the prefix length only against the wire width, so whatever parses also builds and no exemption is needed. If the data model cannot hold such a value, refuse it on parse, kept quiet as Raw.
  2. tc_reserved is a new required field, which breaks direct construction of the QoS model. Give it a default of 0, or mark the PR breaking with a ! subject.

…ed in one octet (#2684)

- Option fields were read at their own widths whatever the length said, so a
  length they overran or fell short of read octets that were not the option's
  and rebuilt another. A mobility option, FID, ANI or LCMP sub-option, QoS
  attribute or CGA extension whose fields do not take its declared length is
  refused, and so is a target care-of address neither IPv4 nor IPv6, as BID's is.
- A CGA public key takes the long form of a DER length, and a CGA parameter
  whose fixed fields or key run past its option is refused in the schema.
- The IPv4 Address Acknowledgement, IPv4 Home Address Request and Reply, and
  Delegated MNP makers check a prefix length against its field width alone, so
  one past the address that the reader keeps no longer refuses to rebuild.
- The Service Selection and MN-ID NAI makers count the octets the identifier
  packs as, rather than its text re-encoded or its characters.
- The QoS option keeps the two reserved traffic-class bits as tc_reserved, a
  new required field of its data model, which a direct construction now has to
  pass: the breaking change.
- Adds a one-octet mutation sweep over every registered mobility option.

New tests fail on be83061 without the pcapkit/ change; MH and guard modules pass.
@JarryShaw

Copy link
Copy Markdown
Owner Author

The fix round is ready at faa02c75e. Its push is held because the Actions queue is at 56, and fix rounds wait for fewer than 40.

@JarryShaw
JarryShaw force-pushed the fix/2684-mh-option-roundtrip branch from 87a0172 to faa02c7 Compare November 2, 2026 23:10
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Nov 2, 2026
@JarryShaw JarryShaw changed the title fix(internet): refuse or rebuild exactly an MH mobility option changed in one octet (#2684) fix(internet)!: refuse or rebuild exactly an MH mobility option changed in one octet (#2684) Nov 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at faa02c75e, rebased onto be8306140. I checked the delta myself.

  • Prefix length: the data-model exemption is gone. Options 30, 36, 37 and 55 check the prefix length against its field width only, so hand-built and parsed models are treated alike. A new test pins that.
  • tc_reserved: it stays a required field, because data models generate constructors with no defaults. The PR is therefore breaking, with a ! subject.

@JarryShaw
JarryShaw merged commit 92ecf2f into main Nov 2, 2026
12 checks passed
@JarryShaw
JarryShaw deleted the fix/2684-mh-option-roundtrip branch November 2, 2026 23:11
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 2, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 2, 2026
JarryShaw added a commit that referenced this pull request Nov 2, 2026
…s, the cut schema lists, the TSO checksum cut and short ZigBee keys into 1.6.0 (#2449)

- Breaking: MH (#2700) refuses a mobility option, or a sub-option, QoS
  attribute or CGA extension of one, whose fields do not take its length,
  where 1.5.x rebuilt another; QualityOfServiceOption gains a required
  tc_reserved. Prefix lengths are held to their field's width, the NAI and
  service identifier makers count octets, and a long-form DER CGA key
  parses. Folded into the MH/DSS and makers' range-check entries, with a
  QoS entry of its own; old and new values measured against v1.5.1.
- Fixed: ListField zero-fills a cut list of schema items, bounded by
  ZERO_PAD_BUDGET_RATIO with no free band, and a SACK past the data raises
  ProtocolError where it raised FieldValueError (#2701); a Decryption
  Secrets Block made from a short parsed ZigBee key is written as captured,
  and zigbee_key_held's schema holds the octets given (#2697).
- Folded into unreleased entries: radiotap's mixed bitmaps, their
  known-bits registries and BitmapField (#2696), into the Radiotap entry;
  the TSO and AH checksum cut (#2699), into the checksum and contextvars
  entries; the zero-filled SETTINGS and IGMPv3 records and the IGMPv3
  Additional Data rebuild (#2701), into the HTTP/2 SETTINGS and IGMP
  entries, dropping HTTPv2.make's unreleased frame_length.
- No entry for the docstring-and-test-only be83061 (#2695).
- CHANGELOG.md regenerated; util/changelog_md.py --check passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) bug Issues reporting a defect (set by the bug report template; a default, not an assessment)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(internet): MH mobility options that do not round-trip after a one-octet change

1 participant