mh: reject a wrong-type MN-ID identifier in-library, not as a stdlib leak by JarryShaw · Pull Request #481 · JarryShaw/PyPCAPKit

JarryShaw · 2026-09-18T17:49:57Z

Closes #469

Summary

_make_opt_mn_id documents identifier as bytes | str | IPv6Address | int,
but each MN-ID subtype's field (mn_id_selector) accepts only one of the first
two: a StringField for NAI, a BytesField for the other six. Handing the
wrong one -- or a value of neither type -- escaped as a bare stdlib exception
instead of a ProtocolError. #468 already closed the int half of this
family (#467); this PR closes the rest, per #469's own scoping ("only its
remaining half" -- the int half is untouched).

What changed

pcapkit/protocols/internet/mh.py::MH._make_opt_mn_id:

The IPv6_Address branch now wraps the ipaddress.IPv6Address(identifier)
conversion in try/except ValueError, re-raising as ProtocolError. This
catches ipaddress.AddressValueError (itself a bare ValueError) for any
identifier ipaddress.IPv6Address cannot parse, without swallowing the
sibling ProtocolError raised just above it for an out-of-range int.
A new elif subtype_val == Enum_MNIDSubtype.NAI branch rejects any
non-str identifier with a ProtocolError before the len() call that
used to leak TypeError/AttributeError.
The remaining else branch (the six octet subtypes) rejects any identifier
that isn't bytes/bytearray with a ProtocolError, for the same reason.

tests/protocols/internet/test_mh_unit.py gains
test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype, sweeping all
eight subtypes against every wrong type (including empty-value boundaries),
and asserting the matching type still works.

Measurements

Re-measured myself on e80c42217 (current main) before changing anything,
through the public maker, not a hand-built schema:

subtype	wrong identifier	before	after
`NAI`	`b'\x01\x02'`, `[1,2]`, `{'a':1}`	`AttributeError`	`ProtocolError`
`NAI`	`1.5`, `None`, `IPv6Address('::1')`	`TypeError`	`ProtocolError`
`NAI`	`b''` (empty)	`AttributeError`	`ProtocolError`
six octet subtypes	`'user@realm'`, `[1,2]`, `{'a':1}`	`struct.error`	`ProtocolError`
six octet subtypes	`1.5`, `None`, `IPv6Address('::1')`	`TypeError`	`ProtocolError`
six octet subtypes	`''` (empty), `memoryview(...)`	`struct.error`	`ProtocolError`
`IPv6_Address`	`b'\x01\x02'`, `'user@realm'`, `1.5`, `None`, `[1,2]`, `{'a':1}`, `b''`, `bytearray(16)`, `memoryview(16)`	`ipaddress.AddressValueError` (a `ValueError`)	`ProtocolError`

Correct-type identifiers are unaffected: str for NAI, bytes/bytearray
for the six octet subtypes, an IPv6Address/valid int/valid str for
IPv6_Address -- all still construct and pack, including the empty-value
boundary (NAI + '', the six + b''). The int half from #468 is
unaffected: valid ints still convert per subtype, and the negative/NAI/
>=2**128 rejections still raise exactly as before.

Revert-proof: reverting only mh.py (test file left in place) makes the new
test fail with 64 subtest failures across the wrong-type sweep; restoring the
fix makes it pass again.

Build: tests/protocols/internet/test_mh_unit.py -> 38 passed, 408
subtests passed. Full suite tests/ -> 1032 passed, 17 skipped, 1707 subtests
passed. mypy --config-file mypy.ini pcapkit/protocols/internet/mh.py -> 97
errors, down from a 98-error baseline on unmodified main (the one resolved
is this method's own len(identifier) "incompatible type... expected Sized"
complaint, now that isinstance narrows the union before each len() call);
the remaining 2 are pre-existing and unrelated to this method (lines outside
_make_opt_mn_id).

Judgement call: `bytearray`, `memoryview`, and ASCII `str`

bytearray is accepted alongside bytes for the six octet subtypes.
Measured: struct.pack('Ns', bytearray(...)) already round-trips
correctly today, so rejecting it would tighten behaviour that was never
broken, for no benefit.
memoryview is rejected even though it looks equally bytes-like.
Measured: struct.pack('Ns', memoryview(...)) raises
struct.error: argument for 's' must be a bytes object -- it does not
actually survive the pack call, so letting it through would just relocate
the leak rather than fix it.
A str that happens to be pure ASCII is not auto-encoded for the
octet subtypes, and a bytes that happens to decode cleanly is not
auto-decoded for NAI. Either direction reintroduces the same "accepts a
value that means the wrong thing" class of bug MH MN-ID: an int identifier cannot be packed for any subtype except IPv6_Address, and its declared length is wrong #467 removed for int,
just relocated to bytes/str.

Out of scope

Nothing further found in this half; no new issues filed.

…leak _make_opt_mn_id documents identifier as bytes | str | IPv6Address | int, but each subtype's field (mn_id_selector) accepts only one of the first two: a StringField for NAI, a BytesField for the other six. Handing the wrong one -- or a value of neither type -- escaped as a bare stdlib exception instead of a ProtocolError. #468 already closed the int half of this (#467); this closes the rest (#469). Measured through the maker on e80c422, all now raising ProtocolError instead of leaking: - NAI + bytes/list/dict -> was AttributeError ('bytes' object has no attribute 'encode') - NAI + float/None/IPv6Address -> was TypeError (no len()) - IMSI/P_TMSI/EUI_48_address/EUI_64_address/GUTI/DUID + str/list/dict -> was struct.error (argument for 's' must be a bytes object) - same six + float/None/IPv6Address -> was TypeError (no len()) - IPv6_Address + any of the above -> was ipaddress.AddressValueError, itself a bare ValueError, now caught before it can be confused with the sibling ProtocolError/FieldValueError raises in this method bytearray is accepted alongside bytes for the six octet subtypes: it already round-trips correctly through struct.pack('Ns', ...) (measured), so rejecting it would tighten behaviour that was never broken. memoryview looks equally bytes-like but does not survive that same pack call (measured: same struct.error), so it is rejected with everything else. A str that is ASCII-only is not auto-encoded for the octet subtypes, and bytes is not auto-decoded for NAI: either would reintroduce the same "accepts a value that means the wrong thing" class of bug #467 removed for int, just relocated to bytes/str. test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype sweeps every subtype against every wrong type, including empty-value boundary cases, and is confirmed to fail against the pre-fix code (64 subtest failures) before passing against the fix. Build: tests/protocols/internet/test_mh_unit.py -> 38 passed, 408 subtests passed. Full suite (tests/) -> 1032 passed, 17 skipped, 1707 subtests passed. mypy --config-file mypy.ini pcapkit/protocols/internet/mh.py -> 97 errors (down from 98 baseline; the one resolved was this method's own len(identifier) Sized-type complaint), remaining two pre-existing and unrelated to this method.

JarryShaw · 2026-09-18T18:30:38Z

Review at head 37f026846 (verified: git rev-parse HEAD after git checkout --detach FETCH_HEAD on refs/pull/481/head returned 37f0268468894b7d760403a143f4a47af375b361, matching headRefOid exactly — the worktree had started on main/e43ad4354 and I re-detached before measuring anything).

Scope of the diff

git diff --stat main...HEAD (three-dot, merge-base e80c42217): only pcapkit/protocols/internet/mh.py (+72/-3) and tests/protocols/internet/test_mh_unit.py (+112). No other files touched.

CI

Waited it out rather than judging a partial rollup. Final state, gh pr view 481 --json statusCheckRollup:

CheckRun  Docs test gate                 SKIPPED
CheckRun  Gate (full suite, Python 3.14) SKIPPED
CheckRun  (all 21 others: Analyze, Compat/Integration/Python 3.10-3.15, deploy-pages, CodeQL) SUCCESS
StatusContext pyup.io/safety-ci          SUCCESS

21 SUCCESS + 2 SKIPPED-by-design + 1 StatusContext SUCCESS = fully green.

Independent verification of the PR's own measurements

All run myself in this worktree, pcapkit.__file__ asserted to start with the worktree root before every run, fixtures generated first via PYTHONPATH=<worktree root> .venv/bin/python examples/generators/make_samples.py.

mypy, python -m mypy --config-file mypy.ini pcapkit/protocols/internet/mh.py: 98 errors on merge-base e80c42217 (detached, ran, then returned to PR head) → 97 errors on 37f026846. The one that disappeared is exactly the claimed one — merge-base has mh.py:7767: error: Argument 1 to "len" has incompatible type "bytes | str | IPv6Address"; expected "Sized", which is gone at the PR head; the remaining 2 (mh.py:7604 prefix arg-type, mh.py:7971 CGA-extensions list-invariance) are both outside _make_opt_mn_id and present at both revisions.
Unit file: pytest tests/protocols/internet/test_mh_unit.py -q → 38 passed, 8 warnings, 408 subtests passed in 37.40s. Matches exactly.
Full suite: pytest tests/ -q → 1032 passed, 17 skipped, 11137 warnings, 1707 subtests passed in 872.08s. Matches exactly.
Revert-proof, run myself (copied mh.py out, overwrote it with git show e80c42217:pcapkit/protocols/internet/mh.py, left the new test file in place, ran only the new test, then restored the PR-head mh.py byte-for-byte and re-ran to confirm a clean git status): reverted → 64 failed, 1 passed, 12 subtests passed; restored → 1 passed, 76 subtests passed. The 64-failure figure matches the PR description exactly.

Attack 1 — the `except ValueError` ordering (mh.py:7717-7752)

Read the control flow rather than trusting the tests alone, as asked. The out-of-range-int ProtocolError (line 7731) sits in its own if statement, sequentially before and outside the try (lines 7745-7751) — not nested inside it. Raising unwinds the stack immediately, so if that if fires, the try below is never entered; structurally, the except ValueError cannot see that raise. The try body itself contains only the stdlib call ipaddress.IPv6Address(identifier) — no in-library raise lives inside it to be swallowed either.

I also independently probed ipaddress.IPv6Address() directly against every exotic type I threw at it (None, float, list, dict, set, frozenset, a generator, a __len__-only object, an encode-only object, memoryview, bytearray, wrong-length bytes) — every single one raises ipaddress.AddressValueError (confirmed isinstance(e, ValueError) == True in each case), never a bare TypeError. So the except ValueError is complete against everything reachable, not just the cases the new test happens to try.

Bonus: I checked whether the existing (unmodified, #468-era) test would even catch the specific regression this attack worries about — merging the out-of-range check into the same try. It would: test_mh_mn_id_option_converts_int_identifier_per_subtype asserts self.assertIn('below 2**128', str(ctx.exception)) for that case, and a merged-try version would produce the generic "must be an ipaddress.IPv6Address..." message instead, failing that assertion even though the exception class would still be ProtocolError.

Attack 2 — completeness of the type gate

Ran an independent 8-subtype × 12-exotic-type sweep (96 combos: None, float, list, dict, set, frozenset, generator, HasLen(), HasEncode(), bool True/False, memoryview) through the public maker directly (not the PR's own test). Zero non-BaseError leaks. The only "NO RAISE" results were bool for the six octet subtypes and IPv6_Address (14 of 96) — and that's bool-is-int in Python (isinstance(True, int) == True), routed through the pre-existing, settled #468 int-conversion path before this PR's new gates are ever reached, not a gap this PR introduced. NAI correctly still rejects bool (int is rejected there too, per #468). Left a non-blocking inline comment noting it.

Also checked bytes/str subclasses and out-of-range/unrecognized subtype_val values (0, -1, 300, 999) against the same exotic types: all correctly rejected via ProtocolError, no leaks anywhere.

Attack 3 — the two judgement calls

Verified both claimed measurements independently, directly against struct.pack: struct.pack('4s', bytearray(b'ABCD')) → b'ABCD' (round-trips); struct.pack('4s', memoryview(b'ABCD')) → struct.error: argument for 's' must be a bytes object. Confirmed exactly as claimed.

I'd also point out the same principle is applied consistently to IPv6_Address, where bytearray is rejected even though it's accepted for the six octet subtypes — this isn't an inconsistency, because CPython's ipaddress.IPv6Address.__init__ only takes the packed-bytes fast path for an exact bytes instance (isinstance(address, bytes)); a bytearray/memoryview of the correct 16-byte length falls through to the str(address) fallback and fails with AddressValueError regardless (confirmed directly). So "does it survive the actual conversion call this branch uses" is applied uniformly across both branches, not just asserted for one. I agree accepting bytearray but not memoryview is a defensible, measured line rather than an arbitrary one.

Attack 4 — would the new test catch a re-break

test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype gives every one of the 8 subtypes its own dedicated wrong-type loop (NAI: 1, six octets: 1 shared loop, IPv6_Address: 1) — no subtype is left out of a loop, which is precisely the shape of #468's own miss (excluding IPv6_Address from the negative-int sweep). Assertions pin isinstance(ctx.exception, ProtocolError) (the concrete class, not just BaseError) plus a message substring naming the subtype and the accepted type — a real pin, if a slightly looser one than the pre-existing test's full-string assertIn('below 2**128', ...). It also separately pins the bytearray-accepted judgement call (schema.identifier == bytearray(...), schema.pack()[3:] == ...) and the correct-type/empty-value boundary. I ran this test standalone against the revert and confirmed it fails hard (64 subtests) without the fix.

Attack 5 — docstring/comment accuracy

Enumerated all 6 distinct ProtocolError-raising conditions in the code (negative int; int≥2**128 for IPv6_Address; int for NAI; type ipaddress.IPv6Address itself rejects; non-str for NAI; non-bytes/bytearray for the six) against the Raises: clause — all 6 are represented, unlike #468's own prior 2-of-3 omission. One precision nit, left inline: "anything but bytes/bytearray for the other six" reads as if int also raises there, but it doesn't (int is accepted/converted, intercepted earlier). Non-blocking — the Args: section already disambiguates it, but the Raises: bullet standing alone overstates it, which is the same category of thing #468 had to fix once.

Verdict

No blocking defects found. CI is fully green, the diff is scoped to exactly the two files it should touch, every quantitative claim in the PR description reproduced exactly under independent measurement, the exception-swallowing control flow is safe by construction (not just by test), an independent 96-combo exotic-type sweep found no leaks beyond a pre-existing/settled bool-is-int corner, and the two measured judgement calls (bytearray in, memoryview out) hold up and are applied consistently across both branches. Two non-blocking nits posted inline (a docstring precision issue and the bool/int observation) — neither affects correctness.

GOOD TO MERGE at 37f026846

…e overstating (#469) Both at the owner's request on the PR, on the two nits its review had raised as non-blocking. The docstring's Raises: bullet said "anything but bytes/bytearray for the other six", which read in isolation implies an int raises there too. It does not -- #468 accepts and converts an int for those six subtypes, and the Args: section above already says so. The bullet now names int alongside and points at #468, so the two halves of the docstring agree. bool is an int subclass, so True/False fell through to #468's numeric path and silently produced a plausible-looking wire form: IPv6Address(1), that is ::1, for IPv6_Address, and a one-octet identifier for the six BytesField subtypes. The review flagged this as out of scope and asked for no action; the owner asked for it fixed. An MN-ID of True is a caller mistake in every case rather than a value anyone means. The guard sits BEFORE the subtype dispatch, not inside the int branch -- my first attempt put it after, where it could not catch the IPv6_Address case at all, which is the same placement error #468's negative-int guard had to be corrected for. Measured all 8 subtypes x {True, False}: 16 combinations, every one now ProtocolError; the message names int(flag) as the escape hatch, and int(True) still converts to b'\x01'. Real ints are untouched: IPv6_Address 0x1234 -> IPv6Address('::1234'), IMSI 0x1234 -> b'\x124'. Verified: tests/protocols/internet/test_mh_unit.py 39 passed, 424 subtests, 0 failed. Revert-proof -- disabling only the bool guard fails 16 subtests of the new case and nothing else; restoring passes.

The bool guard shipped without the docstring that describes it, and the Args: sentence was left saying an int is accepted for every subtype but NAI -- which is now false, since bool is an int subclass and True is rejected. Reported on review of 35834b6. Args: carries the carve-out and points at Raises: for the reason. Raises: leads with it and says why rejecting is right rather than only that it happens: True would otherwise be converted by two different paths, to ::1 for IPv6_Address and to a one-octet identifier for the other six, and int(...) is the way to ask for the numeric value. This is the same class of defect the previous commit fixed two lines below -- a Raises: clause not matching behaviour, as #468 had to correct once already -- so it is worth fixing rather than filing. Docstring only; no behaviour change. Verified: 16 of 16 subtype/bool combinations still raise ProtocolError, IMSI 0x1234 still packs to b'\x124', tests/protocols/internet/test_mh_unit.py 39 passed / 424 subtests, and mypy --config-file mypy.ini reports the same two pre-existing errors on this file as before the change.

…tion (#500) Closes #491, the unfixed IPv4 sibling of #469/#481. - `_IPAddressField.pre_process`, `IPv4InterfaceField.pre_process`, and `IPv6InterfaceField.pre_process` (pcapkit/corekit/fields/ipaddress.py) now reject a `bool` value before dispatching into `ipaddress.ip_address`/ `ip_interface`, via a new shared `_reject_bool` helper. `bool` is an `int` subclass, and `ipaddress.ip_address()` treats any `int` below `2**32` as IPv4 -- so `True`/`False` used to be silently packed as `0.0.0.1`/ `0.0.0.0` on an IPv4-typed field, with no exception and no warning. On an IPv6-typed field the same conversion happened to raise instead (version mismatch), which is the asymmetry that let this slip past #481's guard for MH's `_make_opt_mn_id` -- that PR fixed the mechanism at one call site, not at its shared root. - `SecurityAssociation.__init__` (pcapkit/protocols/internet/esp.py) calls `ipaddress.ip_address()` directly on `destination`, bypassing the Field abstraction entirely; it gets the same guard, raising `ProtocolError`. - Both guards point the caller at `int(...)`, matching #481's message shape. tests/corekit/test_fields_ipaddress.py sweeps `{True, False}` across all four field classes (IPv4/IPv6 address and interface) and reproduces the original corruption through the public `IPv4.make(src=True, dst=False)` API. tests/protocols/internet/test_esp_unit.py adds the equivalent case for `SecurityAssociation`. Build: tests/corekit/test_fields_ipaddress.py -> 14 passed, 25 subtests. tests/protocols/internet/test_esp_unit.py -> 30 passed, 38 subtests. Full tests/corekit + tests/protocols/internet -> 254 passed, 662 subtests, 0 failed.

- `TSOption.post_process` converted `ts_data` entries to addresses with a bare `ipaddress.ip_address`, which takes a `bool` as the `int` it subclasses, so `ts_data=[True, 5]` packed and reported `IPv4Address('0.0.0.1')` with no exception. It runs on the packing path too, and `IPv4.make` accepts a caller-built option schema, so this was reachable from the public API. All three conversions now go through `parse_ip_address`, the fifth site of the defect #481, #500, #539 and #540 fixed before it. - `quick_start_data_selector` sized the nested Quick-Start suboption with a hardcoded `SchemaField(length=5)` -- the width of a Request's `ttl` and `nonce` alone. A well-formed 8-octet option decoded its nonce as 55 rather than 933982136 and left three octets to be read as a fabricated option, so the datagram failed with `ProtocolError`. The length now comes from `quick_start_option_length`, computed from the resolved suboption, and `QuickStartReportOption` gains the RFC 4782 section 3.1 `Not Used` octet it was missing, which had made it seven octets wide against the `length=8` both `_make_opt_qs` and `_read_opt_qs` use. - `_make_opt_ts` passed `data=` where the schema field is `ts_data`, so every timestamp was dropped with an `UnknownFieldWarning` and the Timestamp option was unbuildable through `make`. The `TYPE_CHECKING` `__init__` stub that advertised `data` is corrected too. Three new tests, each shown to fail without its fix; `ipv4-option/TS` deleted from `EXPECTED_FAILURES` now that it round-trips. Full unit tier green, 1107 passed with 2666 subtests; both changed modules at 100% statement and branch coverage.

JarryShaw commented Sep 18, 2026

View reviewed changes

Comment thread pcapkit/protocols/internet/mh.py Outdated

JarryShaw commented Sep 18, 2026

View reviewed changes

Comment thread pcapkit/protocols/internet/mh.py

JarryShaw added 2 commits September 18, 2026 14:58

Merge branch 'main' into fix-469-mn-id-wrong-type-identifier

a0a511a

JarryShaw commented Sep 18, 2026

View reviewed changes

Comment thread pcapkit/protocols/internet/mh.py

JarryShaw merged commit cf3b959 into main Sep 18, 2026
23 checks passed

JarryShaw deleted the fix-469-mn-id-wrong-type-identifier branch September 18, 2026 20:22

JarryShaw mentioned this pull request Sep 21, 2026

fix(link,internet): reject a bool at the four remaining maker call sites (#540) #568

Merged

7 tasks done

JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Sep 22, 2026

JarryShaw added this to the 1.5 milestone Oct 6, 2026

JarryShaw added this to PyPCAPKit Oct 6, 2026

Navigation Menu

Uh oh!

mh: reject a wrong-type MN-ID identifier in-library, not as a stdlib leak - #481

mh: reject a wrong-type MN-ID identifier in-library, not as a stdlib leak#481
JarryShaw merged 4 commits into
mainfrom
fix-469-mn-id-wrong-type-identifier

JarryShaw commented Sep 18, 2026

Uh oh!

Uh oh!

Uh oh!

JarryShaw commented Sep 18, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

JarryShaw commented Sep 18, 2026

Summary

What changed

Measurements

Judgement call: bytearray, memoryview, and ASCII str

Out of scope

Uh oh!

Uh oh!

Uh oh!

JarryShaw commented Sep 18, 2026

Scope of the diff

CI

Independent verification of the PR's own measurements

Attack 1 — the except ValueError ordering (mh.py:7717-7752)

Attack 2 — completeness of the type gate

Attack 3 — the two judgement calls

Attack 4 — would the new test catch a re-break

Attack 5 — docstring/comment accuracy

Verdict

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Judgement call: `bytearray`, `memoryview`, and ASCII `str`

Attack 1 — the `except ValueError` ordering (mh.py:7717-7752)