From cce86ca4c6d02c408631095376e1d6913ea6bce9 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 00:04:46 -0400 Subject: [PATCH 1/3] protocols: floor four more HIP list-length callbacks at zero (#463) - NATTraversalModeParameter.modes, TransportFormatListParameter.formats, ESPTransformParameter.suites and HIPTransportModeParameter.mode each computed their ListField length as pkt['len'] - 2 with no lower bound; a peer declaring Length=0 drove that to -2, and ListField's own `while length > 0` loop silently returned an empty list instead of raising, so a malformed parameter parsed "successfully" with no exception at all. - Add a shared two_octet_prefix_list_len() helper, following #460's reg_info_list_len() precedent: same floor-at-zero-and-raise shape, raising FieldValueError. Named for the shared shape (a two-octet reserved/port field ahead of the list) rather than either field's meaning, since the expression is byte-identical across all four sites. - Document the new helper in hip.rst next to its siblings. - Add one regression test per site in test_hip_unit.py, each asserting the parse now raises instead of silently returning an empty list. - TransportFormatListParameter has no such two-octet field on the wire (RFC 7401's TRANSPORT_FORMAT_LIST has nothing between Length and the list), so its previously-benign default (empty-list, Length=0) case now fails at CONSTRUCT instead of RECONSTRUCT; updated its EXPECTED_FAILURES entry in test_option_roundtrip_unit.py to match, with the pre-existing wrong-offset defect noted as out of scope here. Build: brazil-build n/a (public repo); full suite at faf86d26b before this change is 4 failed/995 passed/17 skipped (1016 collected), and 0 failed/999 passed/17 skipped after, both with freshly regenerated fixtures matching the code under test. --- .../source/pcapkit/protocols/internet/hip.rst | 1 + pcapkit/protocols/schema/internet/hip.py | 35 ++++- tests/protocols/internet/test_hip_unit.py | 140 ++++++++++++++++++ tests/protocols/test_option_roundtrip_unit.py | 24 ++- 4 files changed, 194 insertions(+), 6 deletions(-) diff --git a/docs/source/pcapkit/protocols/internet/hip.rst b/docs/source/pcapkit/protocols/internet/hip.rst index b3bceb9f19..432bc66d21 100644 --- a/docs/source/pcapkit/protocols/internet/hip.rst +++ b/docs/source/pcapkit/protocols/internet/hip.rst @@ -426,6 +426,7 @@ Auxiliary Functions .. autofunction:: pcapkit.protocols.schema.internet.hip.host_id_hi_selector .. autofunction:: pcapkit.protocols.schema.internet.hip.registration_type_list_len .. autofunction:: pcapkit.protocols.schema.internet.hip.reg_info_list_len +.. autofunction:: pcapkit.protocols.schema.internet.hip.two_octet_prefix_list_len Data Models ----------- diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index 4bedd87001..7758d2ddd0 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -216,6 +216,33 @@ def reg_info_list_len(pkt: 'dict[str, Any]') -> 'int': return length +def two_octet_prefix_list_len(pkt: 'dict[str, Any]') -> 'int': + """Return list length for a parameter with a two-octet prefix. + + Used by the ``modes``, ``formats``, ``suites`` and ``mode`` fields of + :class:`NATTraversalModeParameter`, :class:`TransportFormatListParameter`, + :class:`ESPTransformParameter` and :class:`HIPTransportModeParameter` + respectively, each of which follows a two-octet ``reserved`` or ``port`` + field with a list of items sized by the remainder of the parameter. + + Args: + pkt: Parameter unpacked schema. + + Returns: + List length. + + Raises: + FieldValueError: If the parameter's ``Length`` on the wire is too + short to hold the two octets already read, which would otherwise + underflow the list length below zero. + + """ + length = pkt['len'] - 2 + if length < 0: + raise FieldValueError(f'HIP: invalid parameter length: {pkt["len"]}') + return length + + class Parameter(EnumSchema[Enum_Parameter]): """Base schema for HIP parameters.""" @@ -477,7 +504,7 @@ class NATTraversalModeParameter(Parameter, code=Enum_Parameter.NAT_TRAVERSAL_MOD reserved: 'bytes' = PaddingField(length=2) #: NAT traversal modes. modes: 'list[Enum_NATTraversal]' = ListField( - length=lambda pkt: pkt['len'] - 2, + length=two_octet_prefix_list_len, item_type=EnumField(length=1, namespace=Enum_NATTraversal), ) #: Padding. @@ -831,7 +858,7 @@ class TransportFormatListParameter(Parameter, code=Enum_Parameter.TRANSPORT_FORM #: Transport formats. formats: 'list[Enum_Parameter]' = ListField( - length=lambda pkt: pkt['len'] - 2, + length=two_octet_prefix_list_len, item_type=EnumField(length=1, namespace=Enum_Parameter), ) #: Padding. @@ -849,7 +876,7 @@ class ESPTransformParameter(Parameter, code=Enum_Parameter.ESP_TRANSFORM): reserved: 'bytes' = PaddingField(length=2) #: Suite IDs. suites: 'list[Enum_ESPTransformSuite]' = ListField( - length=lambda pkt: pkt['len'] - 2, + length=two_octet_prefix_list_len, item_type=EnumField(length=1, namespace=Enum_ESPTransformSuite), ) #: Padding. @@ -964,7 +991,7 @@ class HIPTransportModeParameter(Parameter, code=Enum_Parameter.HIP_TRANSPORT_MOD port: 'int' = UInt16Field() #: Mode IDs. mode: 'list[Enum_Transport]' = ListField( - length=lambda pkt: pkt['len'] - 2, + length=two_octet_prefix_list_len, item_type=EnumField(length=2, namespace=Enum_Transport), ) #: Padding. diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index 2ae772b3d4..ffafe80ef8 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -1840,6 +1840,146 @@ def test_hip_reg_info_parameter_rejects_underflowing_length(self) -> None: with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): HIP(raw, len(raw), extension=True) + def test_hip_nat_traversal_mode_parameter_rejects_underflowing_length(self) -> None: + """#463: a ``NAT_TRAVERSAL_MODE`` parameter's ``Length`` too small + for its own two-octet ``reserved`` field must raise, not silently + drop the NAT traversal mode list. + + ``modes`` sizes its list of NAT traversal mode entries as + ``Length - 2``, the ``- 2`` accounting for the ``reserved`` field + read unconditionally ahead of it. Nothing floored that at zero, so a + peer declaring ``Length = 0`` drove the list length to ``-2``. + Unlike a :class:`~pcapkit.corekit.fields.strings.BytesField`, + :class:`~pcapkit.corekit.fields.collections.ListField` never reaches + :func:`struct.calcsize` for a negative length -- its own ``while + length > 0`` loop just returns an empty list instead -- so this + parsed to an empty ``modes`` with no exception and no diagnostic, + rather than rejecting the malformed ``Length``. + + """ + from pcapkit.protocols.internet.hip import HIP + from pcapkit.utilities.exceptions import FieldValueError + + # next(1) len(1)=5 pkt(1) ver(1)=0x01 (the reserved bit that must be 1) + # checksum(2) control(2) shit(16) rhit(16) -- the fixed 40-octet header, + # declaring one 8-octet parameter to follow: (5 - 4) * 8 == 8. + fixed = bytes([0x3b, 0x05, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) + self.assertEqual(len(fixed), 40) + + # type(2)=608 (NAT_TRAVERSAL_MODE) len(2)=0, reserved(2), then 2 + # octets padding out the 8-octet parameter area the outer header + # declared. + param = (608).to_bytes(2, 'big') + (0).to_bytes(2, 'big') + bytes(2) + bytes(2) + raw = fixed + param + + with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): + HIP(raw, len(raw), extension=True) + + def test_hip_transport_format_list_parameter_rejects_underflowing_length(self) -> None: + """#463: a ``TRANSPORT_FORMAT_LIST`` parameter's ``Length`` too + small must raise, not silently drop the transport format list. + + ``formats`` sizes its list of transport format entries as + ``Length - 2`` even though this parameter carries no explicit + two-octet field ahead of the list on the wire -- the ``- 2`` is + shared with the other three sites via the same helper. Nothing + floored that at zero, so a peer declaring ``Length = 0`` drove the + list length to ``-2``. Unlike a + :class:`~pcapkit.corekit.fields.strings.BytesField`, + :class:`~pcapkit.corekit.fields.collections.ListField` never reaches + :func:`struct.calcsize` for a negative length -- its own ``while + length > 0`` loop just returns an empty list instead -- so this + parsed to an empty ``formats`` with no exception and no diagnostic, + rather than rejecting the malformed ``Length``. + + """ + from pcapkit.protocols.internet.hip import HIP + from pcapkit.utilities.exceptions import FieldValueError + + # next(1) len(1)=5 pkt(1) ver(1)=0x01 (the reserved bit that must be 1) + # checksum(2) control(2) shit(16) rhit(16) -- the fixed 40-octet header, + # declaring one 8-octet parameter to follow: (5 - 4) * 8 == 8. + fixed = bytes([0x3b, 0x05, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) + self.assertEqual(len(fixed), 40) + + # type(2)=2049 (TRANSPORT_FORMAT_LIST) len(2)=0, then 4 filler octets + # padding out the 8-octet parameter area the outer header declared; + # the list-length underflow raises before those filler octets would + # ever be read. + param = (2049).to_bytes(2, 'big') + (0).to_bytes(2, 'big') + bytes(4) + raw = fixed + param + + with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): + HIP(raw, len(raw), extension=True) + + def test_hip_esp_transform_parameter_rejects_underflowing_length(self) -> None: + """#463: an ``ESP_TRANSFORM`` parameter's ``Length`` too small for + its own two-octet ``reserved`` field must raise, not silently drop + the ESP transform suite list. + + ``suites`` sizes its list of ESP transform suite entries as + ``Length - 2``, the ``- 2`` accounting for the ``reserved`` field + read unconditionally ahead of it. Nothing floored that at zero, so a + peer declaring ``Length = 0`` drove the list length to ``-2``. + Unlike a :class:`~pcapkit.corekit.fields.strings.BytesField`, + :class:`~pcapkit.corekit.fields.collections.ListField` never reaches + :func:`struct.calcsize` for a negative length -- its own ``while + length > 0`` loop just returns an empty list instead -- so this + parsed to an empty ``suites`` with no exception and no diagnostic, + rather than rejecting the malformed ``Length``. + + """ + from pcapkit.protocols.internet.hip import HIP + from pcapkit.utilities.exceptions import FieldValueError + + # next(1) len(1)=5 pkt(1) ver(1)=0x01 (the reserved bit that must be 1) + # checksum(2) control(2) shit(16) rhit(16) -- the fixed 40-octet header, + # declaring one 8-octet parameter to follow: (5 - 4) * 8 == 8. + fixed = bytes([0x3b, 0x05, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) + self.assertEqual(len(fixed), 40) + + # type(2)=4095 (ESP_TRANSFORM) len(2)=0, reserved(2), then 2 octets + # padding out the 8-octet parameter area the outer header declared. + param = (4095).to_bytes(2, 'big') + (0).to_bytes(2, 'big') + bytes(2) + bytes(2) + raw = fixed + param + + with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): + HIP(raw, len(raw), extension=True) + + def test_hip_transport_mode_parameter_rejects_underflowing_length(self) -> None: + """#463: a ``HIP_TRANSPORT_MODE`` parameter's ``Length`` too small + for its own two-octet ``port`` field must raise, not silently drop + the transport mode list. + + ``mode`` sizes its list of transport mode entries as ``Length - 2``, + the ``- 2`` accounting for the ``port`` field read unconditionally + ahead of it. Nothing floored that at zero, so a peer declaring + ``Length = 0`` drove the list length to ``-2``. Unlike a + :class:`~pcapkit.corekit.fields.strings.BytesField`, + :class:`~pcapkit.corekit.fields.collections.ListField` never reaches + :func:`struct.calcsize` for a negative length -- its own ``while + length > 0`` loop just returns an empty list instead -- so this + parsed to an empty ``mode`` with no exception and no diagnostic, + rather than rejecting the malformed ``Length``. + + """ + from pcapkit.protocols.internet.hip import HIP + from pcapkit.utilities.exceptions import FieldValueError + + # next(1) len(1)=5 pkt(1) ver(1)=0x01 (the reserved bit that must be 1) + # checksum(2) control(2) shit(16) rhit(16) -- the fixed 40-octet header, + # declaring one 8-octet parameter to follow: (5 - 4) * 8 == 8. + fixed = bytes([0x3b, 0x05, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) + self.assertEqual(len(fixed), 40) + + # type(2)=7680 (HIP_TRANSPORT_MODE) len(2)=0, port(2), then 2 octets + # padding out the 8-octet parameter area the outer header declared. + param = (7680).to_bytes(2, 'big') + (0).to_bytes(2, 'big') + bytes(2) + bytes(2) + raw = fixed + param + + with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): + HIP(raw, len(raw), extension=True) + def test_hip_schema_selectors_and_encrypted_parameter_branches(self) -> None: from pcapkit.const.hip.cipher import Cipher from pcapkit.const.hip.hi_algorithm import HIAlgorithm diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 9eda19c983..aa0140c4f4 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -356,7 +356,7 @@ class Gap(NamedTuple): 'pcapkit/protocols/internet/hip.py:822 -- Parameter.registry[128] is ' 'UnassignedParameter, because R1CounterParameter declares code=129 only'), - # Sixteen parameters whose ``_read_param_*`` stores a list-valued field as a + # Fifteen parameters whose ``_read_param_*`` stores a list-valued field as a # tuple, which ``_make_param_*`` passes straight back to a ``ListField`` # that accepts only a list. This is the class of defect the reconstruct step # exists to find: each one constructs and parses perfectly. @@ -368,11 +368,31 @@ class Gap(NamedTuple): for name in ( 'ACK', 'DH_GROUP_LIST', 'HIP_CIPHER', 'NAT_TRAVERSAL_MODE', 'HIT_SUITE_LIST', 'REG_INFO', 'REG_REQUEST', 'REG_RESPONSE', - 'REG_FAILED', 'TRANSPORT_FORMAT_LIST', 'ESP_TRANSFORM', 'ACK_DATA', + 'REG_FAILED', 'ESP_TRANSFORM', 'ACK_DATA', 'ROUTE_DST', 'HIP_TRANSPORT_MODE', 'ROUTE_VIA', 'VIA_RVS', ) }, + # #463 gave four ``ListField`` length callbacks -- including this one's -- + # a shared floor-at-zero-and-raise guard (``two_octet_prefix_list_len``, + # pcapkit/protocols/schema/internet/hip.py:219), matching the other three + # sites' ``- 2`` accounting for a two-octet ``reserved``/``port`` field + # read ahead of the list. But ``TransportFormatListParameter`` has no such + # field -- RFC 7401's TRANSPORT_FORMAT_LIST is Type, Length, TF types, + # Padding, with nothing between Length and the list -- so its default + # (empty ``formats``) case packs with ``Length=0``, and the same + # byte-identical ``- 2`` that is correct for the other three now floors + # that construction to a raise instead of reaching the tuple/list + # mismatch this case used to hit further down the cycle. Pre-existing and + # out of #463's scope (a wrong offset, not a missing lower bound); tracked + # for a follow-up rather than fixed here, since the fix is directed to be + # byte-identical across all four sites. + 'hip-parameter/TRANSPORT_FORMAT_LIST': Gap( + 'CONSTRUCT', 'FieldValueError: HIP: invalid parameter length: 0', + 'pcapkit/protocols/schema/internet/hip.py:219 -- two_octet_prefix_list_len ' + 'assumes a 2-octet prefix that TransportFormatListParameter does not ' + 'have, so its default empty-list case (Length=0) now underflows'), + # ``_make_param_encrypted`` passes ``cipher=``, which is not a field of # ``EncryptedParameter`` -- so the cipher id is dropped with an # ``UnknownFieldWarning`` and never reaches the wire. The mismatch itself From e62a240956a93ba74c85e6e24736d98aefea1135 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 00:46:23 -0400 Subject: [PATCH 2/3] protocols: give TransportFormatListParameter its own length, no -2 (#463) - Correct a regression in the fix itself: TransportFormatListParameter has no two-octet reserved/port field ahead of its list -- RFC 7401 S5.2.11 defines Length as literally "2x number of TF types", nothing else between Length and the list -- unlike NATTraversalModeParameter, ESPTransformParameter and HIPTransportModeParameter, which genuinely do and keep two_octet_prefix_list_len unchanged. Reusing that helper here turned the parameter's legitimate empty-list encoding (Length=0, formats=[]) into a raise, converting a working main case into a failure. - Add transport_format_list_len(): sizes the list at Length exactly, with the same floor-and-raise discipline for a direct, bypassing construction call, even though real wire bytes (an unsigned len) can never underflow it. - The old -2 also silently under-read every non-empty list by two octets on parse; pinned with a new regression test, since it predates this PR and is not limited to the empty-list case. - EXPECTED_FAILURES['hip-parameter/TRANSPORT_FORMAT_LIST'] rejoins the sixteen-entry tuple/list RECONSTRUCT group now that CONSTRUCT/PARSE succeed again, confirmed by re-running the round-trip suite rather than assumed. - Measured separately, not fixed here: item_type=EnumField(length=1) sizes each TF type entry at one octet, but RFC 7401 S5.2.11's own diagram and "Length = 2x number of TF types" both say two, and the high-level maker already computes len=2*len(tf_type) assuming two-octet entries. Confirmed by construction crashing (struct.error) for any real Enum_Parameter value (all HIP parameter type numbers exceed 255) -- a real, separate defect, reported rather than folded in. Build: full suite regenerating fixtures fresh for pre- and post-correction trees (fixture generation itself depends on hip.py); both show 0 failed at the top level because EXPECTED_FAILURES was kept in lockstep with the regression -- the substance is in the new empty-list and full-length tests. --- .../source/pcapkit/protocols/internet/hip.rst | 1 + pcapkit/protocols/schema/internet/hip.py | 61 +++++++++- tests/protocols/internet/test_hip_unit.py | 113 ++++++++++++++---- tests/protocols/test_option_roundtrip_unit.py | 39 +++--- 4 files changed, 160 insertions(+), 54 deletions(-) diff --git a/docs/source/pcapkit/protocols/internet/hip.rst b/docs/source/pcapkit/protocols/internet/hip.rst index 432bc66d21..e1eddaf607 100644 --- a/docs/source/pcapkit/protocols/internet/hip.rst +++ b/docs/source/pcapkit/protocols/internet/hip.rst @@ -427,6 +427,7 @@ Auxiliary Functions .. autofunction:: pcapkit.protocols.schema.internet.hip.registration_type_list_len .. autofunction:: pcapkit.protocols.schema.internet.hip.reg_info_list_len .. autofunction:: pcapkit.protocols.schema.internet.hip.two_octet_prefix_list_len +.. autofunction:: pcapkit.protocols.schema.internet.hip.transport_format_list_len Data Models ----------- diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index 7758d2ddd0..5fd4194e60 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -219,11 +219,16 @@ def reg_info_list_len(pkt: 'dict[str, Any]') -> 'int': def two_octet_prefix_list_len(pkt: 'dict[str, Any]') -> 'int': """Return list length for a parameter with a two-octet prefix. - Used by the ``modes``, ``formats``, ``suites`` and ``mode`` fields of - :class:`NATTraversalModeParameter`, :class:`TransportFormatListParameter`, - :class:`ESPTransformParameter` and :class:`HIPTransportModeParameter` - respectively, each of which follows a two-octet ``reserved`` or ``port`` - field with a list of items sized by the remainder of the parameter. + Used by the ``modes``, ``suites`` and ``mode`` fields of + :class:`NATTraversalModeParameter`, :class:`ESPTransformParameter` and + :class:`HIPTransportModeParameter` respectively, each of which follows a + two-octet ``reserved`` or ``port`` field with a list of items sized by + the remainder of the parameter. + + :class:`TransportFormatListParameter` looked like a fourth call site -- + same ``pkt['len'] - 2`` expression -- but is not: see + :func:`transport_format_list_len` for why it has no such prefix to + subtract. Args: pkt: Parameter unpacked schema. @@ -243,6 +248,50 @@ def two_octet_prefix_list_len(pkt: 'dict[str, Any]') -> 'int': return length +def transport_format_list_len(pkt: 'dict[str, Any]') -> 'int': + """Return ``TRANSPORT_FORMAT_LIST`` transport format list length. + + Used by the ``formats`` field of :class:`TransportFormatListParameter`. + Unlike :func:`two_octet_prefix_list_len`'s three call sites, this + parameter has no ``reserved`` or ``port`` field between ``Length`` and + the list: :rfc:`7401` Section 5.2.11 defines ``Length`` as literally + "2x number of TF types" and places the list directly after it -- + + :: + + | Type | Length | + +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + | TF type #1 | TF type #2 / + + -- so the list's byte length *is* ``Length``, with nothing to subtract. + Subtracting 2 anyway (as a since-corrected revision of this module once + did, mirroring the three genuine two-octet-prefix sites) silently + dropped the trailing two octets of *every* non-empty list on parse, and + rejected the parameter's own legitimate empty-list encoding + (``Length = 0``, ``formats = []``) as malformed. + + Args: + pkt: Parameter unpacked schema. + + Returns: + Transport format list length. + + Raises: + FieldValueError: If the parameter's ``Length`` on the wire is + negative. This cannot happen from real wire bytes -- ``len`` is + an unsigned 16-bit field -- but a caller constructing the schema + directly, bypassing + :meth:`~pcapkit.protocols.internet.hip.HIP._make_param_transport_format_list`, + could still pass one; this keeps that path to the same + floor-and-raise discipline as :func:`two_octet_prefix_list_len`. + + """ + length = pkt['len'] + if length < 0: + raise FieldValueError(f'HIP: invalid parameter length: {pkt["len"]}') + return length + + class Parameter(EnumSchema[Enum_Parameter]): """Base schema for HIP parameters.""" @@ -858,7 +907,7 @@ class TransportFormatListParameter(Parameter, code=Enum_Parameter.TRANSPORT_FORM #: Transport formats. formats: 'list[Enum_Parameter]' = ListField( - length=two_octet_prefix_list_len, + length=transport_format_list_len, item_type=EnumField(length=1, namespace=Enum_Parameter), ) #: Padding. diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index ffafe80ef8..ad2623558c 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -1875,42 +1875,93 @@ def test_hip_nat_traversal_mode_parameter_rejects_underflowing_length(self) -> N with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): HIP(raw, len(raw), extension=True) - def test_hip_transport_format_list_parameter_rejects_underflowing_length(self) -> None: - """#463: a ``TRANSPORT_FORMAT_LIST`` parameter's ``Length`` too - small must raise, not silently drop the transport format list. - - ``formats`` sizes its list of transport format entries as - ``Length - 2`` even though this parameter carries no explicit - two-octet field ahead of the list on the wire -- the ``- 2`` is - shared with the other three sites via the same helper. Nothing - floored that at zero, so a peer declaring ``Length = 0`` drove the - list length to ``-2``. Unlike a - :class:`~pcapkit.corekit.fields.strings.BytesField`, - :class:`~pcapkit.corekit.fields.collections.ListField` never reaches - :func:`struct.calcsize` for a negative length -- its own ``while - length > 0`` loop just returns an empty list instead -- so this - parsed to an empty ``formats`` with no exception and no diagnostic, - rather than rejecting the malformed ``Length``. + def test_hip_transport_format_list_parameter_accepts_an_empty_list_at_length_zero(self) -> None: + """#463/#466: ``TRANSPORT_FORMAT_LIST`` has no two-octet prefix, so + ``Length = 0`` is a legitimate empty list, not a malformed one. + + A first pass at #463 applied the same ``pkt['len'] - 2`` guard used + by :class:`NATTraversalModeParameter`, :class:`ESPTransformParameter` + and :class:`HIPTransportModeParameter` to this parameter's ``formats`` + field too, on the assumption that the expression was byte-identical + across all four sites for the same reason. It is not: :rfc:`7401` + Section 5.2.11 defines ``Length`` as literally "2x number of TF + types", with nothing between ``Length`` and the list to account for. + So ``Length = 0`` with an empty ``formats`` list -- which packs + correctly on ``main`` as ``08010000`` -- was turned into a raise by + that first pass, a regression rather than a declined fix. This + parses it back to confirm the corrected :func:`~pcapkit.protocols. + schema.internet.hip.transport_format_list_len` accepts it again. + + Two copies, not one: a single parameter's own total is always + ``4 (mod 8)`` under this module's padding rule (see + ``examples/generators/options.py``'s ``HIP_COPIES``), so one 40-octet + fixed header plus two empty ``TRANSPORT_FORMAT_LIST`` parameters (4 + octets each) is what actually lands on an 8-octet boundary. """ + from pcapkit.const.hip.parameter import Parameter from pcapkit.protocols.internet.hip import HIP - from pcapkit.utilities.exceptions import FieldValueError # next(1) len(1)=5 pkt(1) ver(1)=0x01 (the reserved bit that must be 1) # checksum(2) control(2) shit(16) rhit(16) -- the fixed 40-octet header, - # declaring one 8-octet parameter to follow: (5 - 4) * 8 == 8. + # declaring one 8-octet parameter area to follow: (5 - 4) * 8 == 8. fixed = bytes([0x3b, 0x05, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) self.assertEqual(len(fixed), 40) - # type(2)=2049 (TRANSPORT_FORMAT_LIST) len(2)=0, then 4 filler octets - # padding out the 8-octet parameter area the outer header declared; - # the list-length underflow raises before those filler octets would - # ever be read. - param = (2049).to_bytes(2, 'big') + (0).to_bytes(2, 'big') + bytes(4) - raw = fixed + param + # Two copies of: type(2)=2049 (TRANSPORT_FORMAT_LIST) len(2)=0, no + # formats, no padding -- 4 octets each, 8 octets together. + empty = (2049).to_bytes(2, 'big') + (0).to_bytes(2, 'big') + self.assertEqual(len(empty), 4) + raw = fixed + empty * 2 + + proto = HIP(raw, len(raw), extension=True) + copies = proto.info.parameters.getlist(Parameter.TRANSPORT_FORMAT_LIST) + self.assertEqual(len(copies), 2) + self.assertEqual(copies[0].tf_type, ()) + self.assertEqual(copies[1].tf_type, ()) + + def test_hip_transport_format_list_parameter_parses_the_full_declared_length(self) -> None: + """#463/#466: the same ``- 2`` also under-read every *non-empty* + ``TRANSPORT_FORMAT_LIST``, silently dropping its last two octets. + + Pre-existing on ``main``, and not introduced by #466's guard: with + ``Length = 4`` and four wire octets of transport format entries, + ``pkt['len'] - 2 == 2`` sized the list at only two entries. + Confirmed directly against a ``main``-shaped schema object before + this fix: ``TransportFormatListParameter(type=..., len=4, + formats=[10, 20, 30, 40])`` packs to twelve octets and reads back as + ``formats == [Unassigned_10, Unassigned_20]`` -- the ``30`` and + ``40`` octets are simply gone, with no exception and no diagnostic, + because ``Length = 4`` never underflows and so never reaches + #460/#463's floor-and-raise guard at all. Removing the ``- 2`` + (:func:`~pcapkit.protocols.schema.internet.hip. + transport_format_list_len`) makes the list length equal to + ``Length`` exactly, so all four entries now survive the round trip. - with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): - HIP(raw, len(raw), extension=True) + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + + # next(1) len(1)=7 pkt(1) ver(1)=0x01 (the reserved bit that must be 1) + # checksum(2) control(2) shit(16) rhit(16) -- the fixed 40-octet header, + # declaring one 24-octet parameter area to follow: (7 - 4) * 8 == 24. + fixed = bytes([0x3b, 0x07, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) + self.assertEqual(len(fixed), 40) + + # Two copies of: type(2)=2049 len(2)=4, four one-octet format entries, + # four octets of padding (this module's padding rule pads the + # *contents* to eight, ignoring the four-octet type-and-length + # header) -- 12 octets each, 24 octets together. + one = (2049).to_bytes(2, 'big') + (4).to_bytes(2, 'big') + bytes([0x0a, 0x14, 0x1e, 0x28]) + bytes(4) + self.assertEqual(len(one), 12) + raw = fixed + one * 2 + + proto = HIP(raw, len(raw), extension=True) + copies = proto.info.parameters.getlist(Parameter.TRANSPORT_FORMAT_LIST) + self.assertEqual(len(copies), 2) + for copy in copies: + self.assertEqual(len(copy.tf_type), 4) + self.assertEqual([int(tf) for tf in copy.tf_type], [10, 20, 30, 40]) def test_hip_esp_transform_parameter_rejects_underflowing_length(self) -> None: """#463: an ``ESP_TRANSFORM`` parameter's ``Length`` too small for @@ -2018,6 +2069,16 @@ def test_hip_schema_selectors_and_encrypted_parameter_branches(self) -> None: with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): hip_schema.registration_type_list_len({'len': 0}) + # #463/#466: ``TRANSPORT_FORMAT_LIST`` has no prefix octet ahead of + # its list, so its length is ``Length`` exactly -- including zero. + self.assertEqual(hip_schema.transport_format_list_len({'len': 0}), 0) + self.assertEqual(hip_schema.transport_format_list_len({'len': 4}), 4) + # unreachable from real wire bytes (``len`` is unsigned on the wire), + # but a direct, bypassing construction call could still pass a + # negative ``len``; keep it to the same floor-and-raise discipline. + with self.assertRaisesRegex(FieldValueError, 'invalid parameter length'): + hip_schema.transport_format_list_len({'len': -1}) + missing_packet: dict[str, object] = {} with mock.patch('pcapkit.protocols.schema.internet.hip.warn') as warn: hip_schema.EncryptedParameter.pre_unpack(missing_packet) diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index aa0140c4f4..f3c9cd556e 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -356,10 +356,25 @@ class Gap(NamedTuple): 'pcapkit/protocols/internet/hip.py:822 -- Parameter.registry[128] is ' 'UnassignedParameter, because R1CounterParameter declares code=129 only'), - # Fifteen parameters whose ``_read_param_*`` stores a list-valued field as a + # Sixteen parameters whose ``_read_param_*`` stores a list-valued field as a # tuple, which ``_make_param_*`` passes straight back to a ``ListField`` # that accepts only a list. This is the class of defect the reconstruct step # exists to find: each one constructs and parses perfectly. + # + # TRANSPORT_FORMAT_LIST briefly left this group during #466's review: an + # early revision reused NAT_TRAVERSAL_MODE/ESP_TRANSFORM/HIP_TRANSPORT_MODE's + # shared ``two_octet_prefix_list_len`` guard for this parameter's ``formats`` + # field too, on the mistaken premise that its ``pkt['len'] - 2`` was + # byte-identical *for the same reason*. It is not: :rfc:`7401` Section + # 5.2.11 defines this parameter's ``Length`` as literally "2x number of TF + # types", with nothing between ``Length`` and the list to subtract -- unlike + # the other three, which each genuinely read a two-octet ``reserved``/ + # ``port`` field first. That reuse turned the parameter's own legitimate + # empty-list encoding (``Length = 0``) into a raise, and separately + # under-read every non-empty list by two octets on parse -- both fixed by + # ``transport_format_list_len`` in pcapkit/protocols/schema/internet/hip.py, + # which sizes the list at ``Length`` exactly. With that corrected, this case + # is back to failing the same way its fifteen siblings do. **{ f'hip-parameter/{name}': Gap( 'RECONSTRUCT', "unsupported type ", @@ -368,31 +383,11 @@ class Gap(NamedTuple): for name in ( 'ACK', 'DH_GROUP_LIST', 'HIP_CIPHER', 'NAT_TRAVERSAL_MODE', 'HIT_SUITE_LIST', 'REG_INFO', 'REG_REQUEST', 'REG_RESPONSE', - 'REG_FAILED', 'ESP_TRANSFORM', 'ACK_DATA', + 'REG_FAILED', 'TRANSPORT_FORMAT_LIST', 'ESP_TRANSFORM', 'ACK_DATA', 'ROUTE_DST', 'HIP_TRANSPORT_MODE', 'ROUTE_VIA', 'VIA_RVS', ) }, - # #463 gave four ``ListField`` length callbacks -- including this one's -- - # a shared floor-at-zero-and-raise guard (``two_octet_prefix_list_len``, - # pcapkit/protocols/schema/internet/hip.py:219), matching the other three - # sites' ``- 2`` accounting for a two-octet ``reserved``/``port`` field - # read ahead of the list. But ``TransportFormatListParameter`` has no such - # field -- RFC 7401's TRANSPORT_FORMAT_LIST is Type, Length, TF types, - # Padding, with nothing between Length and the list -- so its default - # (empty ``formats``) case packs with ``Length=0``, and the same - # byte-identical ``- 2`` that is correct for the other three now floors - # that construction to a raise instead of reaching the tuple/list - # mismatch this case used to hit further down the cycle. Pre-existing and - # out of #463's scope (a wrong offset, not a missing lower bound); tracked - # for a follow-up rather than fixed here, since the fix is directed to be - # byte-identical across all four sites. - 'hip-parameter/TRANSPORT_FORMAT_LIST': Gap( - 'CONSTRUCT', 'FieldValueError: HIP: invalid parameter length: 0', - 'pcapkit/protocols/schema/internet/hip.py:219 -- two_octet_prefix_list_len ' - 'assumes a 2-octet prefix that TransportFormatListParameter does not ' - 'have, so its default empty-list case (Length=0) now underflows'), - # ``_make_param_encrypted`` passes ``cipher=``, which is not a field of # ``EncryptedParameter`` -- so the cipher id is dropped with an # ``UnknownFieldWarning`` and never reaches the wire. The mismatch itself From 1fc3860c034f53e989ad076b7e54ecb7b30f07fd Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 01:26:42 -0400 Subject: [PATCH 3/3] protocols: size TRANSPORT_FORMAT_LIST entries at two octets, not one (#463) - transport_format_list_len()'s own docstring quoted RFC 7401 S5.2.11's "Length = 2x number of TF types" while formats' item_type stayed at EnumField(length=1, ...): an internally inconsistent fix. TF type values are HIP parameter type numbers (2050-4095 per the RFC), which need two octets; item_type=EnumField(length=2, ...) now matches, and matches HIPTransportModeParameter's mode field, the existing two-octet-item precedent in this file. - The two defects were not independent: _make_param_transport_format_list already computes len=2*len(tf_type), so with the length fix alone and a one-octet item_type, a two-entry list's len=4 read back as four entries (two spurious trailing zeros) instead of two -- confirmed before this commit, and confirmed gone after it, for one, two and three entries plus a case with two real TF types over 255. - Add a round-trip test that goes through the public maker (_make_param_transport_format_list) rather than a hand-built schema with a self-consistent len, since a hand-built case cannot expose a maker/schema disagreement. Includes ESP_TRANSFORM (4095) and HIP_TRANSPORT_MODE (7680), both within the RFC's TF type range and both values a one-octet item_type cannot pack (struct.error) -- so the one-octet assumption cannot return silently. - Rewrote the existing full-declared-length test's raw bytes for two-octet entries, and its docstring to name the item-width defect explicitly. Build: full suite green (0 failed, 1012 passed, 17 skipped, 1557 subtests, 1029 collected). EXPECTED_FAILURES unchanged -- the round-trip generator's own TRANSPORT_FORMAT_LIST case uses an empty formats list, unaffected by item width -- confirmed by running tests/protocols/test_option_roundtrip_ unit.py, not by assumption. examples/captures/options-internet.pcap is byte-for-byte unchanged (same reason). --- pcapkit/protocols/schema/internet/hip.py | 14 ++- tests/protocols/internet/test_hip_unit.py | 101 +++++++++++++++++----- 2 files changed, 93 insertions(+), 22 deletions(-) diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index 5fd4194e60..b4db0dd6b9 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -270,6 +270,18 @@ def transport_format_list_len(pkt: 'dict[str, Any]') -> 'int': rejected the parameter's own legitimate empty-list encoding (``Length = 0``, ``formats = []``) as malformed. + "2x number of TF types" also fixes each ``TF type`` entry's own width at + two octets -- the same width the base :class:`Parameter` class uses for + its own ``type`` field, since a TF type *is* a HIP parameter type number + -- which is why ``formats``' ``item_type`` is + ``EnumField(length=2, ...)``, matching :class:`HIPTransportModeParameter`'s + ``mode`` rather than the one-octet items of :func:`two_octet_prefix_list_len`'s + other two call sites. This function only answers how many *bytes* the + list occupies; getting that number right and the item width wrong (as a + still-earlier revision did, at one octet) still corrupts every non-empty + list, just by reading twice as many entries as the wire holds instead of + dropping octets. + Args: pkt: Parameter unpacked schema. @@ -908,7 +920,7 @@ class TransportFormatListParameter(Parameter, code=Enum_Parameter.TRANSPORT_FORM #: Transport formats. formats: 'list[Enum_Parameter]' = ListField( length=transport_format_list_len, - item_type=EnumField(length=1, namespace=Enum_Parameter), + item_type=EnumField(length=2, namespace=Enum_Parameter), ) #: Padding. padding: 'bytes' = PaddingField(length=lambda pkt: (8 - (pkt['len'] % 8)) % 8) diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index ad2623558c..74dcae0ebd 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -1921,22 +1921,31 @@ def test_hip_transport_format_list_parameter_accepts_an_empty_list_at_length_zer self.assertEqual(copies[1].tf_type, ()) def test_hip_transport_format_list_parameter_parses_the_full_declared_length(self) -> None: - """#463/#466: the same ``- 2`` also under-read every *non-empty* - ``TRANSPORT_FORMAT_LIST``, silently dropping its last two octets. - - Pre-existing on ``main``, and not introduced by #466's guard: with - ``Length = 4`` and four wire octets of transport format entries, - ``pkt['len'] - 2 == 2`` sized the list at only two entries. - Confirmed directly against a ``main``-shaped schema object before - this fix: ``TransportFormatListParameter(type=..., len=4, - formats=[10, 20, 30, 40])`` packs to twelve octets and reads back as - ``formats == [Unassigned_10, Unassigned_20]`` -- the ``30`` and - ``40`` octets are simply gone, with no exception and no diagnostic, - because ``Length = 4`` never underflows and so never reaches - #460/#463's floor-and-raise guard at all. Removing the ``- 2`` - (:func:`~pcapkit.protocols.schema.internet.hip. - transport_format_list_len`) makes the list length equal to - ``Length`` exactly, so all four entries now survive the round trip. + """#463/#466: a since-corrected revision's ``- 2`` under-read every + *non-empty* ``TRANSPORT_FORMAT_LIST``, silently dropping trailing + entries; this proves the fully declared list now survives. + + Pre-#463, on ``main``, with the then-one-octet ``item_type`` still in + place: ``Length = 4`` and four one-octet transport format entries + underflowed to ``pkt['len'] - 2 == 2``, sizing the list at only two + entries and silently dropping the last two octets. That bug is + independent of the item-width defect fixed alongside it below -- + confirmed directly against a ``main``-shaped schema object: + ``TransportFormatListParameter(type=..., len=4, formats=[10, 20, 30, + 40])`` packs to twelve octets and reads back as ``formats == + [Unassigned_10, Unassigned_20]``. + + Each ``TF type`` entry is two octets, not one -- :rfc:`7401` Section + 5.2.11 fixes it at "2x number of TF types" and the diagram shows two + 16-bit ``TF type`` fields per 32-bit row, matching + :class:`HIPTransportModeParameter`'s ``mode`` field rather than the + one-octet items :func:`two_octet_prefix_list_len`'s other two call + sites use. So this test's four entries are four *two*-octet values + (``Length = 8``), and with both defects fixed -- + :func:`~pcapkit.protocols.schema.internet.hip. + transport_format_list_len` returning ``Length`` unchanged, and + ``item_type=EnumField(length=2, ...)`` -- all four survive the round + trip rather than being dropped or double-counted. """ from pcapkit.const.hip.parameter import Parameter @@ -1948,11 +1957,13 @@ def test_hip_transport_format_list_parameter_parses_the_full_declared_length(sel fixed = bytes([0x3b, 0x07, 0x00, 0x01]) + bytes(2) + bytes(2) + bytes(16) + bytes(16) self.assertEqual(len(fixed), 40) - # Two copies of: type(2)=2049 len(2)=4, four one-octet format entries, - # four octets of padding (this module's padding rule pads the - # *contents* to eight, ignoring the four-octet type-and-length - # header) -- 12 octets each, 24 octets together. - one = (2049).to_bytes(2, 'big') + (4).to_bytes(2, 'big') + bytes([0x0a, 0x14, 0x1e, 0x28]) + bytes(4) + # Two copies of: type(2)=2049 len(2)=8, four two-octet format entries + # (10, 20, 30, 40), no padding needed (8 is already a multiple of + # eight under this module's padding rule, which pads the *contents* + # to eight and ignores the four-octet type-and-length header) -- + # 12 octets each, 24 octets together. + one = (2049).to_bytes(2, 'big') + (8).to_bytes(2, 'big') + b''.join( + n.to_bytes(2, 'big') for n in (10, 20, 30, 40)) self.assertEqual(len(one), 12) raw = fixed + one * 2 @@ -1963,6 +1974,54 @@ def test_hip_transport_format_list_parameter_parses_the_full_declared_length(sel self.assertEqual(len(copy.tf_type), 4) self.assertEqual([int(tf) for tf in copy.tf_type], [10, 20, 30, 40]) + def test_hip_transport_format_list_parameter_round_trips_through_the_maker(self) -> None: + """#463/#466: the public maker and the schema's own read path must + agree on the entry width, or a hand-built test cannot catch either + one being wrong -- which is exactly what happened here. + + ``_make_param_transport_format_list`` computes ``len=2 * + len(tf_type)``, already assuming two-octet entries, independently of + whatever ``item_type`` the ``formats`` field declares. A hand-built + ``TransportFormatListParameter(type=..., len=..., formats=...)`` + picks a self-consistent ``len`` by hand and so cannot expose a + maker/schema disagreement -- which is why the one-octet + ``item_type`` survived review once already. Building through the + maker instead pins the two to agree: with one-octet items and two + real entries, the maker's ``len=4`` reads back as ``len // 1 == 4`` + entries -- ``[10, 20, 0, 0]``, two spurious trailing zeros -- while + with two-octet items it reads back as ``len // 2 == 2`` entries, + matching what went in. + + Includes ``Parameter.ESP_TRANSFORM`` (4095) and + ``Parameter.HIP_TRANSPORT_MODE`` (7680), both real HIP parameter + type numbers over 255 and so within :rfc:`7401`'s own TF type range + (2050-4095) -- and both exactly the values a one-octet ``item_type`` + cannot pack at all (``struct.error: 'B' format requires 0 <= number + <= 255``), so the one-octet assumption cannot pass this test + silently by falling back to small integers. + + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.schema.internet import hip as hip_schema + + proto = object.__new__(HIP) + cases = ( + [], + [Parameter.ESP_TRANSFORM], + [Parameter.ESP_TRANSFORM, Parameter.HIP_TRANSPORT_MODE], + [Parameter.ESP_TRANSFORM, Parameter.HIP_TRANSPORT_MODE, Parameter.HIP_CIPHER], + ) + for formats in cases: + with self.subTest(formats=formats): + schema = proto._make_param_transport_format_list( + Parameter.TRANSPORT_FORMAT_LIST, version=2, formats=list(formats)) + self.assertEqual(schema.len, 2 * len(formats)) + + packed = bytes(schema) + reparsed = hip_schema.TransportFormatListParameter.unpack(packed) + self.assertEqual(list(reparsed.formats), list(formats)) + def test_hip_esp_transform_parameter_rejects_underflowing_length(self) -> None: """#463: an ``ESP_TRANSFORM`` parameter's ``Length`` too small for its own two-octet ``reserved`` field must raise, not silently drop