From d429404145e83231ba7fe18b68bfd3e4c58dcc95 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 17:44:55 -0400 Subject: [PATCH] fix(hip,ipv6-route): stop dropping the ENCRYPTED IV and RPL's pack-time cast (#556) Two small defects flagged in review this release and never filed: - HIP._make_param_encrypted passed cipher= to Schema_EncryptedParameter, a keyword the schema does not declare. __cipher__ is a pre_unpack packet-context key, not a field either spelling reaches -- both draw UnknownFieldWarning and are dropped -- so every AES-cipher ENCRYPTED parameter built through make packed without its IV. Fix: set the resolved cipher as a plain attribute after construction; pack()'s packet.update(self.__dict__) carries it into pre_unpack, which now trusts it ahead of its own HIP_CIPHER sibling lookup (a standalone make call has no options list for that lookup to search anyway). - RPL.post_process cast self.addresses to bytes unconditionally, but it runs on every Schema.pack, not only after a parse, and a schema built via make (IPv6_Route._make_data_type_rpl) still holds addresses as list[bytes]. Slicing and re-joining that list as the concatenated buffer a parse produces raised at pack time. Fix: skip the SRH decompression when addresses is not bytes -- nothing reads .ip on the make path. Fixing the second defect exposes two pre-existing ones immediately behind it, neither fixed here: IPv6_Route._read_data_type_rpl's own `header.length % 16 != 0` guard (ipv6_route.py:612) treats Hdr Ext Len as octets rather than 8-octet units, the same confusion #487 fixed for Source Route and Type 2; and the header it is judging is not well-formed either way, since RPL's fixed area (cmpr_i + cmpr_e + pad) packs to 5 octets against the 4 RFC 6554 specifies, filed as #564. RPL addresses are also variable-length under cmpr_i/cmpr_e, so no fixed bound is obviously right even once both are fixed, and nothing has been checked against a real RPL capture. The RPL_Source_Route_Header entry in tests/protocols/test_option_roundtrip_unit.py's EXPECTED_FAILURES is re-pointed at these, not deleted, since the case still does not construct -- just for different, now-documented reasons. New regression tests for both original defects, each shown to fail without its fix. Build and full test suite pass. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 9 ++++ pcapkit/protocols/internet/hip.py | 17 +++++- pcapkit/protocols/internet/ipv6_route.py | 12 ++--- pcapkit/protocols/schema/internet/hip.py | 24 ++++++++- .../protocols/schema/internet/ipv6_route.py | 19 ++++++- tests/protocols/internet/test_hip_unit.py | 53 +++++++++++++++++++ .../internet/test_ipv6_extension_unit.py | 42 +++++++++++++++ tests/protocols/test_option_roundtrip_unit.py | 31 ++++++++--- 9 files changed, 190 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc99bac9..b6f7e877a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- constant lookups that rejected a value the registry defines. `RouterAlert(0)` is the only value [RFC 2113](https://datatracker.ietf.org/doc/html/rfc2113) defines and the one IGMP, RSVP and MLD actually send, and it was discarded because the vendor crawler skipped a header row IANA's CSV does not have; IPX `Socket(0)` is that protocol's own default, so `bytes(IPX(...))` crashed on its own defaults; and two FTP `_missing_` overrides were plain methods rather than classmethods, so every unregistered value raised `TypeError` instead of extending the enumeration (#492, #503). - **Fixed** -- `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did. - **Fixed** -- 45 places where a documentation page contradicted the code (#413), ambiguous cross-references and five autodoc signature failures (#416), and `Extractor`'s documented exception plus 40 phantom or stale `Args:` labels (#501). +- **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 4e230831a..624741d9a 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -282,6 +282,15 @@ pull requests between #326 and #509. (#413), ambiguous cross-references and five autodoc signature failures (#416), and ``Extractor``'s documented exception plus 40 phantom or stale ``Args:`` labels (#501). +* **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during + this release and never filed until now: HIP's ``_make_param_encrypted`` passed + ``cipher=`` to a schema with no such field, so the value was silently dropped + and an AES-cipher ``ENCRYPTED`` parameter built through ``make`` packed + without its IV; and IPv6-Route's ``RPL.post_process``, which runs on every + ``Schema.pack`` and not only after a parse, assumed ``self.addresses`` was + still the concatenated ``bytes`` a parse leaves it as, and raised slicing the + ``list[bytes]`` a ``make``-built multi-address header actually holds there + (#556). Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/pcapkit/protocols/internet/hip.py b/pcapkit/protocols/internet/hip.py index 0ba95de47..c6615a236 100644 --- a/pcapkit/protocols/internet/hip.py +++ b/pcapkit/protocols/internet/hip.py @@ -3527,13 +3527,26 @@ def _make_param_encrypted(self, code: 'Enum_Parameter', param: 'Optional[Data_En if len(iv) != 16: raise ProtocolError(f'HIPv{version}: [ParamNo {code}] IV length must be 16 bytes for AES cipher') - return Schema_EncryptedParameter( + schema = Schema_EncryptedParameter( type=code, len=4 + len(iv or b'') + len(data), - cipher=cipher_id, iv=iv, data=data, ) + # NOTE: ``cipher`` is not a schema field -- ``ENCRYPTED``'s own wire + # format carries no cipher ID of its own, only the preceding + # ``HIP_CIPHER`` parameter does -- so passing it as a constructor + # keyword (as this used to) drew an ``UnknownFieldWarning`` and was + # dropped, leaving ``pre_unpack`` to fall back to its own sibling + # lookup, which a standalone ``make`` call gives no ``options`` to + # search and which then always treated the parameter as cipher-less + # and silently packed the ``ENCRYPTED`` parameter without its IV. + # Setting the already-resolved ``cipher_id`` as a plain attribute + # instead reaches ``pack()``'s packet context via + # ``packet.update(self.__dict__)``, where ``pre_unpack`` now honours + # it ahead of that lookup. See #556. + schema.cipher = cipher_id + return schema def _make_param_host_id(self, code: 'Enum_Parameter', param: 'Optional[Data_HostIDParameter]' = None, *, # pylint: disable=unused-argument version: 'int', diff --git a/pcapkit/protocols/internet/ipv6_route.py b/pcapkit/protocols/internet/ipv6_route.py index 3f1a5ba0a..2af414416 100644 --- a/pcapkit/protocols/internet/ipv6_route.py +++ b/pcapkit/protocols/internet/ipv6_route.py @@ -603,12 +603,12 @@ def _read_data_type_rpl(self, schema: 'Schema_RPL', *, header: 'Schema_IPv6_Rout # count. It is left as-is here: RPL addresses are variable-length # (compressed by ``cmpr_i``/``cmpr_e``), so a fixed ``% 16`` bound is # not obviously the right invariant even under correct units, and - # this module's RPL construction already fails before ever reaching - # this method, from an unrelated defect (``RPL.post_process`` in - # pcapkit/protocols/schema/internet/ipv6_route.py assumes ``bytes`` - # on a path ``Schema.pack`` also runs, per #476/#480) -- so there is - # no working round trip here to validate a replacement against. - # Flagged for follow-up rather than guessed at. + # nothing here has been checked against a real RPL capture. Flagged + # for follow-up rather than guessed at. (The round trip through + # ``RPL.post_process`` this note used to say was broken -- it + # treated a ``make``-built ``list[bytes]`` as ``bytes`` and raised + # on pack -- was fixed by #556; that no longer blocks validating a + # replacement here, but the replacement itself is still unwritten.) if header.length % 16 != 0: raise ProtocolError(f'{self.alias}: [TypeNo {header.type}] invalid format') diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index 48050edd1..d6219c4d9 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -613,7 +613,22 @@ def pre_unpack(cls, packet: 'dict[str, Any]') -> 'None': Args: packet: packet data + Notes: + When ``packet`` already carries a resolved ``cipher`` -- as it + does when this schema was built via + :meth:`HIP._make_param_encrypted + `, which + sets it as a plain attribute so ``pack()``'s own + ``packet.update(self.__dict__)`` carries it in here -- that value + is trusted over the ``HIP_CIPHER`` sibling lookup below, which a + parameter packed on its own has no ``options`` list for. See + #556. + """ + if 'cipher' in packet: + packet['__cipher__'] = packet.pop('cipher') + return + if 'options' in packet: cipher_list = cast('list[Data_HIPCipherParameter]', packet['options'].getlist(Enum_Parameter.HIP_CIPHER)) @@ -661,10 +676,15 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': return self if TYPE_CHECKING: - #: Cipher ID. + #: Cipher ID. Not a schema field -- set as a plain attribute, either + #: by :meth:`post_process` after unpacking, or by + #: :meth:`HIP._make_param_encrypted + #: ` before + #: packing -- so it is documented here rather than accepted by + #: ``__init__``. See #556. cipher: 'Enum_Cipher' - def __init__(self, type: 'Enum_Parameter', len: 'int', cipher: 'Enum_Cipher', + def __init__(self, type: 'Enum_Parameter', len: 'int', iv: 'Optional[bytes]', data: 'bytes') -> 'None': ... diff --git a/pcapkit/protocols/schema/internet/ipv6_route.py b/pcapkit/protocols/schema/internet/ipv6_route.py index d12a73e0c..0ef07b776 100644 --- a/pcapkit/protocols/schema/internet/ipv6_route.py +++ b/pcapkit/protocols/schema/internet/ipv6_route.py @@ -205,7 +205,24 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': Revised schema. """ - buffer = cast('bytes', self.addresses) + buffer = self.addresses + if not isinstance(buffer, bytes): + # NOTE: ``self.addresses`` is still a ``list[bytes]`` -- one + # already-compressed address per item -- when this schema was + # built via ``make`` (see + # :meth:`~pcapkit.protocols.internet.ipv6_route.IPv6_Route._make_data_type_rpl`) + # rather than parsed off the wire. There is nothing to decode in + # that case: the caller supplied each address already + # compressed, and :meth:`Schema.pack + # `'s own + # :class:`~pcapkit.corekit.fields.collections.ListField` handling + # packs that list directly. The SRH prefix-decompression below + # only makes sense against the raw octets a real parse hands + # here -- treating the list as ``bytes`` (as a bare ``cast`` + # used to, without a runtime check) raised trying to slice and + # re-join it. See #556. + return self + dst_val = cast('Optional[IPv6Address]', packet.get('dst')) dst = dst_val.packed if dst_val is not None else None diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index a0f9610bf..a6840f839 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -1293,6 +1293,59 @@ def test_hip_parameter_constructors_cover_keyword_paths(self) -> None: hmac=b'relh', ).hmac, b'relh') + def test_hip_make_param_encrypted_preserves_iv_on_pack(self) -> None: + # #556: ``_make_param_encrypted`` used to pass ``cipher=`` to + # ``Schema_EncryptedParameter``, a keyword the schema does not + # accept (the packing-time cipher lookup keys off ``__cipher__``, + # a packet-context key -- not a constructible field), so the value + # was dropped with an ``UnknownFieldWarning`` and the schema's own + # ``pre_unpack`` fallback then always treated the parameter as + # cipher-less, silently packing an AES-cipher ``ENCRYPTED`` + # parameter without its IV. Building one through ``make`` and + # packing it must retain the IV. + import warnings + + from pcapkit.const.hip.cipher import Cipher + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + proto = object.__new__(HIP) + iv = b'\x11' * 16 + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + schema = proto._make_param_encrypted( + Parameter.ENCRYPTED, + version=2, + cipher=Cipher.AES_128_CBC, + iv=iv, + data=b'DATA', + ) + packed = bytes(schema) + # constructing and packing must not have drawn an + # ``UnknownFieldWarning`` for an unrecognised ``cipher`` keyword, nor + # the ``pre_unpack`` fallback's "missing HIP_CIPHER parameter" one. + self.assertEqual(caught, []) + + self.assertIn(iv, packed) + self.assertEqual(schema.cipher, Cipher.AES_128_CBC) + + # a cipher that needs no IV (e.g. ``NULL_ENCRYPT``) still packs + # cleanly, with no IV octets and no "missing HIP_CIPHER" warning + # (the resolved cipher is known directly; there is nothing to + # infer from a sibling parameter this schema was never given). + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + schema_null = proto._make_param_encrypted( + Parameter.ENCRYPTED, + version=2, + cipher=Cipher.NULL_ENCRYPT, + data=b'DATA', + ) + packed_null = bytes(schema_null) + self.assertEqual(caught, []) + self.assertEqual(packed_null, b'\x02\x81\x00\x08\x00\x00\x00\x00DATA\x00\x00\x00\x00') + def test_hip_parameter_constructors_cover_data_model_and_default_paths(self) -> None: from pcapkit.const.hip.certificate import Certificate from pcapkit.const.hip.cipher import Cipher diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 71699e8c6..02c693a1b 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -1453,6 +1453,48 @@ def test_ipv6_route_schema_selector_and_rpl_post_process_branches(self) -> None: with_dst.post_process({'dst': ip_address('2001:db8::ffff')}) self.assertEqual([str(item) for item in with_dst.ip], ['2001:db8::1', '2001:db8::2']) + def test_ipv6_route_rpl_packs_a_multi_address_list(self) -> None: + """Regression test for GH-556. + + ``RPL.post_process`` runs on every ``Schema.pack``, not only after a + real parse, and a schema built through ``make`` (see + ``IPv6_Route._make_data_type_rpl``) still holds ``self.addresses`` + as the ``list[bytes]`` the caller passed in -- one already- + compressed address per item -- rather than the concatenated + ``bytes`` a parse produces. ``post_process`` used to assume the + latter unconditionally, so packing sliced and re-joined the *list* + as though it were that concatenated buffer and raised. Merely + constructing the schema does not exercise this: the defect is only + reachable through an actual pack. + """ + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + from pcapkit.protocols.schema.internet import ipv6_route as route_schema + + first = ip_address('2001:db8::1') + second = ip_address('2001:db8::2') + + # Directly at the schema level: ``addresses`` is a ``list[bytes]``, + # exactly as ``_make_data_type_rpl`` hands it to the constructor. + rpl_schema = route_schema.RPL( + cmpr_i=0, cmpr_e=0, pad={'pad_len': 0}, + addresses=[first.packed, second.packed], + ) + packed = bytes(rpl_schema) + self.assertIn(first.packed, packed) + self.assertIn(second.packed, packed) + + # And through the public ``make`` entry point, which is what + # actually builds a multi-address RPL routing header end to end. + proto = object.__new__(IPv6_Route) + header = proto.make( + type=Routing.RPL_Source_Route_Header, + data={'ip': [first, second]}, + ) + header_packed = bytes(header) + self.assertIn(first.packed, header_packed) + self.assertIn(second.packed, header_packed) + def _assert_padding_options_parse_from_the_wire(self, protocol_cls: type) -> None: """A ``Pad1`` option must consume exactly one octet, wherever it sits. diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 723788eea..2f33c0700 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -322,14 +322,31 @@ class Gap(NamedTuple): # offset. Both cases round-trip now; entries deleted rather than left # behind, per the note at the top of this table. - # RPL fails earlier still: ``post_process`` assumes ``addresses`` is bytes, - # which is true after unpacking and false while packing, where it is still - # the list the constructor was handed. Unrelated to #487 (see #476/#480); - # still open. + # RPL used to fail in ``post_process``, which assumed ``addresses`` was + # bytes -- true after unpacking, false while packing, where it is still the + # list the constructor was handed. That was fixed by #556, and fixing it + # exposed the defect immediately behind it: the reader's own length guard. + # ``header.length`` is ``Hdr Ext Len``, in 8-octet units rather than octets, + # so ``% 16`` cannot be the right invariant -- the same unit confusion #487 + # fixed for Source Route and Type 2. Behind *that* is a third defect (#564): + # the fixed area -- ``cmpr_i`` + ``cmpr_e`` + ``pad`` -- packs to 5 octets, + # one wider than the 4 RFC 6554 specifies and this method's own docstring + # diagram draws, so the header the guard is judging is not well-formed + # either way (measured: it constructs to 41 octets against the 48 its own + # ``Hdr Ext Len`` of 5 declares). None of the three is fixed here: RPL + # addresses are also variable-length under ``cmpr_i``/``cmpr_e``, so no + # fixed bound is obviously correct even once the units and the field + # widths are both right, and nothing has been checked against a real RPL + # capture. Unrelated to #487 (see #476/#480); still open. 'ipv6-route-type/RPL_Source_Route_Header': Gap( - 'CONSTRUCT', 'does not appear to be an IPv4 or IPv6 address', - 'pcapkit/protocols/schema/internet/ipv6_route.py:156 -- post_process ' - 'assumes bytes; it runs on the pack path too, from schema.py:647'), + 'CONSTRUCT', 'IPv6-Route: [TypeNo 3] invalid format', + 'pcapkit/protocols/internet/ipv6_route.py:612 -- the guard rejects ' + 'the header, and the header is not well-formed to begin with: the ' + '5-octet cmpr_i/cmpr_e/pad fixed area is one octet wider than the 4 ' + 'RFC 6554 specifies (echoed in the docstring above), so Hdr Ext Len ' + 'is computed from a mis-sized data area (#564); % 16 additionally ' + 'treats Hdr Ext Len as octets rather than 8-octet units, the same ' + 'confusion #487 fixed for Source Route and Type 2'), # -- Mobility Header ------------------------------------------------------ #