diff --git a/pcapkit/protocols/internet/hopopt.py b/pcapkit/protocols/internet/hopopt.py index a02f91e45..926321475 100644 --- a/pcapkit/protocols/internet/hopopt.py +++ b/pcapkit/protocols/internet/hopopt.py @@ -1356,20 +1356,19 @@ def _make_pad_options(self, offset: 'int') -> 'tuple[list[Schema_PadOption], int A ``PadN`` option spends two octets on its own type and ``Opt Data Len`` fields before any padding data, so occupying ``pad_len`` octets means an ``Opt Data Len`` of ``pad_len - 2``. - One or two octets are padded with ``Pad1`` options instead, that - being the only form which can occupy a single octet - [:rfc:`8200#section-4.2`]; two octets need two separate ``Pad1`` - schemas, since a schema instance is mutable and must not be shared - between two entries of the option list. + One octet is padded with a ``Pad1`` option instead, that being the + only form which can occupy a single octet. Two octets are one + ``PadN`` with an ``Opt Data Len`` of ``0``, not two ``Pad1``: + :rfc:`8200#section-4.2` says a ``PadN`` should be used rather than + multiple ``Pad1`` options. """ if offset % 8 == 0: return [], 0 pad_len = 8 - (offset % 8) - if pad_len <= 2: - return [self._make_opt_pad(Enum_Option.Pad1, length=0) # type: ignore[arg-type] - for _ in range(pad_len)], pad_len + if pad_len == 1: + return [self._make_opt_pad(Enum_Option.Pad1, length=0)], pad_len # type: ignore[arg-type] return [self._make_opt_pad(Enum_Option.PadN, length=pad_len - 2)], pad_len # type: ignore[arg-type] def _make_hopopt_options(self, options: 'list[Schema_Option | tuple[Enum_Option, dict[str, Any]] | bytes] | Option') -> 'tuple[list[Schema_Option | bytes], int]': diff --git a/pcapkit/protocols/internet/ipv6_opts.py b/pcapkit/protocols/internet/ipv6_opts.py index 87c924e40..a7c30ac01 100644 --- a/pcapkit/protocols/internet/ipv6_opts.py +++ b/pcapkit/protocols/internet/ipv6_opts.py @@ -1345,20 +1345,19 @@ def _make_pad_options(self, offset: 'int') -> 'tuple[list[Schema_PadOption], int A ``PadN`` option spends two octets on its own type and ``Opt Data Len`` fields before any padding data, so occupying ``pad_len`` octets means an ``Opt Data Len`` of ``pad_len - 2``. - One or two octets are padded with ``Pad1`` options instead, that - being the only form which can occupy a single octet - [:rfc:`8200#section-4.2`]; two octets need two separate ``Pad1`` - schemas, since a schema instance is mutable and must not be shared - between two entries of the option list. + One octet is padded with a ``Pad1`` option instead, that being the + only form which can occupy a single octet. Two octets are one + ``PadN`` with an ``Opt Data Len`` of ``0``, not two ``Pad1``: + :rfc:`8200#section-4.2` says a ``PadN`` should be used rather than + multiple ``Pad1`` options. """ if offset % 8 == 0: return [], 0 pad_len = 8 - (offset % 8) - if pad_len <= 2: - return [self._make_opt_pad(Enum_Option.Pad1, length=0) # type: ignore[arg-type] - for _ in range(pad_len)], pad_len + if pad_len == 1: + return [self._make_opt_pad(Enum_Option.Pad1, length=0)], pad_len # type: ignore[arg-type] return [self._make_opt_pad(Enum_Option.PadN, length=pad_len - 2)], pad_len # type: ignore[arg-type] def _make_ipv6_opts(self, options: 'list[Schema_Option | tuple[Enum_Option, dict[str, Any]] | bytes] | Option') -> 'tuple[list[Schema_Option | bytes], int]': diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index be332e597..6376ad898 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7742,20 +7742,19 @@ def _make_pad_options(self, offset: 'int') -> 'tuple[list[Schema_PadOption], int A ``PadN`` option spends two octets on its own type and ``Option Length`` fields before any padding data, so occupying ``pad_len`` octets means an ``Option Length`` of ``pad_len - 2``. - One or two octets are padded with ``Pad1`` options instead, that - being the only form which can occupy a single octet - [:rfc:`6275#section-6.2.2`]; two octets need two separate ``Pad1`` - schemas, since a schema instance is mutable and must not be shared - between two entries of the option list. + One octet is padded with a ``Pad1`` option instead, that being the + only form which can occupy a single octet. Two octets are one + ``PadN`` with an ``Option Length`` of ``0``, not two ``Pad1``: + :rfc:`6275#section-6.2.2` says a ``PadN`` should be used rather than + multiple ``Pad1`` options. """ if offset % 8 == 0: return [], 0 pad_len = 8 - (offset % 8) - if pad_len <= 2: - return [self._make_opt_pad(Enum_Option.Pad1, length=0) # type: ignore[arg-type] - for _ in range(pad_len)], pad_len + if pad_len == 1: + return [self._make_opt_pad(Enum_Option.Pad1, length=0)], pad_len # type: ignore[arg-type] return [self._make_opt_pad(Enum_Option.PadN, length=pad_len - 2)], pad_len # type: ignore[arg-type] def _pad_mh_message(self, data: 'Schema_Packet | bytes') -> 'Schema_Packet | bytes': diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 6b467ebf6..5f3606261 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -1068,17 +1068,19 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) - self.assertEqual(schema_length, 14) self.assertEqual(type(schema_options[0]).__name__, 'UnassignedOption') - # NOTE: Two ``Pad1`` options are emitted when the shortfall is exactly 2 - # octets, i.e. when ``2 + opt_len`` is 6 past a multiple of 8. That is a - # 4-octet option, not the 6-octet one this case used to pass -- 2 + 6 is - # already aligned and now needs no padding at all. - pad1_options, pad1_length = make_options([ + # NOTE: A shortfall of exactly 2 octets, i.e. ``2 + opt_len`` 6 past a + # multiple of 8, is one ``PadN`` of ``Opt Data Len`` 0 rather than two + # ``Pad1`` (#1793). That is a 4-octet option, not the 6-octet one this + # case used to pass -- 2 + 6 is already aligned and needs no padding. + pad2_options, pad2_length = make_options([ bytes([Option.CALIPSO]) + b'123', ]) - # 4 octets of CALIPSO + two 1-octet Pad1 = 6; 2 + 6 = 8 - self.assertEqual(pad1_length, 6) + # 4 octets of CALIPSO + a 2-octet PadN = 6; 2 + 6 = 8 + self.assertEqual(pad2_length, 6) self.assertEqual([type(item).__name__ if not isinstance(item, bytes) else 'bytes' - for item in pad1_options], ['bytes', 'PadOption', 'PadOption']) + for item in pad2_options], ['bytes', 'PadOption']) + self.assertEqual(pad2_options[1].type, Option.PadN) + self.assertEqual(pad2_options[1].pack(), b'\x01\x00') # NOTE: Likewise, the option that needs no padding is the 6-octet one; the # 8-octet option this case used to pass leaves 2 + 8 = 10 and now draws 6 @@ -1113,7 +1115,7 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) - for label, (built, built_len) in { 'bytes + padn': (options, total_length), 'schema + padn': (schema_options, schema_length), - 'two pad1': (pad1_options, pad1_length), + 'two-octet padn': (pad2_options, pad2_length), 'no padding': (no_pad_options, no_pad_length), 'from dict': (dict_options, dict_length), }.items(): @@ -2287,8 +2289,9 @@ def _assert_constructed_header_round_trips(self, protocol_cls: type) -> None: The option lengths are chosen for the padding they draw: the whole header, its two fixed octets included, has to be a multiple of 8, so an - option of ``8n + 5`` octets needs a single ``Pad1``, one of ``8n + 4`` - needs two, and anything else needs a ``PadN``. + option of ``8n + 5`` octets needs a single ``Pad1``, and anything else + short of the boundary needs a ``PadN`` -- including one of ``8n + 4``, + whose two spare octets are a ``PadN`` of ``Opt Data Len`` 0. """ from pcapkit.const.ipv6.option import Option @@ -2298,9 +2301,9 @@ def _assert_constructed_header_round_trips(self, protocol_cls: type) -> None: # a 5-octet option leaves one octet, which only a Pad1 can fill ('one pad1', [(Option.get(250), {'data': b'abc'})], 8, [(Option.get(250), 5), (Option.Pad1, 1)]), - # a 4-octet option leaves two, i.e. two Pad1 rather than a PadN - ('two pad1', [(Option.Router_Alert, {'alert': 0})], 8, - [(Option.Router_Alert, 4), (Option.Pad1, 1), (Option.Pad1, 1)]), + # a 4-octet option leaves two, i.e. one PadN of length 0 (#1793) + ('two-octet padn', [(Option.Router_Alert, {'alert': 0})], 8, + [(Option.Router_Alert, 4), (Option.PadN, 2)]), # three or more spare octets are a PadN, whose own two octets of # overhead have to come out of the padding it is asked to occupy ('padn', [(Option.Tunnel_Encapsulation_Limit, {'limit': 3})], 8, diff --git a/tests/protocols/internet/test_mh_flags_reserved_unit.py b/tests/protocols/internet/test_mh_flags_reserved_unit.py index 4ef547afb..bafe1f982 100644 --- a/tests/protocols/internet/test_mh_flags_reserved_unit.py +++ b/tests/protocols/internet/test_mh_flags_reserved_unit.py @@ -52,6 +52,14 @@ 'PerSessionBitRateAttribute': ('3b020000000000003a0e00000000000003063fff000003e8', 14), } +#: ``site -> octets make() emits``, where they differ from :data:`FRAMES`. Two +#: octets of fresh padding are one ``PadN`` of length 0 (#1793), whereas these +#: frames end in two ``Pad1``, which a parsed header keeps on rebuild. +MADE = { + 'DynamicIPMulticastSelectorOption': '3b0100000000000036048f7f00000100', + 'IPv4TrafficOffloadSelectorOption': '3b0100000000000035047fffffff0100', +} + MESSAGES = { 'BindingRevocationMessage': 'Binding_Revocation_Message', 'FastBindingAcknowledgmentMessage': 'Fast_Binding_Acknowledgment', @@ -169,7 +177,7 @@ def test_make_writes_the_reserved_bits(self) -> None: for site, (frame, width) in FRAMES.items(): with self.subTest(site=site): built = self._make(site, value=(1 << width) - 1) - self.assertEqual(built.hex(), frame) + self.assertEqual(built.hex(), MADE.get(site, frame)) model, attr = self._site(self._parse(built).info, site) # type: ignore[attr-defined] self.assertEqual(getattr(model, attr), (1 << width) - 1) diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 2b5fe3e85..eed184106 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -2052,8 +2052,9 @@ def test_mh_constructed_header_round_trips(self) -> None: brr = Packet.Binding_Refresh_Request cases = ( - # 2 octets of reserved + a 6-octet PadN = 8, so 6 + 8 = 14 needs 2 more - ('padn then two pad1', [(Option.PadN, {'length': 4})], 16), + # 2 octets of reserved + a 6-octet PadN = 8, so 6 + 8 = 14 needs 2 + # more, which are one PadN of Option Length 0 (#1793) + ('padn then a two-octet padn', [(Option.PadN, {'length': 4})], 16), # 2 + 8 = 10, and 6 + 10 = 16 is already aligned ('padn, already aligned', [(Option.PadN, {'length': 6})], 16), # 2 + 4 = 6, and 6 + 6 = 12 needs 4 more octets diff --git a/tests/protocols/internet/test_mpl_seed_rebuild_unit.py b/tests/protocols/internet/test_mpl_seed_rebuild_unit.py index a7d6c1c48..e9852942c 100644 --- a/tests/protocols/internet/test_mpl_seed_rebuild_unit.py +++ b/tests/protocols/internet/test_mpl_seed_rebuild_unit.py @@ -76,7 +76,9 @@ def test_address_seed_keyword_selects_source_address_kind(self) -> None: with self.subTest(proto=name): built = cls(options=[(Option.MPL_Option, {'seed': SRC, 'seq': 7, 'max': True})], next=TransType.IPv6_NoNxt, payload=b'') - self.assertEqual(built.data, HEADERS[0]) + # HEADERS[0] ends in two Pad1, which a parsed header keeps; two + # octets of fresh padding are one PadN of length 0 (#1793) + self.assertEqual(built.data, HEADERS[0][:-2] + b'\x01\x00') def test_seed_that_does_not_fit_its_kind_is_rejected(self) -> None: from pcapkit.const.ipv6.option import Option diff --git a/tests/protocols/internet/test_padn_two_octets_1793_unit.py b/tests/protocols/internet/test_padn_two_octets_1793_unit.py new file mode 100644 index 000000000..abfe35e99 --- /dev/null +++ b/tests/protocols/internet/test_padn_two_octets_1793_unit.py @@ -0,0 +1,121 @@ +# -*- coding: utf-8 -*- +"""Two octets of option padding are one ``PadN``, not two ``Pad1``. + +GitHub issue #1793: HOPOPT, IPv6-Opts and MH each filled a two-octet shortfall +with two ``Pad1`` options. :rfc:`8200#section-4.2`, and :rfc:`6275#section-6.2.2` +for mobility options, say a ``PadN`` should be used rather than multiple +``Pad1``, so two octets are now one ``PadN`` of length ``0``: the octets +``01 00``. One octet is still a ``Pad1``, and parsed padding is still rebuilt as +it was read -- only freshly built padding changed. + +Every case builds its own octets in memory and reads no capture. Classes are +imported inside each test, after :func:`~tests._support.reimport_once_per_class`. + +""" + +import importlib +import io +import unittest +import warnings + +from tests._support import reimport_once_per_class + +#: Protocol module, class name and option enum module of each padding builder. +PROTOCOLS = ( + ('pcapkit.protocols.internet.hopopt', 'HOPOPT', 'pcapkit.const.ipv6.option'), + ('pcapkit.protocols.internet.ipv6_opts', 'IPv6_Opts', 'pcapkit.const.ipv6.option'), + ('pcapkit.protocols.internet.mh', 'MH', 'pcapkit.const.mh.option'), +) + +#: Parsed headers whose padding is two ``Pad1`` options, which have to rebuild +#: unchanged: a Router Alert in an options header, and a Binding Refresh Request +#: carrying a 6-octet ``PadN``. +TWO_PAD1 = { + 'HOPOPT': '3b00' '05020000' '00' '00', + 'IPv6_Opts': '3b00' '05020000' '00' '00', + 'MH': '3b01000000000000' '010400000000' '00' '00', +} + + +class TestTwoOctetPadding(unittest.TestCase): + """Pin the wire form of two octets of HOPOPT, IPv6-Opts and MH padding.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def _protocols(self) -> 'list[tuple[type, type]]': + return [(getattr(importlib.import_module(mod), name), + importlib.import_module(enum).Option) + for mod, name, enum in PROTOCOLS] + + def test_two_octets_are_one_zero_length_padn(self) -> None: + for cls, option in self._protocols(): + proto = object.__new__(cls) + # the offset counts the fixed header too, so 6 and 14 are two short + for offset in (6, 14): + with self.subTest(protocol=cls.__name__, offset=offset): + with warnings.catch_warnings(): + warnings.simplefilter('error') + pads, pad_len = proto._make_pad_options(offset) + self.assertEqual(pad_len, 2) + self.assertEqual([pad.type for pad in pads], [option.PadN]) + self.assertEqual(pads[0].pack(), b'\x01\x00') + + def test_one_and_three_octets_keep_their_form(self) -> None: + for cls, option in self._protocols(): + proto = object.__new__(cls) + with self.subTest(protocol=cls.__name__, octets=1): + pads, pad_len = proto._make_pad_options(7) + self.assertEqual(pad_len, 1) + self.assertEqual([(pad.type, pad.pack()) for pad in pads], + [(option.Pad1, b'\x00')]) + with self.subTest(protocol=cls.__name__, octets=3): + pads, pad_len = proto._make_pad_options(5) + self.assertEqual(pad_len, 3) + self.assertEqual([(pad.type, pad.pack()) for pad in pads], + [(option.PadN, b'\x01\x01\x00')]) + + def test_constructed_header_ends_in_one_zero_length_padn(self) -> None: + from pcapkit.const.ipv6.option import Option as IPv6_Option + from pcapkit.const.mh.option import Option as MH_Option + from pcapkit.const.mh.packet import Packet + from pcapkit.const.reg.transtype import TransType + + protocols = {cls.__name__: cls for cls, _ in self._protocols()} + built = { + # 2 + a 4-octet Router Alert = 6, two short of 8 + 'HOPOPT': (protocols['HOPOPT'](next=TransType.IPv6_NoNxt, + options=[(IPv6_Option.Router_Alert, {'alert': 0})]), + '3b00' '05020000' '0100', + [(IPv6_Option.Router_Alert, 4), (IPv6_Option.PadN, 2)]), + 'IPv6_Opts': (protocols['IPv6_Opts'](next=TransType.IPv6_NoNxt, + options=[(IPv6_Option.Router_Alert, {'alert': 0})]), + '3b00' '05020000' '0100', + [(IPv6_Option.Router_Alert, 4), (IPv6_Option.PadN, 2)]), + # 6 + 2 octets of reserved + a 6-octet PadN = 14, two short of 16 + 'MH': (protocols['MH'](next=TransType.IPv6_NoNxt, type=Packet.Binding_Refresh_Request, + chksum=b'\x00\x00', data={'options': [(MH_Option.PadN, {'length': 4})]}), + '3b01000000000000' '010400000000' '0100', + [(MH_Option.PadN, 6), (MH_Option.PadN, 2)]), + } + for name, (proto, hexstr, expected) in built.items(): + with self.subTest(protocol=name): + raw = bytes(proto) + self.assertEqual(raw.hex(), hexstr) + parsed = type(proto)(io.BytesIO(raw), len(raw), extension=True) + self.assertEqual([(code, opt.length) + for code, opt in parsed.info.options.items(multi=True)], + expected) + + def test_parsed_two_pad1_rebuild_byte_for_byte(self) -> None: + for cls, option in self._protocols(): + octets = bytes.fromhex(TWO_PAD1[cls.__name__]) + with self.subTest(protocol=cls.__name__): + parsed = cls(io.BytesIO(octets), len(octets), extension=True) + pads = list(parsed.info.options.items(multi=True))[-2:] + self.assertEqual([code for code, _ in pads], [option.Pad1, option.Pad1]) + self.assertEqual(bytes(cls.from_data(parsed.info)).hex(), octets.hex()) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/test_protocol_make_unit.py b/tests/protocols/test_protocol_make_unit.py index 0997b1090..c60fb0fe3 100644 --- a/tests/protocols/test_protocol_make_unit.py +++ b/tests/protocols/test_protocol_make_unit.py @@ -51,12 +51,13 @@ def _attr(path: 'str') -> 'Any': 'TCP options': ('pcapkit.protocols.transport.tcp.TCP', {'options': [b'\x02\x04\x05\xb4']}, '0000000000000000000000006000ffff00000000020405b4'), + # two octets of padding follow the option, as one zero-length PadN (#1793) 'HOPOPT options': ('pcapkit.protocols.internet.hopopt.HOPOPT', {'extension': True, 'options': [b'\x05\x02\x00\x00']}, - '1100050200000000'), + '1100050200000100'), 'IPv6_Opts options': ('pcapkit.protocols.internet.ipv6_opts.IPv6_Opts', {'extension': True, 'options': [b'\x05\x02\x00\x00']}, - '1100050200000000'), + '1100050200000100'), 'MH options': ('pcapkit.protocols.internet.mh.MH', {'data': {'options': [b'\x01\x02\x00\x00']}}, '11010000000000000102000001020000'),