diff --git a/CHANGELOG.md b/CHANGELOG.md index e61bdbdfc..b7de9a4ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Added** -- `pcapkit.utilities.logging` as a real interface: `get_logger()` for per-module children, `configure()` to set level, handler, stream, format or propagation at runtime, `reset()` to return to library-neutral, and `ensure_output()`. Seventeen modules now log under their own `__name__`, so a consumer can silence `pcapkit.foundation.registry` while keeping `pcapkit.foundation.extraction` (#384). - **Added** -- `conflict` on the reassembly data models: absolute, inclusive ranges where two fragments claimed the same span with different bytes, which was previously lost silently on both the IP (#482) and TCP (#443, #478) paths. - **Added** -- an end-to-end test tier (#376), sample-capture generators so a fresh clone can rebuild every fixture (#340), a Dockerised engine benchmark covering every supported Python version (#410), and registry round-trip coverage that records the entries which cannot close the cycle rather than skipping them (#440, #504). +- **Added** -- coverage for the untested half of the #431 accommodation: a TCP or IPv4 option whose declared length asks for more data than the capture actually holds, pinning that it still parses, with the short read left-padded rather than rejected. The one test #431 left behind only covers an option area with no data behind it at all; a candidate fix for #554 turned the untested half into an unwrapped `FieldValueError` while the rest of the suite stayed green (#571, #572). - **Changed** -- `pcapkit` no longer configures logging at import. It installs a `NullHandler` and sets no level, so verbosity is inherited from the application instead of being seized by whichever library was imported second; the old stderr handler stays as the `PCAPKIT_DEVMODE` opt-in. Three consequences worth knowing: the previous behaviour is `configure(logging.INFO, stream=sys.stderr)`; 38 registry and extractor `info` calls became `debug`, so those messages are invisible even at `INFO`; and the handler is no longer `logger.handlers[0]`. `verbose=` output stays on stdout and is not logging (#384). - **Changed** -- each warning is reported once per channel, and `pcapkit` no longer inserts a `simplefilter('ignore', ...)` at the front of the process-global `warnings.filters` (#362--#364, #390). The application's own filter therefore wins now, which is the point of the change and also the sharp edge in it: under `-W error`, or pytest's `filterwarnings = error`, a pcapkit warning that used to be suppressed will raise. Suppress them deliberately with `warnings.filterwarnings('ignore', category=BaseWarning)`. `quiet=True` now means no record at any level and no longer sets `sys.tracebacklimit`, and the `pcapkit.utilities.warnings.DEVMODE` re-export is gone -- its canonical home is `pcapkit.utilities.logging`. - **Changed** -- `layer=` and `protocol=` are honoured rather than inert. Both were read under the wrong names, so every value a caller passed was dropped into `**kwargs` and discarded; the CLI's `-L` also now validates its argument instead of accepting anything. The packet context reaches the schema layer for the first time as well, so a field the wire elides can be resolved from its enclosing packet (#404). `follow_tcp_stream` dispatches on the engine type, where both branches of the old test were dead and the native adapter ran against every engine's frames (#402). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index acd05bd5a..fd0a9fb59 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -79,6 +79,13 @@ pull requests between #326 and #509. covering every supported Python version (#410), and registry round-trip coverage that records the entries which cannot close the cycle rather than skipping them (#440, #504). +* **Added** -- coverage for the untested half of the #431 accommodation: a TCP + or IPv4 option whose declared length asks for more data than the capture + actually holds, pinning that it still parses, with the short read left-padded + rather than rejected. The one test #431 left behind only covers an option + area with no data behind it at all; a candidate fix for #554 turned the + untested half into an unwrapped ``FieldValueError`` while the rest of the + suite stayed green (#571, #572). * **Changed** -- ``pcapkit`` no longer configures logging at import. It installs a ``NullHandler`` and sets no level, so verbosity is inherited from the application instead of being seized by whichever library was imported diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index bd380dd9a..499afcf6c 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -2002,6 +2002,69 @@ def test_an_option_area_longer_than_the_datagram_still_parses(self) -> None: [(OptionNumber.EOOL, 1)], ) + def test_a_truncated_option_still_parses_its_declared_length(self) -> None: + """A capture cut short mid-option is tolerated, not just mid-header. C.f. #431, #572. + + :meth:`test_an_option_area_longer_than_the_datagram_still_parses` above + pins the *empty*-tail half of the #431 accommodation: an option area + that runs out before it starts. Nothing pinned the other half -- an + option that *does* start, declares more data than the capture actually + holds, and runs out partway through, the shape of a datagram cut short + by the snapshot length rather than one with no options at all. A + candidate fix for #554 (PR #571) turns that into an unwrapped + ``FieldValueError`` while the rest of the suite stays green, because + nothing exercises it. See + ``TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length`` + for the same case on TCP, where the shortfall is simpler to reach. + + The header below sets ``ihl`` to 9 -- 16 declared octets of options -- + for an unassigned option (code 31) declaring ``length=12``, which asks + :class:`~pcapkit.protocols.schema.internet.ipv4.UnassignedOption`'s + ``data`` field (``BytesField(length=lambda pkt: pkt['length'] - 2)``, + 10 octets here) for more than the 6 octets actually behind it, and + :meth:`FieldBase.unpack ` + left-pads the short read with zero octets rather than raising -- the + same accommodation as the TCP case, reached the same way. + + ``ihl`` has to declare *more* than the 8 octets actually present, + though, which the TCP case does not need. Unlike TCP, + :meth:`~pcapkit.protocols.internet.ipv4.IPv4._read_ipv4_options` sums + each option's self-*declared* ``length`` -- not what it actually + consumed -- and raises ``IPv4: invalid format`` if that sum exceeds + the declared option area; declaring exactly 8 would make the single + option's own ``length=12`` trip that check before the accommodation + under test is ever reached. Declaring 16 leaves headroom, at the cost + of a second effect: once the option loop's 16-octet budget outlives + the 8 octets its one real option consumed, the loop reads one further, + fully exhausted phantom option, decodes it as end-of-option-list (the + same mechanism the test above pins), and the #431 machinery in + :meth:`~pcapkit.corekit.fields.collections.OptionField.unpack` + rewinds and hands the same 8 octets to the schema a second time as + padding. That is why ``bytes(proto.__header__)`` does not round-trip + to ``raw`` here and is not asserted -- immaterial to what this test + pins, which is solely the ``data`` field's short-read reconstruction. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + from tests._support import time_limit + + custom = OptionNumber.get(31) + trailing = bytes([0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]) + raw = (bytes.fromhex('4900001c00010000400600000a0000010a000002') + + bytes([custom, 12]) + trailing) + with time_limit(5): + proto = IPv4(raw, len(raw)) + + self.assertEqual(proto.info.hdr_len, 36) + self.assertEqual( + [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], + [(custom, 12), (OptionNumber.EOOL, 1)], + ) + unassigned = next(opt for code, opt in proto.info.options.items(multi=True) + if code == custom) + self.assertEqual(unassigned.data, b'\x00\x00\x00\x00' + trailing) + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 2ec3e109f..4971856d4 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1257,6 +1257,71 @@ def test_an_option_area_longer_than_the_segment_still_parses(self) -> None: if code == Option.Maximum_Segment_Size) self.assertEqual(mss.mss, 1460) + def test_a_truncated_option_still_parses_its_declared_length(self) -> None: + """A capture cut short mid-option is tolerated, not just mid-header. C.f. #431, #572. + + :meth:`test_an_option_area_longer_than_the_segment_still_parses` above + pins the *empty*-tail half of the #431 accommodation: an option area + that runs out before it starts, so the type byte decodes as 0 and the + loop reads end-of-option-list. Nothing pinned the other half -- an + option that *does* start, declares more data than the capture actually + holds, and runs out partway through, the shape of a segment cut short + by the snapshot length rather than one with no options at all. A + candidate fix for #554 (PR #571) turned that into an unwrapped + ``FieldValueError`` while the rest of the suite stayed green, because + nothing exercised it. + + The segment below sets a data offset of 7 -- 8 octets of option area + -- for an unassigned option kind (``0x4f``) declaring ``length=12``, + which asks + :class:`~pcapkit.protocols.schema.transport.tcp.UnassignedOption`'s + ``data`` field (``BytesField(length=lambda pkt: pkt['length'] - 2)``, + 10 octets here) for more than the 6 octets actually behind it. + :meth:`FieldBase.unpack ` + left-pads the short read with zero octets rather than raising, so the + option parses with its declared ``length`` intact and a ``data`` value + of four zero octets followed by the six real ones. That is reachable + here because :meth:`OptionField.unpack + ` sizes this + option's schema by what it actually consumed (8 octets) rather than by + its self-reported ``length``, so the separate ``TCP: invalid format`` + threshold in :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` + never sees the shortfall -- unlike IPv4's equivalent check, which sums + the *declared* lengths instead and does see it (see + ``IPv4UnitTests.test_a_truncated_option_still_parses_its_declared_length``). + ``length=32`` (30 octets of data wanted, still only 6 available) is + checked alongside 12, since the fix under discussion would reject both + identically. + + """ + import struct + + from pcapkit.const.tcp.option import Option + from pcapkit.protocols.transport.tcp import TCP + from tests._support import time_limit + + def segment(data_offset: 'int', options: 'bytes') -> 'bytes': + return struct.pack('!HHIIBBHHH', 1, 2, 0, 0, data_offset << 4, + 0x10, 0, 0, 0) + options + + custom = Option.get(0x4f) + trailing = bytes([0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]) + for declared_length, zeroes in ((12, 4), (32, 24)): + with self.subTest(declared_length=declared_length): + raw = segment(7, bytes([custom, declared_length]) + trailing) + with time_limit(5): + proto = TCP(raw, len(raw)) + + self.assertEqual(proto.info.hdr_len, 28) + self.assertEqual( + [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], + [(custom, declared_length)], + ) + unassigned = next(opt for code, opt in proto.info.options.items(multi=True) + if code == custom) + self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing) + self.assertEqual(bytes(proto.__header__), raw) + def test_unregistered_option_kind_does_not_mutate_the_class_registry(self) -> None: """Parsing must not write to the shared ``TCP.__option__``.