From 37f0268468894b7d760403a143f4a47af375b361 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 13:49:24 -0400 Subject: [PATCH 1/3] mh: reject a wrong-type MN-ID identifier in-library, not as a stdlib 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 e80c42217, 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. --- pcapkit/protocols/internet/mh.py | 72 ++++++++++++++- tests/protocols/internet/test_mh_unit.py | 112 +++++++++++++++++++++++ 2 files changed, 181 insertions(+), 3 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 49fb6b1b7f..1665fcf46b 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7664,10 +7664,15 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption Raises: ProtocolError: If ``identifier`` is a negative :obj:`int` (no subtype has a wire form for one), an :obj:`int` of any value - with the ``NAI`` subtype, or an :obj:`int` of ``2**128`` or + with the ``NAI`` subtype, an :obj:`int` of ``2**128`` or above with the ``IPv6_Address`` subtype (whose wire form is a fixed 16 octets, unlike the other subtypes, which have no - ceiling and simply pack into more). + ceiling and simply pack into more), or ``identifier`` is of a + type its subtype's field cannot hold at all: anything but + :obj:`str` for ``NAI``, anything but :obj:`bytes`/ + :obj:`bytearray` for the other six, or anything + :class:`ipaddress.IPv6Address` itself does not accept for + ``IPv6_Address`` (c.f. #469). """ if option is not None: @@ -7727,7 +7732,23 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption f'{self.alias}: [OptNo {type}] MN-ID subtype IPv6_Address ' f'identifier must be an int below 2**128, not {identifier!r}') if not isinstance(identifier, ipaddress.IPv6Address): - identifier = ipaddress.IPv6Address(identifier) + # NOTE: catches ipaddress.AddressValueError -- itself a bare + # ValueError -- for every identifier ipaddress.IPv6Address + # cannot turn into an address: bytes of the wrong length, a + # str that is not an IPv6 literal, or a type it does not + # accept at all (float, None, list, dict, bytearray, + # memoryview -- ipaddress.IPv6Address only ever dispatches on + # bytes, int or str). The ``try`` wraps only this call, not + # the whole branch, so it cannot swallow the ProtocolError + # raised above for an out-of-range int, which is also a + # ValueError subclass (c.f. #467, #468, #469). + try: + identifier = ipaddress.IPv6Address(identifier) + except ValueError as error: + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID subtype IPv6_Address ' + f'identifier must be an ipaddress.IPv6Address, an int, or ' + f'bytes/str it accepts, not {identifier!r}') from error id_len = 16 elif isinstance(identifier, int): if subtype_val == Enum_MNIDSubtype.NAI: @@ -7763,7 +7784,52 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # encoder would produce. id_len = max(1, math.ceil(identifier.bit_length() / 8)) identifier = identifier.to_bytes(id_len, 'big') + elif subtype_val == Enum_MNIDSubtype.NAI: + # NOTE: NAI's field is a StringField (c.f. mn_id_selector), which + # calls ``identifier.encode(...)`` to pack -- so anything but a + # genuine str leaks a bare stdlib exception: AttributeError for + # bytes/list/dict (no ``.encode``), TypeError for float/None/an + # ipaddress.IPv6Address (no ``__len__`` either, so this branch's + # own ``len()`` call below would be the one to raise). Guarded + # here, before ``len()``, rather than relying on whichever of + # those two happens to fire first (c.f. #469). Deliberately not + # decoding a ``bytes`` identifier here: an NAI that happens to be + # ASCII-encodable is still the caller handing over the wrong + # representation, the same "accepts a value that means the wrong + # thing" #467 removed for int, just relocated to bytes. + if not isinstance(identifier, str): + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID subtype NAI ' + f'identifier must be str, not {identifier!r}') + id_len = len(identifier) else: + # NOTE: every other subtype's field is a + # BytesField(length=pkt['length'] - 1) (c.f. mn_id_selector), + # which hands ``identifier`` to ``struct.pack('Ns', ...)`` + # unconverted -- so anything but bytes leaks a bare stdlib + # exception: struct.error for str/list/dict (struct's ``s`` + # format demands a bytes object), TypeError for float/None/an + # ipaddress.IPv6Address (no ``__len__``, so this branch's own + # ``len()`` call below would raise instead). Guarded here, + # before ``len()``, for the same reason as the NAI branch above + # (c.f. #469). + # + # bytearray is accepted alongside bytes -- unlike every other + # wrong type here, it already round-trips correctly through + # ``struct.pack('Ns', ...)`` (measured), so rejecting it would + # be a gratuitous behaviour change to a type nothing here is + # actually broken for. memoryview looks equally bytes-like but + # does *not* survive struct's ``s`` format (measured: same + # ``struct.error`` as str/list/dict), so it is rejected with + # everything else rather than let through to leak anyway. + if not isinstance(identifier, (bytes, bytearray)): + try: + subtype_repr = repr(Enum_MNIDSubtype(subtype_val)) + except ValueError: + subtype_repr = repr(subtype_val) + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID subtype {subtype_repr} ' + f'identifier must be bytes, not {identifier!r}') id_len = len(identifier) return Schema_MNIDOption( diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 42ffe9bbe3..cc51e4e666 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2482,6 +2482,118 @@ def test_mh_mn_id_option_converts_int_identifier_per_subtype(self) -> None: Option.MN_ID_OPTION_TYPE, subtype=subtype, identifier=-5) self.assertIsInstance(ctx.exception, BaseError) + def test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype(self) -> None: + """Every MN-ID subtype rejects the *other* documented type in-library. + + ``_make_opt_mn_id`` annotates ``identifier`` as + ``bytes | str | IPv6Address | int``, and #467/#468 (see + ``test_mh_mn_id_option_converts_int_identifier_per_subtype`` above) + closed the ``int`` half of that union. But each subtype's field + (c.f. ``mn_id_selector``) accepts exactly *one* of ``bytes``/``str`` + -- a ``StringField`` for ``NAI``, a ``BytesField`` for the other six + -- so handing the wrong one, or a value of neither type, used to + leak a bare stdlib exception instead of an in-library one: + + * ``bytes``/list/dict for ``NAI`` -- ``AttributeError`` (``StringField`` + calls ``identifier.encode(...)`` to pack) + * ``str``/list/dict for the six octet subtypes -- ``struct.error`` + (struct's ``s`` format demands a bytes object) + * anything without ``__len__`` (``float``, ``None``, an + :class:`~ipaddress.IPv6Address`) for any of the seven -- + ``TypeError: object of type '...' has no len()`` + * anything :class:`ipaddress.IPv6Address` itself cannot parse, for + ``IPv6_Address`` -- ``ipaddress.AddressValueError``, itself a bare + ``ValueError`` + + Every one of those must become a ``ProtocolError`` naming the subtype + and the type it actually accepts. See #469. + """ + from ipaddress import IPv6Address + + from pcapkit.const.mh.mn_id_subtype import MNIDSubtype + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import BaseError, ProtocolError + + proto = object.__new__(MH) + + octet_subtypes = ('IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address', + 'GUTI', 'DUID') + + # values that are the *wrong* type for every subtype below -- none of + # bytes, str, an IPv6Address instance, a float, None, a list, or a + # dict is what any of these subtypes' fields can hold except the one + # matching type tested separately further down. + wrong_for_nai = (b'\x01\x02', [1, 2], {'a': 1}, IPv6Address('::1'), 1.5, None, b'') + wrong_for_octets = ('user@realm', [1, 2], {'a': 1}, IPv6Address('::1'), 1.5, None, '', memoryview(b'\x01\x02')) + wrong_for_ipv6 = (b'\x01\x02', 'user@realm', [1, 2], {'a': 1}, 1.5, None, b'', + bytearray(b'\x00' * 16), memoryview(b'\x00' * 16)) + + for value in wrong_for_nai: + with self.subTest(subtype='NAI', identifier=value): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier=value) + self.assertIsInstance(ctx.exception, ProtocolError) + self.assertIn('NAI', str(ctx.exception)) + self.assertIn('str', str(ctx.exception)) + + for subtype in octet_subtypes: + for value in wrong_for_octets: + with self.subTest(subtype=subtype, identifier=value): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=value) + self.assertIsInstance(ctx.exception, ProtocolError) + self.assertIn('bytes', str(ctx.exception)) + + for value in wrong_for_ipv6: + with self.subTest(subtype='IPv6_Address', identifier=value): + with self.assertRaises(BaseError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, + identifier=value) + self.assertIsInstance(ctx.exception, ProtocolError) + self.assertIn('IPv6_Address', str(ctx.exception)) + + # the matching type, including the empty-value boundary, still works + # for every subtype -- this fix must reject the wrong type, not + # tighten what already worked. + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier='') + self.assertEqual(schema.length, 1) + self.assertEqual(len(schema.pack()), schema.length + 2) + + for subtype in octet_subtypes: + with self.subTest(subtype=subtype, identifier=b''): + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=b'') + self.assertEqual(schema.length, 1) + self.assertEqual(len(schema.pack()), schema.length + 2) + + # ``bytearray`` is accepted alongside ``bytes`` for the six octet + # subtypes: unlike every value in ``wrong_for_octets`` above, it + # already round-trips correctly through + # ``struct.pack('Ns', ...)`` (measured separately), so rejecting + # it would tighten behaviour that was never broken. ``memoryview`` + # looks equally bytes-like but does *not* survive that same pack + # call, so it stays in ``wrong_for_octets`` above rather than + # being let through to leak a bare ``struct.error`` anyway. + with self.subTest(subtype=subtype, identifier='bytearray'): + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=getattr(MNIDSubtype, subtype), + identifier=bytearray(b'\x01\x02')) + self.assertEqual(schema.identifier, bytearray(b'\x01\x02')) + self.assertEqual(schema.pack()[3:], b'\x01\x02') + + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, + identifier=IPv6Address('2001:db8::1')) + self.assertEqual(schema.length, 17) + self.assertEqual(len(schema.pack()), schema.length + 2) + def test_mh_redirect_option_rejects_contradictory_flags(self) -> None: """:rfc:`6463#section-4.2` allows exactly one of the ``K`` and ``N`` flags. From 35834b69beefe9ab68bc4ec1fdab49bbc3baf86d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 15:03:35 -0400 Subject: [PATCH 2/3] protocols: reject a bool MN-ID identifier, and stop the Raises: clause 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. --- pcapkit/protocols/internet/mh.py | 18 ++++++++++++- tests/protocols/internet/test_mh_unit.py | 34 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 1665fcf46b..d167b63887 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7670,7 +7670,8 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption ceiling and simply pack into more), or ``identifier`` is of a type its subtype's field cannot hold at all: anything but :obj:`str` for ``NAI``, anything but :obj:`bytes`/ - :obj:`bytearray` for the other six, or anything + :obj:`bytearray`/:obj:`int` for the other six -- an :obj:`int` + is converted rather than rejected there, per #468 -- or anything :class:`ipaddress.IPv6Address` itself does not accept for ``IPv6_Address`` (c.f. #469). @@ -7707,6 +7708,21 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption f'{self.alias}: [OptNo {type}] MN-ID subtype {subtype_repr} ' f'identifier must be a non-negative int, not {identifier!r}') + if isinstance(identifier, bool): + # NOTE: checked before the subtype dispatch, because ``bool`` is an + # ``int`` subclass and so would otherwise be converted by *two* + # different paths below -- ``IPv6Address(1)``, that is ``::1``, for + # ``IPv6_Address``, and a one-octet identifier for the six + # ``BytesField`` subtypes. An MN-ID of ``True`` is a caller mistake + # in every case rather than a value anyone means, so it is refused + # before either path can give it a plausible-looking wire form. A + # caller who genuinely wants the integer should pass ``int(flag)`` + # (c.f. #469 review). + raise ProtocolError( + f'{self.alias}: [OptNo {type}] MN-ID identifier must not be a ' + f'bool, not {identifier!r} -- pass int({identifier!r}) if the ' + f'numeric value is what is wanted') + # NOTE: The wire format is chosen by ``subtype_val`` (c.f. ``mn_id_selector``), # not by the Python type of ``identifier``, so the width has to be taken from # the former. For the ``IPv6_Address`` subtype the schema always packs a fixed diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index cc51e4e666..462c4a0ef6 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2594,6 +2594,40 @@ def test_mh_mn_id_option_rejects_wrong_type_identifier_per_subtype(self) -> None self.assertEqual(schema.length, 17) self.assertEqual(len(schema.pack()), schema.length + 2) + def test_mh_mn_id_option_rejects_a_bool_identifier_for_every_subtype(self) -> None: + """A ``bool`` identifier is a caller mistake, not a one-octet integer. + + ``bool`` is an :class:`int` subclass, so before #469's review flagged it + ``True``/``False`` fell through to the #468 int-conversion 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. Refused for every subtype now, before the + subtype dispatch, so neither numeric path can reach it. A caller who + genuinely wants the integer passes ``int(flag)``, which the message says. + """ + from pcapkit.const.mh.mn_id_subtype import MNIDSubtype + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import ProtocolError + + proto = object.__new__(MH) + for subtype in ('NAI', 'IPv6_Address', 'IMSI', 'P_TMSI', + 'EUI_48_address', 'EUI_64_address', 'GUTI', 'DUID'): + for identifier in (True, False): + with self.subTest(subtype=subtype, identifier=identifier): + with self.assertRaises(ProtocolError) as ctx: + proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, + subtype=getattr(MNIDSubtype, subtype), + identifier=identifier) + self.assertIn('must not be a bool', str(ctx.exception)) + + # ``int(flag)`` is the documented escape hatch and still converts. + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IMSI, + identifier=int(True)) + self.assertEqual(schema.identifier, b'\x01') + def test_mh_redirect_option_rejects_contradictory_flags(self) -> None: """:rfc:`6463#section-4.2` allows exactly one of the ``K`` and ``N`` flags. From e41d3b7202621e44f487f6b8ba644207d8162930 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 15:41:52 -0400 Subject: [PATCH 3/3] docs: document the bool rejection in _make_opt_mn_id's own docstring 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 35834b69b. 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. --- pcapkit/protocols/internet/mh.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index d167b63887..7b6d588c88 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7646,7 +7646,9 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption subtype_namespace: MN-ID subtype namespace. subtype_reversed: MN-ID subtype reversed flag. identifier: Identifier. An :obj:`int` is accepted for every - subtype except ``NAI``. For ``IPv6_Address`` it is converted + subtype except ``NAI``, with the sole exception of a + :obj:`bool`, which is rejected for every subtype -- see + ``Raises`` below. For ``IPv6_Address`` it is converted and validated the same way as any other value :class:`ipaddress.IPv6Address` accepts. For the other six subtypes -- all numeric identifiers (an IMSI, a P-TMSI, an @@ -7662,8 +7664,14 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption Constructed option schema. Raises: - ProtocolError: If ``identifier`` is a negative :obj:`int` (no - subtype has a wire form for one), an :obj:`int` of any value + ProtocolError: If ``identifier`` is a :obj:`bool`, for any subtype + -- :obj:`bool` is an :obj:`int` subclass, so ``True`` would + otherwise be converted by two different paths below, to + ``::1`` for ``IPv6_Address`` and to a one-octet identifier for + the other six, neither of which a caller passing a flag can + plausibly have meant; pass ``int(...)`` to get the numeric + value (c.f. #469). If ``identifier`` is a negative :obj:`int` + (no subtype has a wire form for one), an :obj:`int` of any value with the ``NAI`` subtype, an :obj:`int` of ``2**128`` or above with the ``IPv6_Address`` subtype (whose wire form is a fixed 16 octets, unlike the other subtypes, which have no