From ff0ea997be1eb6086ec05269dfbc346dbf25a02d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 02:24:45 -0400 Subject: [PATCH] fix(ipv4): correct the SEC option bitmap and narrow SID to 16 bits (#534, #537) * `_make_opt_sec` never set RFC 1108's field termination indicator, so every SEC option the library wrote with an authority was one its own reader warned about -- including in the project's own `options-ipv4.pcap`. Bit 0 of the final octet is now set. * `_make_opt_sec` sized the bitmap from the highest authority *index* rather than the bit count, so a lone `GENSER` (value 0) built a zero-octet bitmap and raised a bare `IndexError`, and every multiple of eight under-sized by an octet. It sizes from the count and rejects a non-authority index with `ProtocolError`. * `Field_Termination_Indicator` (index 7) was accepted as an authority, writing an option that read as terminated while carrying none. Index 7, 15 and 23 are termination bits by `_read_opt_sec`'s own numbering and are now rejected; the reader's `range(7)` is left alone, being the correct half. * `SIDOption.sid` was a `UInt32Field` where RFC 791 gives a 2-octet Stream ID, so a well-formed option over-read by two octets (`packet length < 0: -2`) and re-emitted six octets wide, dragging NOP/EOOL padding in behind it. Narrowed to `UInt16Field`; a parsed datagram now rebuilds byte-identically. * Adds a UDP-in-IPv4 payload test, closing the gap that let a fix special-casing `(Protocol, Raw, NoPayload)` pass all four of #536's tests. Full pytest suite green: 1181 passed, 17 skipped, 2661 subtests. --- examples/generators/options.py | 8 +- pcapkit/protocols/internet/ipv4.py | 45 +- pcapkit/protocols/schema/internet/ipv4.py | 11 +- tests/protocols/internet/test_ipv4_unit.py | 466 +++++++++++++++++- tests/protocols/test_option_roundtrip_unit.py | 128 +---- 5 files changed, 538 insertions(+), 120 deletions(-) diff --git a/examples/generators/options.py b/examples/generators/options.py index b26c25a09f..6b9555ad37 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -466,8 +466,12 @@ def _ipv4_overrides() -> 'dict[Any, dict[str, Any]]': from pcapkit.const.ipv4.option_number import OptionNumber from pcapkit.const.ipv4.protection_authority import ProtectionAuthority return { - # A single authority whose value is 0 makes ``_make_opt_sec`` compute a - # zero-octet bitmap and then index into it; two keeps it non-empty. + # Two authorities rather than one because two bits set in the bitmap say + # more than one does, not because one is unrepresentable: it used to be, + # a single ``GENSER`` (value 0) making ``_make_opt_sec`` size a + # zero-octet bitmap and then index into it for a bare ``IndexError``, and + # #537 fixed that arithmetic. So this is a coverage choice now and no + # longer routes around anything. OptionNumber.SEC: {'authorities': [ProtectionAuthority.GENSER, ProtectionAuthority.NSA]}, # ``counts=10``, the default, needs 43 option octets, which overflows diff --git a/pcapkit/protocols/internet/ipv4.py b/pcapkit/protocols/internet/ipv4.py index 042eb9cd0b..2048a82b4a 100644 --- a/pcapkit/protocols/internet/ipv4.py +++ b/pcapkit/protocols/internet/ipv4.py @@ -1392,6 +1392,22 @@ def _make_opt_sec(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_SECOpt Returns: Constructured option schema. + Raises: + ProtocolError: If ``authorities`` names a bit position that is not a + protection authority -- a negative one, or one that :rfc:`1108` + reserves as a field termination indicator. + + Notes: + :rfc:`1108` section 2.2 lays each protection authority octet out as + seven authority bits followed by a *field termination indicator* in + bit 0: ``0`` means another octet follows, ``1`` means this is the + last. So the authority numbering skips every position that is a + termination bit -- 7, 15, 23 -- which is what + :meth:`_read_opt_sec` encodes by looping over ``range(7)`` per + octet, and the reason ``Field_Termination_Indicator`` is rejected + here rather than written: the enumeration names it as structure, and + a value written there would be dropped on the way back in. See #537. + """ if option is not None: level_val = option.level @@ -1402,12 +1418,39 @@ def _make_opt_sec(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_SECOpt authorities = [] if authorities is None else authorities if authorities: + for auth in authorities: + if auth < 0: + raise ProtocolError(f'{self.alias}: [OptNo {kind}] invalid protection ' + f'authority: {auth}') + if auth % 8 == 7: + # ``.name`` where there is one, rather than + # ``Enum_ProtectionAuthority.get(auth)``: that call runs + # ``_missing_`` for an unnamed index, which extends the + # enumeration as a side effect. An error path is the last + # place that should mutate a registry. + raise ProtocolError(f'{self.alias}: [OptNo {kind}] invalid protection ' + f'authority: {getattr(auth, "name", auth)} is a field ' + f'termination indicator, not an authority') + + # ``max_auth`` is the highest bit *index*, so the octet count comes + # from the bit *count* one past it. Sizing from the index itself put + # a single ``GENSER`` (index 0) in a zero-octet bitmap and then + # indexed into it, raising a bare ``IndexError``; and it under-sized + # by an octet at every exact multiple of eight. See #537. max_auth = max(authorities) - int_len = math.ceil(max_auth / 8) + int_len = math.ceil((max_auth + 1) / 8) data_list = [b'0' for _ in range(int_len * 8)] for auth in authorities: data_list[auth] = b'1' + + # Bit 0 of the *last* octet terminates the field. The intermediate + # octets keep the ``0`` they were initialised with, which is what + # says "another octet follows" -- so this single assignment is the + # whole of the indicator, and without it every option this method + # wrote was one its own reader warned about. + data_list[-1] = b'1' + data = int(b''.join(data_list), base=2).to_bytes(int_len, 'big', signed=False) else: data = b'' diff --git a/pcapkit/protocols/schema/internet/ipv4.py b/pcapkit/protocols/schema/internet/ipv4.py index 3afcbed688..e075a834fa 100644 --- a/pcapkit/protocols/schema/internet/ipv4.py +++ b/pcapkit/protocols/schema/internet/ipv4.py @@ -364,8 +364,15 @@ def __init__(self, type: 'Enum_OptionNumber', length: 'int', pointer: 'int', rou class SIDOption(Option, code=Enum_OptionNumber.SID): """Header schema for IPv4 stream identifier (``SID``) option.""" - #: Stream identifier. - sid: 'int' = UInt32Field() + #: Stream identifier. Two octets, per :rfc:`791` section 3.1, which gives the + #: option as four octets in total: one of type, one of length, and a 16-bit + #: stream identifier. This was a :class:`~pcapkit.corekit.fields.numbers.UInt32Field`, + #: which over-read a well-formed option by two octets on the way in -- the + #: ``packet length < 0: -2`` the library warned about -- and re-emitted it two + #: octets too wide on the way out, against the ``length=4`` that + #: :meth:`~pcapkit.protocols.internet.ipv4.IPv4._make_opt_sid` had always + #: written. See #534. + sid: 'int' = UInt16Field() if TYPE_CHECKING: def __init__(self, type: 'Enum_OptionNumber', length: 'int', sid: 'int') -> 'None': ... diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 1cf8cff264..445c99dc52 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -301,6 +301,134 @@ def header(value: 'object') -> 'Schema_IPv4': with self.assertRaises(ProtocolUnbound): header(object()).pack() + def test_schema_pack_packs_a_second_real_protocol_as_payload(self) -> None: + """A real protocol nested in another one, not ``Raw`` and not ``NoPayload``. + + This closes a hole in the three tests above rather than describing a + defect of its own. Every one of them hands the payload field a + :class:`~pcapkit.protocols.misc.raw.Raw` or a + :class:`~pcapkit.protocols.misc.null.NoPayload`, because those are what + *parsing* yields, so between them they exercise exactly two of the 43 + :class:`~pcapkit.protocols.protocol.ProtocolBase` descendants the widened + check in :meth:`Schema.pack + ` has to accept. That is + enough coverage to be defeated by a fix that is not one: replacing the + ``isinstance(data, ProtocolBase)`` check of #536 with + + .. code-block:: python + + elif isinstance(data, (Protocol, Raw, NoPayload)): + + -- special-casing precisely the classes the tests use -- passes all four + of them, measured, while leaving the general defect in place for the + other 40 protocols. #536's own reviewer found that by falsification, and + this test is what makes the shortcut fail. + + So the payload here is a ``UDP``, and the assertions are chosen to be + unsatisfiable by any finite list of special cases: + + * ``UDP`` is a ``ProtocolBase`` and is *none* of the three classes such a + list would name, which is what makes it a witness rather than another + instance of the covered case; + * three different real protocols are nested, so a list extended to + include ``UDP`` alone still fails; + * the count of descendants outside that list is asserted, so the reason + a class list cannot be the fix is recorded as a number rather than as a + remark. It is a lower bound, since the check only has to keep holding + as protocols are added. + + UDP-in-IPv4 is also the plainest thing a packet library is for, which is + the other reason it belongs in the suite on its own merits. + + """ + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.misc.null import NoPayload + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.protocol import Protocol, ProtocolBase + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.protocols.transport.udp import UDP + + proto = object.__new__(IPv4) + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + udp = UDP(srcport=1234, dstport=53, payload=b'\xbb' * 8) + udp_octets = bytes(udp) + + # 8 octets of UDP header and the 8 the caller gave it + self.assertEqual(len(udp_octets), 16) + + # The witness: a real protocol is a ``ProtocolBase``, and it is not any + # of the classes a special-cased check would enumerate. Every assertion + # below rests on this one. + self.assertIsInstance(udp, ProtocolBase) + self.assertNotIsInstance(udp, Protocol) + self.assertNotIsInstance(udp, Raw) + self.assertNotIsInstance(udp, NoPayload) + + # Nesting it packs to exactly what handing over its own octets does, so + # the payload branch packed the protocol rather than rejecting it or + # stringifying it. + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + nested = proto.make(protocol=17, payload=udp).pack() + flat = proto.make(protocol=17, payload=udp_octets).pack() + self.assertEqual(nested, flat) + self.assertEqual(nested[20:], udp_octets) + self.assertEqual(len(nested), 36) + + # And the datagram parses back as UDP, which is the end-to-end statement + # that the nesting produced a real packet and not merely equal bytes. + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = IPv4(io.BytesIO(nested), len(nested)) + self.assertEqual(parsed.info.len, 36) + self.assertIsInstance(parsed.payload, ProtocolBase) + + # More than one, so extending the special-case list with ``UDP`` does not + # buy the shortcut anything either. + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + others = [ + TCP(srcport=1234, dstport=80, payload=b'\xcc' * 8), + IPv4(protocol=6, src='192.0.2.1', dst='198.51.100.1', + payload=b'\xcc' * 8), + ] + for inner in others: + with self.subTest(payload=type(inner).__name__): + self.assertIsInstance(inner, ProtocolBase) + self.assertNotIsInstance(inner, (Protocol, Raw, NoPayload)) + octets = bytes(inner) + self.assertEqual( + proto.make(protocol=6, payload=inner).pack()[20:], + octets + ) + + # The number that says why a class list is the wrong shape of fix. The + # walk is over descendants rather than direct subclasses because the + # tree is several levels deep -- ``UDP`` is a ``Transport`` is a + # ``ProtocolBase``. + def descendants(cls: 'type') -> 'set[type]': + found = set() # type: set[type] + pending = [cls] + while pending: + for sub in pending.pop().__subclasses__(): + if sub not in found: + found.add(sub) + pending.append(sub) + return found + + excluded = {Protocol, Raw, NoPayload} | descendants(Protocol) + uncovered = descendants(ProtocolBase) - excluded + self.assertGreaterEqual( + len(uncovered), 30, + 'the payload branch has to accept every ProtocolBase subclass, and ' + 'there are far more of them than the Raw and NoPayload the other ' + 'tests in this file use; if this number has collapsed, the walk ' + 'broke rather than the library shrinking' + ) + self.assertIn(UDP, uncovered) + def test_ipv4_make_options_pads_with_an_eool_option_not_its_wire_code(self) -> None: """Option padding has to be an option, not an option number. C.f. #506. @@ -365,6 +493,263 @@ def test_ipv4_make_options_pads_with_an_eool_option_not_its_wire_code(self) -> N self.assertEqual(options[-1].type, OptionNumber.EOOL) self.assertEqual(bytes(rebuilt), raw) + def test_ipv4_make_opt_sec_sets_the_field_termination_indicator(self) -> None: + """A SEC option this library writes is one this library can read. C.f. #537. + + :rfc:`1108` section 2.2 makes bit 0 of each protection authority octet a + *field termination indicator*: ``0`` means another octet follows, ``1`` + means this is the last. ``_read_opt_sec`` enforces it, warning + ``field termination indicator not set`` when the final octet has it + clear. ``_make_opt_sec`` built the bitmap purely out of authority bit + positions and never set it, so *every* SEC option the library wrote with + at least one authority was one its own reader flagged as malformed -- + visible in the project's own generated capture + :file:`examples/captures/options-ipv4.pcap`, which warned + ``IPv4: [OptNo 130] invalid format: field termination indicator not set`` + on extraction. + + The round trip is asserted through a real datagram and with warnings + promoted to errors, because the defect's only symptom was a warning: + asserting on the octets alone would have let it back in, and asserting + that the flags survive alone would too -- they always did. + + ``0x91`` rather than ``0x90`` is the whole change on the wire: + ``1001 0001``, bits 0 and 3 for ``GENSER`` and ``NSA`` counted from the + most significant, and bit 0 of the octet for the terminator. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.const.ipv4.protection_authority import ProtectionAuthority + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.utilities.warnings import ProtocolWarning + + proto = object.__new__(IPv4) + authorities = [ProtectionAuthority.GENSER, ProtectionAuthority.NSA] + + schema = proto._make_opt_sec(OptionNumber.SEC, authorities=authorities) + self.assertEqual(schema.data, b'\x91') + self.assertEqual(schema.data[-1] & 0x01, 1) + self.assertEqual(schema.length, 4) + + # The reader's own predicate, stated here rather than inferred from the + # absence of a warning below, so a reader that stopped checking does not + # silently make this test vacuous. + self.assertNotEqual(schema.data[-1] & 0x01, 0) + + # And the end-to-end statement: a datagram carrying this option parses + # with no ProtocolWarning at all. The option is four octets -- type, + # length, classification level and the one-octet bitmap -- which is + # exactly the option area an ihl of 6 declares, so nothing is padded. + packed = schema.pack() + self.assertEqual(len(packed), 4) + header = bytes.fromhex('46000018 00000000 00060000 ' + '7f000001 7f000002') + packed + self.assertEqual(len(header), 24) + + with warnings.catch_warnings(): + warnings.simplefilter('error', ProtocolWarning) + parsed = IPv4(header, len(header)) + + self.assertEqual(parsed.info.options[OptionNumber.SEC].flags, + tuple(authorities)) + + def test_ipv4_make_opt_sec_sizes_the_bitmap_from_the_bit_count(self) -> None: + """One authority numbered zero is one octet, not zero octets. C.f. #537. + + ``int_len`` was ``math.ceil(max_auth / 8)``, which sizes the bitmap from + the highest bit *index* rather than from the bit *count* one past it. + With ``GENSER`` (value ``0``) the only authority that is ``0`` octets, + and the ``data_list[auth] = b'1'`` below it then raised a bare + ``IndexError: list assignment index out of range`` -- not an in-library + exception, and out of a ``_make_opt_*`` helper. + + That is why :file:`examples/generators/options.py` passed *two* + authorities to this option; with the arithmetic fixed, one is a + legitimate argument again, which is the thing this test is really + asserting. + + The same off-by-one under-sized by a whole octet at every exact multiple + of eight, which is checked here too. No shipped enumeration member has + value ``8``, so that half was latent rather than reachable -- and it is + the half that would have corrupted an option instead of raising, so it is + worth pinning even though nothing reaches it today. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.const.ipv4.protection_authority import ProtectionAuthority + from pcapkit.protocols.internet.ipv4 import IPv4 + + proto = object.__new__(IPv4) + + # One octet: GENSER at bit 0 from the most significant, and the + # terminator. Formerly an IndexError. + schema = proto._make_opt_sec(OptionNumber.SEC, + authorities=[ProtectionAuthority.GENSER]) + self.assertEqual(schema.data, b'\x81') + self.assertEqual(schema.length, 4) + + # The largest authority that still fits one octet, bit 6, since bit 7 is + # the terminator. + schema = proto._make_opt_sec(OptionNumber.SEC, + authorities=[ProtectionAuthority(6)]) + self.assertEqual(schema.data, b'\x03') + self.assertEqual(schema.length, 4) + + # Index 8 is the first bit of the *second* octet, so it needs two -- + # ceil(8/8) said one. The first octet is all zeros, its own terminator + # included, which is what says "another octet follows". + schema = proto._make_opt_sec(OptionNumber.SEC, + authorities=[ProtectionAuthority(8)]) + self.assertEqual(schema.data, b'\x00\x81') + self.assertEqual(schema.length, 5) + + # Only the last octet terminates the field; an intermediate one that did + # would make the reader warn 'remaining data'. + self.assertEqual(schema.data[0] & 0x01, 0) + self.assertEqual(schema.data[-1] & 0x01, 1) + + # No authorities at all stays a bare 3-octet option with no bitmap, so + # there is no final octet to terminate. + schema = proto._make_opt_sec(OptionNumber.SEC, authorities=[]) + self.assertEqual(schema.data, b'') + self.assertEqual(schema.length, 3) + + def test_ipv4_make_opt_sec_rejects_a_termination_bit_as_an_authority(self) -> None: + """Bit positions the reader treats as structure are not authorities. C.f. #537. + + ``Enum_ProtectionAuthority`` member ``7`` is named + ``Field_Termination_Indicator`` -- it is not an authority at all, yet it + was a member of the enumeration the writer accepted as one. Passing it + produced ``data=b'\\x01'``: an option that reads as validly terminated + while encoding zero authorities. + + The writer and the reader disagreed about whether index 7 is data, and + this test records which of the two won. ``_read_opt_sec`` loops over + ``range(7)`` per octet and maps octet ``base`` bit ``bit`` to authority + ``base * 8 + bit``, so the authority numbering it produces *skips* 7, 15 + and 23 -- those positions are termination bits and nothing else. The + reader is right and the writer was wrong, so the rejection is on the + write side and ``range(7)`` is left alone; widening it would make the + reader report a terminator as an authority. + + Rejecting the whole congruence class rather than only the named ``7`` + follows from that: 15 and 23 are termination bits for exactly the same + reason and are just as undeliverable, they simply have no name in the + enumeration yet. Asserting 15 is what stops the fix being read as a + special case of one value. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.const.ipv4.protection_authority import ProtectionAuthority + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.utilities.exceptions import ProtocolError + + proto = object.__new__(IPv4) + + with self.assertRaises(ProtocolError) as caught: + proto._make_opt_sec( + OptionNumber.SEC, + authorities=[ProtectionAuthority.Field_Termination_Indicator], + ) + self.assertIn('Field_Termination_Indicator', str(caught.exception)) + self.assertIn('field termination indicator', str(caught.exception)) + + # An in-library exception, which a bare IndexError was not. Both halves + # matter: the type, and that it carries the option number the way every + # other rejection in this module does. + self.assertIsInstance(caught.exception, ProtocolError) + self.assertIn(f'[OptNo {OptionNumber.SEC}]', str(caught.exception)) + + # 15 and 23 are termination bits too, by the reader's own numbering. + for value in (15, 23): + with self.subTest(authority=value): + with self.assertRaises(ProtocolError): + proto._make_opt_sec(OptionNumber.SEC, + authorities=[ProtectionAuthority(value)]) + + # A negative index would have written to the terminator through Python's + # negative indexing rather than raising -- silent corruption, and the one + # way the IndexError could still have been reached after the arithmetic + # was fixed. + with self.assertRaises(ProtocolError): + proto._make_opt_sec(OptionNumber.SEC, authorities=[-1]) + + # The control: the neighbouring index is a real authority and still + # works, so this is a statement about position 7 and not about the + # rejection swallowing the whole argument. + self.assertEqual( + proto._make_opt_sec(OptionNumber.SEC, + authorities=[ProtectionAuthority.DOE]).data, + b'\x09', + ) + + def test_ipv4_sid_option_is_four_octets_wide_on_the_wire(self) -> None: + """RFC 791's four-octet Stream ID option survives the round trip. C.f. #534. + + ``SIDOption.sid`` was a + :class:`~pcapkit.corekit.fields.numbers.UInt32Field` where :rfc:`791` + section 3.1 gives the Stream ID two octets inside a four-octet option -- + which is what ``_make_opt_sid`` itself had always written into + ``length``. Only the schema field disagreed, so it over-read a + well-formed option by two octets on the way in, warning + ``packet length < 0: -2``, and over-wrote it by two on the way out: + ``880400000037`` for an option that is ``88040037``. + + Six not being a multiple of four, the option area then reached the + 32-bit padding branch and picked up a ``NOP`` and an ``EOOL``, so the + rebuilt datagram came back four octets longer than the one it was read + from, with ``ihl`` and total length grown to match -- 28 octets and + ``ihl=7`` against the 24 and ``ihl=6`` on the wire. + + This starts from wire octets rather than from a constructed option + because that is the only place the width shows: constructing and + reconstructing both go through the same ``_make_opt_sid``, so the two + halves agreed with each other while disagreeing with :rfc:`791`. That + symmetry is why the defect could not be recorded as an + ``EXPECTED_FAILURES`` entry, and it is why the assertion here is + byte-for-byte identity against the input rather than a comparison of two + outputs. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + + option = bytes.fromhex('88040037') + header = bytes.fromhex('46000018 00000000 00060000 ' + '7f000001 7f000002') + option + self.assertEqual(len(header), 24) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + parsed = IPv4(header, len(header)) + + # The over-read was the library naming its own defect. It is gone. + self.assertNotIn('packet length < 0: -2', + [str(entry.message) for entry in caught]) + + sid = parsed.info.options[OptionNumber.SID] + self.assertEqual(sid.sid, 0x37) + self.assertEqual(sid.length, 4) + + proto = object.__new__(IPv4) + self.assertEqual(proto._make_opt_sid(OptionNumber.SID, sid).pack(), + option) + + # Four octets is already 32-bit aligned, so the padding branch is not + # reached at all -- no NOP and no EOOL, where before there was one of + # each. + options, total_length = proto._make_ipv4_options(parsed.info.options) + self.assertEqual([type(entry).__name__ for entry in options], + ['SIDOption']) + self.assertEqual(total_length, 4) + + # And so the datagram rebuilds to exactly the octets it was read from. + rebuilt = bytes(IPv4.from_data(parsed.info)) + self.assertEqual(rebuilt, header) + self.assertEqual(len(rebuilt), 24) + self.assertEqual(rebuilt[0] & 0x0F, 6) + self.assertEqual(int.from_bytes(rebuilt[2:4], 'big'), 24) + def test_ipv4_properties_read_and_make_cover_packet_paths(self) -> None: from pcapkit.const.ipv4.option_number import OptionNumber from pcapkit.const.reg.transtype import TransType @@ -460,8 +845,12 @@ def test_ipv4_properties_read_and_make_cover_packet_paths(self) -> None: options=[(OptionNumber.SID, {'sid': 5})], payload=b'data', ) - self.assertEqual(made.length, 32) - self.assertEqual(made.vihl['ihl'], 7) + # 20 of header, the 4-octet SID option, and 4 of payload. This read 32 + # with ihl=7 while ``SIDOption.sid`` was 32 bits wide: the option packed + # to six octets, which is not 32-bit aligned, so the option area picked + # up a NOP and an EOOL and grew to eight. See #534. + self.assertEqual(made.length, 28) + self.assertEqual(made.vihl['ihl'], 6) self.assertEqual(made.ttl, 64) self.assertEqual(made.proto, TransType.TCP) @@ -687,9 +1076,15 @@ def test_ipv4_option_constructors_cover_common_and_error_branches(self) -> None: self.assertEqual(proto._make_opt_eool(OptionNumber.EOOL).to_dict()['length'], 1) self.assertEqual(proto._make_opt_nop(OptionNumber.NOP).to_dict()['length'], 1) + # ``GENSER`` alone, where this passed + # ``[GENSER, Field_Termination_Indicator]`` before #537: the expected + # octets are unchanged, because bit 0 of the last octet is now set by + # ``_make_opt_sec`` itself rather than by naming the terminator as though + # it were an authority -- which is what produced the 0x01 here, and what + # is now rejected. sec = proto._make_opt_sec( OptionNumber.SEC, - authorities=[ProtectionAuthority.GENSER, ProtectionAuthority.Field_Termination_Indicator], + authorities=[ProtectionAuthority.GENSER], ) self.assertEqual(sec.to_dict()['length'], 4) self.assertEqual(sec.to_dict()['data'], b'\x81') @@ -741,11 +1136,28 @@ def test_ipv4_option_constructors_cover_common_and_error_branches(self) -> None: (OptionNumber.SID, {'sid': 5}), (OptionNumber.NOP, {}), ]) - self.assertEqual(total_length, 12) - # The padding terminator is an EOOL *option*, not the bare wire code: - # these two assertions pinned the latter, which is what #506 fixed. - self.assertEqual([type(item).__name__ for item in options], ['bytes', 'SIDOption', 'NOPOption', 'EOOLOption']) - self.assertEqual(options[-1].type, OptionNumber.EOOL) + # Four octets of raw option and the four of SID, with the NOP dropped as + # padding. This read 12 before #534, when SID packed to six octets and so + # dragged a NOP and an EOOL in behind it; four is already 32-bit aligned, + # so the alignment branch is not reached here at all any more. + self.assertEqual(total_length, 8) + self.assertEqual([type(item).__name__ for item in options], + ['bytes', 'SIDOption']) + + # Which is why the alignment branch gets a case of its own rather than + # riding along on SID's old width. The padding terminator is an EOOL + # *option*, not the bare wire code, and these assertions pinned the + # latter -- that is what #506 fixed, and it would have been lost here + # when SID stopped being a trigger. A SEC option carrying a two-octet + # bitmap packs to five, so it still is one: three octets of padding, two + # NOPs and the EOOL. + padded, padded_length = proto._make_ipv4_options([ + (OptionNumber.SEC, {'authorities': [ProtectionAuthority(8)]}), + ]) + self.assertEqual(padded_length, 8) + self.assertEqual([type(item).__name__ for item in padded], + ['SECOption', 'NOPOption', 'NOPOption', 'EOOLOption']) + self.assertEqual(padded[-1].type, OptionNumber.EOOL) with self.assertRaises(ProtocolError): proto._make_opt_ts(OptionNumber.TS, timestamp=None) @@ -785,7 +1197,11 @@ def opt_type(code): type=opt_type(OptionNumber.SEC), length=4, level=ClassificationLevel.Unclassified, - flags=(ProtectionAuthority.GENSER, ProtectionAuthority.Field_Termination_Indicator), + # ``NSA`` where this was ``Field_Termination_Indicator``, which #537 + # now rejects. It is also the more faithful data model: the + # terminator is structure, and ``_read_opt_sec`` never puts it in + # ``flags`` in the first place. + flags=(ProtectionAuthority.GENSER, ProtectionAuthority.NSA), ) self.assertEqual(proto._make_opt_sec(OptionNumber.SEC, sec).level, ClassificationLevel.Unclassified) @@ -938,8 +1354,13 @@ def opt_type(code): proto._make_opt_nop(OptionNumber.NOP), proto._make_opt_sid(OptionNumber.SID, sid=9), ]) - self.assertEqual(schema_total, 8) - self.assertEqual(len(schema_options), 3) + # Both NOPs are dropped as padding, on the bytes branch and the schema + # branch respectively, which is what this case is here to cover. Only the + # SID option survives, and since #534 it is four octets and needs no + # padding of its own -- this was 8 across 3 entries while the option + # packed to six and pulled a NOP and an EOOL in after it. + self.assertEqual(schema_total, 4) + self.assertEqual(len(schema_options), 1) self.assertEqual(schema_options[0].sid, 9) option_map = OrderedMultiDict([ @@ -950,14 +1371,35 @@ def opt_type(code): )), (OptionNumber.SID, sid), (OptionNumber.LSR, lsr), + # A SEC option whose authorities reach into a second bitmap octet, so + # it packs to five and needs three octets of padding -- two NOPs and + # the EOOL. Which is what keeps the multi-NOP arm of the alignment + # branch exercised on this, the data-model path: SID used to reach it + # by being two octets over-wide (6 % 4 == 2, one NOP), and since #534 + # it is 4-aligned and reaches it not at all. + (OptionNumber.SEC, ipv4_data.SECOption( + code=OptionNumber.SEC, + type=opt_type(OptionNumber.SEC), + length=5, + level=ClassificationLevel.Unclassified, + flags=(ProtectionAuthority.GENSER, ProtectionAuthority(8)), + )), (OptionNumber.MTUP, mtup), ]) mapped_options, mapped_total = proto._make_ipv4_options(option_map) - self.assertEqual(mapped_total, 20) + # 4 of SID, 8 of LSR and its padding, 8 of SEC and its padding, 4 of + # MTUP, with the NOP dropped. The SID half of this was 20 before #534, + # the extra four being the two octets it was over-wide by plus the two of + # padding they then needed. + self.assertEqual(mapped_total, 24) self.assertEqual(mapped_options[0].sid, 123) # As above: the terminator is an EOOL option schema, so look for its type # rather than for the wire code itself. See #506. self.assertIn(OptionNumber.EOOL, [item.type for item in mapped_options]) + # Two NOPs from the SEC option's three octets of padding, which is the + # arm that ``for _ in range(pad_len - 1)`` only reaches when pad_len > 1. + self.assertEqual( + [item.type for item in mapped_options].count(OptionNumber.NOP), 2) self.assertEqual(mapped_options[-1].mtu, 1500) def test_ipv4_option_readers_cover_common_and_error_branches(self) -> None: diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 6d82f35ac6..b35ad7a041 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -30,11 +30,19 @@ wrong octets, which match each other and so match the assertion. Those are pinned as tests of their own rather than as entries, since an entry would have to record ``'OK'`` as a failure: -:meth:`OptionRoundTripTests.test_a_parsed_sid_option_re_emits_two_octets_too_wide` -for IPv4's ``SID`` option width, tracked as #534, and :meth:`OptionRoundTripTests.test_a_single_hip_parameter_cannot_be_constructed` for the HIP header arithmetic the generator's ``HIP_COPIES`` routes around. +IPv4's ``SID`` option width was the other one, tracked as #534 and pinned here by +a ``test_a_parsed_sid_option_re_emits_two_octets_too_wide`` that no longer exists: +``SIDOption.sid`` has been narrowed to a 16-bit field, so a four-octet option read +off the wire now re-emits as the same four octets. The assertion that says so is +:meth:`IPv4UnitTests.test_ipv4_sid_option_is_four_octets_wide_on_the_wire +`, +in the IPv4 unit suite beside the rest of that option's coverage rather than here +-- what kept it in this module was the ``EXPECTED_FAILURES`` entry it stood in +for, and with the defect fixed there is nothing for it to stand in for. + Why the table is asserted in both directions -------------------------------------------- @@ -65,7 +73,6 @@ import sys import types import unittest -import warnings from typing import TYPE_CHECKING, NamedTuple from tests._support import purge_modules, time_limit @@ -186,23 +193,23 @@ class Gap(NamedTuple): # and they could not have been routed around, their length being # ``3 + counts * 4`` and so never a multiple of four for any argument. # - # ``SID`` reached that same branch for a second reason of its own, and #506 - # fixes only the padding half of it. Its cycle now closes, because the - # generator constructs and reconstructs the *same* six octets either side of - # the trip -- but six is not what the wire holds. ``SIDOption.sid`` is a - # ``UInt32Field`` at pcapkit/protocols/schema/internet/ipv4.py:368 where RFC - # 791's Stream ID is 16 bits, so ``_make_opt_sid`` packs ``880400000037`` - # where the option is ``88040037``, and a genuine 4-octet option read off the - # wire does not survive being re-emitted. + # ``SID`` reached that same branch for a second reason of its own, which #506 + # did not address and #534 now has: ``SIDOption.sid`` was a ``UInt32Field`` + # where RFC 791 section 3.1 gives the Stream ID 16 bits, so the option + # re-emitted as ``880400000037`` where the wire holds ``88040037``. Six not + # being a multiple of four, it then reached the padding branch above and grew + # a ``NOP`` and an ``EOOL``. The field is now ``UInt16Field``, the option + # re-emits as the four octets it was read as, and the padding branch is not + # reached at all. # - # That asymmetry cannot be recorded as a ``Gap``, because the status such an - # entry would have to name is ``'OK'`` -- the one value + # Neither state was expressible here. The cycle closed either way, because + # the generator constructs and reconstructs through the same + # ``_make_opt_sid`` and so compared six octets against six -- and a ``Gap`` + # naming ``'OK'`` is the one entry # :meth:`test_round_trip_is_identity_or_a_recorded_gap` reads as "no entry - # needed". So it is pinned as an assertion instead, by - # :meth:`OptionRoundTripTests.test_a_parsed_sid_option_re_emits_two_octets_too_wide`, - # which is the record this table would otherwise have carried and which is - # what turns red when the field is narrowed. It is tracked as #534, so that - # dropping the entry from this table does not drop the defect with it. + # needed". Which is why the width is asserted against *wire* octets instead, + # now in :meth:`IPv4UnitTests.test_ipv4_sid_option_is_four_octets_wide_on_the_wire + # `. # ``_make_opt_ts`` passes ``data=`` where the schema field is ``ts_data``. # ``Schema.__init__`` only warns about an unknown field name and carries on, @@ -709,91 +716,6 @@ def test_a_single_hip_parameter_cannot_be_constructed(self) -> None: again = bytes(HIP(parameters=reparsed.info.parameters, extension=True, **base)) self.assertEqual(paired, again) - def test_a_parsed_sid_option_re_emits_two_octets_too_wide(self) -> None: - """RFC 791's four-octet Stream ID option comes back six octets wide. - - Tracked as #534. This is the record that ``ipv4-option/SID`` used to - carry in :data:`EXPECTED_FAILURES`, kept here because it can no longer be - carried there, and filed as an issue as well so that the defect is - tracked somewhere a passing test suite cannot hide it. - - #506 fixed the option-padding defect that made the ``SID`` case - fail to construct at all, and with that gone the generator's cycle - closes and the case reports ``'OK'`` -- so a ``Gap`` for it would have to - record ``'OK'`` as a failure status, which is the one value - :meth:`test_round_trip_is_identity_or_a_recorded_gap` reads as "this case - needs no entry". - - The cycle closes for a reason that is worth being precise about: the - generator constructs the option and reconstructs it through the *same* - ``_make_opt_sid``, so both halves emit the same six octets and match each - other. It is only against the wire that the width shows, which is why - this test starts from wire octets rather than from the generator's case. - - ``SIDOption.sid`` is a :class:`~pcapkit.corekit.fields.numbers.UInt32Field` - at ``pcapkit/protocols/schema/internet/ipv4.py:368``, where RFC 791 - section 3.1 gives the Stream ID two octets inside a four-octet option -- - which is also what ``_make_opt_sid`` itself writes into ``length``. So - the field over-reads a well-formed option by exactly two octets on the - way in, and over-writes it by two on the way out. - - Narrowing that field to - :class:`~pcapkit.corekit.fields.numbers.UInt16Field` makes every - assertion below wrong at once -- measured: the option re-emits as - ``88040037``, the datagram stays 24 octets, and the padding branch is not - reached at all. That is the intended fix, and deleting this test is how - it gets recorded, exactly as deleting an :data:`EXPECTED_FAILURES` entry - would have been. - - """ - from pcapkit.const.ipv4.option_number import OptionNumber - from pcapkit.protocols.internet.ipv4 import IPv4 - - # A minimal IPv4 header, ihl=6, carrying one well-formed SID option: - # kind 136, length 4, and the two-octet stream id 0x0037. - option = bytes.fromhex('88040037') - header = bytes.fromhex('46000018 00000000 00060000 ' - '7f000001 7f000002') + option - self.assertEqual(len(header), 24) - - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter('always') - parsed = IPv4(header, len(header)) - - # The over-read, named by the library itself: a four-octet option minus a - # six-octet schema is the -2 in this warning. - self.assertIn('packet length < 0: -2', - [str(entry.message) for entry in caught]) - - # The value still survives the trip in, and the option still declares the - # four octets it occupies -- so nothing here is a parsing failure. - sid = parsed.info.options[OptionNumber.SID] - self.assertEqual(sid.sid, 0x37) - self.assertEqual(sid.length, 4) - - # Out again, the same option is six octets: two of stream id have become - # four, and the declared length no longer describes it. - proto = object.__new__(IPv4) - self.assertEqual(proto._make_opt_sid(OptionNumber.SID, sid).pack(), - bytes.fromhex('880400000037')) - - # Six is not a multiple of four, so the option area now reaches the - # padding branch that #506 fixed. That branch is no longer fatal, which - # is what lets the defect below through instead of stopping at it. - options, total_length = proto._make_ipv4_options(parsed.info.options) - self.assertEqual([type(entry).__name__ for entry in options], - ['SIDOption', 'NOPOption', 'EOOLOption']) - self.assertEqual(total_length, 8) - - # And so the rebuilt datagram is four octets longer than the one it was - # read from, with ihl and total length grown to match. - rebuilt = bytes(IPv4.from_data(parsed.info)) - self.assertNotEqual(rebuilt, header) - self.assertEqual(rebuilt[20:], bytes.fromhex('8804000000370100')) - self.assertEqual(len(rebuilt), 28) - self.assertEqual(rebuilt[0] & 0x0F, 7) - self.assertEqual(int.from_bytes(rebuilt[2:4], 'big'), 28) - def test_recorded_gaps_are_a_minority(self) -> None: """Most of the option space round-trips, and the rest is accounted for.