diff --git a/docs/source/pcapkit/protocols/internet/ipv6_route.rst b/docs/source/pcapkit/protocols/internet/ipv6_route.rst index 67d005ac2f..8b2f54b5e4 100644 --- a/docs/source/pcapkit/protocols/internet/ipv6_route.rst +++ b/docs/source/pcapkit/protocols/internet/ipv6_route.rst @@ -35,6 +35,7 @@ Octets Bits Name Description .. automethod:: make .. automethod:: _make_data + .. automethod:: _make_hdr_ext_len .. automethod:: _read_data_type_none .. automethod:: _read_data_type_src @@ -92,6 +93,7 @@ Auxiliary Functions ~~~~~~~~~~~~~~~~~~~ .. autofunction:: pcapkit.protocols.schema.internet.ipv6_route.ipv6_route_data_selector +.. autofunction:: pcapkit.protocols.schema.internet.ipv6_route.ipv6_route_data_length Data Models ----------- diff --git a/pcapkit/protocols/internet/ipv6_route.py b/pcapkit/protocols/internet/ipv6_route.py index 3abda59c76..e836577e97 100644 --- a/pcapkit/protocols/internet/ipv6_route.py +++ b/pcapkit/protocols/internet/ipv6_route.py @@ -41,6 +41,7 @@ from pcapkit.protocols.schema.internet.ipv6_route import SourceRoute as Schema_SourceRoute from pcapkit.protocols.schema.internet.ipv6_route import Type2 as Schema_Type2 from pcapkit.protocols.schema.internet.ipv6_route import UnknownType as Schema_UnknownType +from pcapkit.protocols.schema.internet.ipv6_route import ipv6_route_data_length from pcapkit.protocols.schema.schema import Schema from pcapkit.utilities.exceptions import ProtocolError, UnsupportedCall from pcapkit.utilities.warnings import RegistryWarning, warn @@ -214,6 +215,42 @@ def read(self, length: 'Optional[int]' = None, *, extension: 'bool' = False, # return ipv6_route return self._decode_next_layer(ipv6_route, schema.next, length - ipv6_route.length) + @staticmethod + def _make_hdr_ext_len(data_length: 'int') -> 'int': + """Compute ``Hdr Ext Len`` for a type-specific data payload of ``data_length`` octets. + + Per :rfc:`8200#section-4.4`, ``Hdr Ext Len`` is *"the length of the + Routing header in 8-octet units, not including the first 8 + octets"*. The routing header's fixed part (``next``/``length``/ + ``type``/``seg_left``) is 4 octets, so the total on-the-wire header + is ``4 + data_length`` octets; equating that to ``8 + 8 * + hdr_ext_len`` and solving gives ``hdr_ext_len = (data_length - 4) / + 8``. ``ipv6_route_data_length`` in + :mod:`pcapkit.protocols.schema.internet.ipv6_route` is this + expression's inverse, used on the read side. + + This is the *only* place ``Hdr Ext Len`` is computed on the write + side: :meth:`make` used to compute it twice, once per branch, in two + different (and both wrong) units -- that duplication, not either + expression individually, is what let the two drift and is why there + is one helper now rather than two call sites. Do NOT "simplify" the + ``- 4`` / ``/ 8`` away: the units either side of it differ (octets + vs. 8-octet units), and dropping the offset silently reinterprets + the field, which is exactly the defect #487 fixed (compare the + ``* 8`` unit bug behind #483 in the scapy adapter). + + Args: + data_length: packed length, in octets, of the type-specific data + (i.e. ``len(data_val.pack())`` for a :class:`~pcapkit.protocols. + schema.schema.Schema`-based payload, or the padded raw + :obj:`bytes` length for the unknown/raw-bytes case). + + Returns: + The ``Hdr Ext Len`` field value. + + """ + return math.ceil(max(0, data_length - 4) / 8) + def make(self, dst: 'Optional[IPv6Address | str | int| bytes]' = None, next: 'Enum_TransType | StdlibEnum | AenumEnum | str | int' = Enum_TransType.UDP, @@ -257,8 +294,13 @@ def make(self, reversed=type_reversed, pack=False)) if isinstance(data, bytes): - length = math.ceil((len(data) + 4) / 8) - data_val = data.ljust(length * 8 - 4, b'\x00') # type: bytes | Schema_RoutingType + # ``data`` here *is* the type-specific data (no per-type + # constructor involved), so pad it out to the next octet count + # ``_make_hdr_ext_len`` can express exactly, then use that same + # helper -- rather than a third, separate expression -- to derive + # ``Hdr Ext Len`` from the now-aligned length. + length = self._make_hdr_ext_len(len(data)) + data_val = data.ljust(ipv6_route_data_length(length), b'\x00') # type: bytes | Schema_RoutingType elif isinstance(data, (dict, Data_IPv6_Route)): name = self._lookup_registry(self.__routing__, type_val) if isinstance(name, str): @@ -273,9 +315,9 @@ def make(self, data_val = meth(type_val, dst=dst_val, **data) else: data_val = meth(type_val, data, dst=dst_val) - length = len(data_val.pack()) + length = self._make_hdr_ext_len(len(data_val.pack())) elif isinstance(data, Schema): - length = math.ceil((len(data.pack()) + 4) / 8) + length = self._make_hdr_ext_len(len(data.pack())) data_val = data else: raise ProtocolError(f'{self.alias}: invalid routing data type: {data.__class__}') @@ -458,7 +500,13 @@ def _read_data_type_src(self, schema: 'Schema_SourceRoute', *, header: 'Schema_I Parsed route data. """ - if (header.length - 8) % 16 != 0: + # ``header.length`` is ``Hdr Ext Len``, in 8-octet units (:rfc:`8200 + # #section-4.4`), not octets -- each 16-octet address costs 2 of + # those units, so a well-formed Source Route header always carries + # an even ``Hdr Ext Len``. The previous ``(header.length - 8) % 16`` + # check assumed ``header.length`` was already a total octet count, + # which is never true of this field; see #487. + if header.length % 2 != 0: raise ProtocolError(f'{self.alias}: [TypeNo {header.type}] invalid format') ipv6_route = Data_SourceRoute( @@ -499,10 +547,14 @@ def _read_data_type_2(self, schema: 'Schema_Type2', *, header: 'Schema_IPv6_Rout Parsed route data. Raises: - ProtocolError: If ``length`` is **NOT** ``24``. + ProtocolError: If ``Hdr Ext Len`` is **NOT** ``2``. """ - if header.length != 24: + # A Type 2 Routing header is fixed at 24 octets total (4 fixed + 4 + # reserved + 16-octet home address), so its ``Hdr Ext Len`` -- in + # 8-octet units, not octets (:rfc:`8200#section-4.4`) -- is always + # ``2``; a literal ``24`` here could never match. See #487. + if header.length != 2: raise ProtocolError(f'{self.alias}: [TypeNo {header.type}] invalid format') ipv6_route = Data_Type2( @@ -542,6 +594,19 @@ def _read_data_type_rpl(self, schema: 'Schema_RPL', *, header: 'Schema_IPv6_Rout Parsed route data. """ + # NOTE: this guard has the same surface shape as the Source Route and + # Type 2 unit confusion #487 fixed above -- ``header.length`` is + # ``Hdr Ext Len`` in 8-octet units, not octets, and ``% 16`` reads + # like a leftover assumption that it was already a total octet + # 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. if header.length % 16 != 0: raise ProtocolError(f'{self.alias}: [TypeNo {header.type}] invalid format') diff --git a/pcapkit/protocols/schema/internet/ipv6_route.py b/pcapkit/protocols/schema/internet/ipv6_route.py index 6813231e08..6374ad3399 100644 --- a/pcapkit/protocols/schema/internet/ipv6_route.py +++ b/pcapkit/protocols/schema/internet/ipv6_route.py @@ -37,6 +37,31 @@ class PadInfo(TypedDict): pad_len: int +def ipv6_route_data_length(hdr_ext_len: 'int') -> 'int': + """Length, in octets, of the IPv6-Route type-specific data for a given ``Hdr Ext Len``. + + Per :rfc:`8200#section-4.4`, ``Hdr Ext Len`` is *"the length of the + Routing header in 8-octet units, not including the first 8 octets"* -- + i.e. the total on-the-wire header is ``8 + 8 * hdr_ext_len`` octets. Of + that, the 4 octets of ``next``/``length``/``type``/``seg_left`` are not + part of the type-specific data, so the data itself -- what + :func:`ipv6_route_data_selector` hands to the nested ``RoutingType`` + schema -- is ``4 + 8 * hdr_ext_len`` octets. This is the single place + that arithmetic is done on the read side; see + :meth:`~pcapkit.protocols.internet.ipv6_route.IPv6_Route._make_hdr_ext_len` + for its inverse on the write side. Do NOT drop the ``4 +``: that turns + the field back into raw octets and is the exact defect #487 fixed. + + Args: + hdr_ext_len: raw ``Hdr Ext Len`` field value, as read off the wire. + + Returns: + Length, in octets, of the type-specific data. + + """ + return 4 + hdr_ext_len * 8 + + def ipv6_route_data_selector(pkt: 'dict[str, Any]') -> 'Field': """Selector function for :attr:`IPv6_Route.data` field. @@ -51,7 +76,7 @@ def ipv6_route_data_selector(pkt: 'dict[str, Any]') -> 'Field': """ type = cast('Enum_Routing', pkt['type']) schema = RoutingType.registry[type] - return SchemaField(length=pkt['length'] * 8, schema=schema) + return SchemaField(length=ipv6_route_data_length(cast('int', pkt['length'])), schema=schema) @schema_final diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index dd4c14b337..71699e8c61 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -316,7 +316,9 @@ def test_ipv6_route_readers_and_constructors_cover_registered_types(self) -> Non with self.assertRaises(ProtocolError): proto._read_data_type_src(route_schema.SourceRoute(ip=[]), header=header) - type2_header = types.SimpleNamespace(next=TransType.TCP, length=24, + # Hdr Ext Len for a Type 2 header is fixed at 2 (24 total octets), in + # 8-octet units per :rfc:`8200#section-4.4` -- not 24. See #487. + type2_header = types.SimpleNamespace(next=TransType.TCP, length=2, type=Routing.Type_2_Routing_Header, seg_left=1) type2 = proto._read_data_type_2(route_schema.Type2(ip='2001:db8::2'), header=type2_header) self.assertEqual(str(type2.ip), '2001:db8::2') @@ -461,7 +463,11 @@ def test_ipv6_route_read_make_registry_and_property_edges(self) -> None: decode.assert_called_once() made_bytes = proto.make(type=Routing.Source_Route, data=b'abcd') - self.assertEqual(made_bytes.length, 1) + # Hdr Ext Len in 8-octet units (:rfc:`8200#section-4.4`): 4 octets of + # raw data is exactly the 4-octet fixed part every routing type's + # data starts with, so no additional 8-octet units are needed. See + # #487 -- this used to assert ``1``, the pre-fix octet-count value. + self.assertEqual(made_bytes.length, 0) self.assertEqual(made_bytes.data, b'abcd') made_dict = proto.make(type=Routing.Source_Route, data={'ip': ['2001:db8::2']}) self.assertEqual(made_dict.type, Routing.Source_Route) @@ -477,7 +483,10 @@ def test_ipv6_route_read_make_registry_and_property_edges(self) -> None: type=Routing.Type_2_Routing_Header, data=route_schema.Type2(ip='2001:db8::4'), ) - self.assertEqual(made_schema.length, 3) + # 4 reserved + 16-octet address = 20 octets of type-specific data -> + # Hdr Ext Len = (20 - 4) / 8 = 2 (:rfc:`8200#section-4.4`). See #487 + # -- this used to assert ``3``, the pre-fix (wrong-sign) value. + self.assertEqual(made_schema.length, 2) with self.assertRaises(ProtocolError): proto.make(data=object()) @@ -2054,14 +2063,20 @@ def test_ipv6_route_source_route_make_accepts_tuple_and_list_addresses(self) -> Boundaries: zero addresses, one, and two, so a fix that special-cases "empty" or stops one item short of the general case is still caught. - The construct-then-parse check below goes through + #487 update: this test previously round-tripped through ``Schema_SourceRoute.unpack`` directly rather than a full - ``IPv6_Route`` read (i.e. ``_read_data_type_src``): that method's own - ``(header.length - 8) % 16`` check rejects every ``length`` - ``IPv6_Route.make`` itself computes for this routing type -- for any - address count, tuple or list alike -- which looks like a pre-existing - defect independent of #480 and is reported separately rather than - papered over here. + ``IPv6_Route`` read, because ``_read_data_type_src``'s own + ``(header.length - 8) % 16`` guard rejected every ``length`` + ``IPv6_Route.make`` computed for this routing type, for any address + count -- and separately, the ``length`` ``make`` computed was itself + wrong (raw octets, not the 8-octet units :rfc:`8200#section-4.4` + specifies for ``Hdr Ext Len``). Both halves are fixed now (see #487), + so this goes through the full ``IPv6_Route`` construct-then-parse + path -- which is a strictly stronger check than unpacking the nested + schema alone -- and the expected on-wire ``Hdr Ext Len`` byte below + changed from the old (buggy) octet counts ``0x04``/``0x14``/``0x24`` + to the RFC-correct 8-octet-unit values ``0x00``/``0x02``/``0x04``. + The tuple-versus-list assertion this test exists for is unchanged. """ from ipaddress import ip_address @@ -2069,7 +2084,6 @@ def test_ipv6_route_source_route_make_accepts_tuple_and_list_addresses(self) -> from pcapkit.const.ipv6.routing import Routing from pcapkit.const.reg.transtype import TransType from pcapkit.protocols.internet.ipv6_route import IPv6_Route - from pcapkit.protocols.schema.internet import ipv6_route as route_schema addr1 = ip_address('2001:db8::1') addr2 = ip_address('2001:db8::2') @@ -2088,11 +2102,11 @@ def make_and_pack(ip): # (case name, tuple form, list form, expected on-wire bytes) cases = [ ('empty', (), [], - bytes([0x11, 0x04, 0x00, 0x00]) + b'\x00' * 4), + bytes([0x11, 0x00, 0x00, 0x00]) + b'\x00' * 4), ('single', (addr1,), [addr1], - bytes([0x11, 0x14, 0x00, 0x01]) + b'\x00' * 4 + addr1.packed), + bytes([0x11, 0x02, 0x00, 0x01]) + b'\x00' * 4 + addr1.packed), ('double', (addr1, addr2), [addr1, addr2], - bytes([0x11, 0x24, 0x00, 0x02]) + b'\x00' * 4 + addr1.packed + addr2.packed), + bytes([0x11, 0x04, 0x00, 0x02]) + b'\x00' * 4 + addr1.packed + addr2.packed), ] for name, as_tuple, as_list, expected in cases: @@ -2107,12 +2121,72 @@ def make_and_pack(ip): # 4 octets fixed header + 4 reserved + 16 per address self.assertEqual(len(packed_tuple), 8 + 16 * len(as_list)) - # construct-then-parse: the addresses come back, in the same - # order, through the schema's own pack/unpack pair - data_bytes = packed_tuple[4:] - parsed = route_schema.SourceRoute.unpack( - data_bytes, len(data_bytes), {'__length__': len(data_bytes)}) - self.assertEqual(list(parsed.ip), as_list) + # construct-then-parse, through the full public API: the + # addresses come back, in the same order, from a real + # IPv6_Route.read() of the bytes IPv6_Route.make() produced + info = IPv6_Route(io.BytesIO(packed_tuple), len(packed_tuple)).info + self.assertEqual(list(info.ip), as_list) + + def test_ipv6_route_source_route_construct_then_parse_round_trip(self) -> None: + """#487: ``IPv6_Route.make()`` for a Source Route (Type 0) header emitted + bytes its own parser rejected, for *every* address count -- the two + branches of ``make`` disagreed with each other, and both disagreed with + :rfc:`8200#section-4.4`, on the unit of ``Hdr Ext Len``. This is the + check the issue asked for explicitly: construct through the public API, + parse the result straight back, for each of 0, 1, 2 and 3 addresses -- + the boundaries, rather than one comfortable middle case. + + """ + from ipaddress import IPv6Address + + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + + # (n addresses, expected total octets, expected Hdr Ext Len) + cases = [ + (0, 8, 0), + (1, 24, 2), + (2, 40, 4), + (3, 56, 6), + ] + + for n, expected_octets, expected_hdr_ext_len in cases: + with self.subTest(addresses=n): + addrs = tuple(IPv6Address(f'2001:db8::{i + 1}') for i in range(n)) + + proto = object.__new__(IPv6_Route) + raw = proto.make(type=Routing.Source_Route, data={'ip': addrs}, + next=0, seg_left=n).pack() + + self.assertEqual(len(raw), expected_octets) + self.assertEqual(raw[1], expected_hdr_ext_len) + + info = IPv6_Route(io.BytesIO(raw), len(raw)).info + self.assertEqual(tuple(info.ip), addrs) + + def test_ipv6_route_source_route_parses_hand_built_wire_form(self) -> None: + """#487's more important half: a hand-built, spec-correct Source Route + header -- one this library never constructed -- must parse, independent + of whatever ``IPv6_Route.make()`` does. A round-trip test alone can pass + on two mistakes that cancel out (a constructor and a parser that agree + with each other but not with the wire format); this does not go through + ``make()`` at all, so it catches exactly that failure mode. Bytes match + the issue's own hand-built example: ``next=0``, ``Hdr Ext Len=2``, + ``type=0`` (Source Route), ``seg_left=1``, 4 reserved octets, one + 16-octet address, 24 octets total. + + """ + from ipaddress import IPv6Address + + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + + addr = IPv6Address('2001:db8::1') + hand_built = bytes([0x00, 0x02, 0x00, 0x01]) + b'\x00' * 4 + addr.packed + self.assertEqual(len(hand_built), 24) + + info = IPv6_Route(io.BytesIO(hand_built), len(hand_built)).info + self.assertEqual(tuple(info.ip), (addr,)) + self.assertEqual(info.length, 24) if __name__ == '__main__': diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index e3a709a0c7..07805bdfb4 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -272,30 +272,25 @@ class Gap(NamedTuple): # -- IPv6-Route ----------------------------------------------------------- - # ``IPv6_Route.make`` writes a raw octet count into ``length``, which is an - # 8-octet-unit field, on the dict and Data paths -- while the bytes and - # Schema paths divide by eight. The read-side guards then reject it, and - # those guards compare the unit field against octet counts too, so the - # RFC-correct value would fail as well. - # Both fragments are tuples rather than the whole message, because the - # message itself is defective: these two guards interpolate a bare ``type`` - # into an f-string in a method that has no ``type`` parameter, so the name - # resolves to the builtin and the detail reads ``[TypeNo ]`` - # (issue #442). Matching the literal rendering would pin that bug into this - # table and turn it red when #442 is fixed, which says nothing about whether - # the round trip closes. The stable parts either side of it do pin the site: - # ``[TypeNo`` occurs at exactly three lines of ``ipv6_route.py`` (:462, :506, - # :546), against 205 occurrences of ``'invalid format'`` tree-wide. - 'ipv6-route-type/Source_Route': Gap( - 'CONSTRUCT', ('IPv6-Route', '[TypeNo', 'invalid format'), - 'pcapkit/protocols/internet/ipv6_route.py:276 -- length in octets, not ' - 'in 8-octet units; guard at :461'), - 'ipv6-route-type/Type_2_Routing_Header': Gap( - 'CONSTRUCT', ('IPv6-Route', '[TypeNo', 'invalid format'), - 'pcapkit/protocols/internet/ipv6_route.py:276; guard at :505'), + # ``Source_Route`` and ``Type_2_Routing_Header`` used to be recorded here: + # ``IPv6_Route.make`` wrote a raw octet count into ``length`` (``Hdr Ext + # Len``) on the dict and Data paths, a different-and-also-wrong expression + # on the bytes and Schema paths, and separately, ``ipv6_route_data_selector`` + # (pcapkit/protocols/schema/internet/ipv6_route.py) sized the nested + # routing-data schema 4 octets short of the wire, missing the "Reserved" + # field every routing type's data starts with -- so a hand-built, + # spec-correct header failed to parse independently of anything ``make`` + # produced. #487 fixed both: one shared helper + # (``IPv6_Route._make_hdr_ext_len``) computes ``Hdr Ext Len`` in the + # 8-octet units :rfc:`8200#section-4.4` specifies, on every ``make`` + # branch, and ``ipv6_route_data_selector`` accounts for the 4-octet + # 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. + # the list the constructor was handed. 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 '