diff --git a/CHANGELOG.md b/CHANGELOG.md index cabc3b53e..6d4f905b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +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). +- **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 zero-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). @@ -64,6 +64,9 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Fixed** -- `httpv1`'s `_RE_METHOD` was unanchored and `re.match` anchors only at the start, so it prefix-matched, and the request-line reader then passed the whole `para1` to `Method.get` rather than the captured `method` group. Together those meant `b'Get'` matched on the single character `G`, satisfied the guard that decides a start-line is a request, and handed the entire mixed-case token to a lookup that raised on it. Fixing either half alone still gives a wrong answer -- normalising the lookup would parse `b'Get'` as `GET` off a one-character match, and passing the group would parse it as a method named `G`. The pattern is now anchored at both ends and the captured group is what is looked up, so a token that is not a method is a malformed request line rather than a mis-parsed one. Method tokens are case-sensitive per [RFC 9110 Section 9.1](https://datatracker.ietf.org/doc/html/rfc9110#section-9.1), so no `re.I` was added: `GET` parses, `Get` and `get` are rejected (#583). - **Fixed** -- `_RE_STATUS` in the same reader carried the same unanchored prefix defect, found by auditing `_RE_METHOD`'s siblings, and it escaped as the wrong exception type. That pattern is only a guard -- the value is taken from `int(para2)` on the raw token -- so a prefix match let a malformed status past the guard and then out of `int()` uncaught, where `_read_http_header` documents `ProtocolError`. Measured: a status of `200x` raised `ValueError: invalid literal for int() with base 10: b'200x'`, and one of `2000` raised `ValueError: 2000 is not a valid StatusCode`; both are now `ProtocolError`. [RFC 9112 Section 4](https://datatracker.ietf.org/doc/html/rfc9112#section-4) gives `status-code = 3DIGIT`, exactly three, so the anchor is what the grammar already said -- the production lives in HTTP/1.1 because `status-code` is part of its `status-line`, while [RFC 9110 Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15) covers the code semantics and the IANA registry rather than the syntax. `_RE_VERSION` was audited at the same time and is safe as it stands, because both of its call sites read the captured group rather than the raw token (#583). - **Fixed** -- `get()`'s documented `default` was ignored on the integer path throughout the generated `pcapkit.const` tree, because `get` delegated the lookup to the enum call and `_missing_` has no access to the caller's `default` -- so `Hardware.get(99999, 0)` raised `ValueError: 99999 is not a valid Hardware` instead of returning the fallback it was handed. The integer path now consults `default` before letting the lookup error escape. `-1`, the placeholder the generated signature already carried, is what separates "no default was supplied" from "a default was supplied and should be used", so a caller that asked for no fallback still gets the error rather than a silent substitution. The sweep #584 asked for puts the scope at 110 of the 118 integer registries, not the three the issue named; the two carrying a bespoke integer fallback of their own, `pcapng` `OptionType` and `reg` `AppType`, are deliberately left alone, since neither drops a default by raising. Not reachable from wire data -- every value a wire field can carry already resolves -- so this is a contract fix rather than a parse fix. Applied to the nine vendor templates as well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). +- **Fixed** -- `FieldBase.unpack` padded a short read on the wrong side. When a field's buffer falls short of its declared length -- the deliberate accommodation that lets a snapshot-truncated capture parse (#431) -- the shortfall was zero-filled with `rjust()`, which places the zeros at the *front*. That asserts the octets never read were the leading ones, and a short read has lost the trailing ones: the buffer ran out. It is now `ljust()`, which is correct for **both** byte orders rather than only for big-endian fields, so the correction is not byte-order-conditional. The two directions fail differently and only one was loud. Measured: one octet of a four-octet little-endian `120` read as `2013265920`, inflated by `2 ** 24`; three octets of a four-octet big-endian `0x01020304` read as `0x10203`, scaled *down* by 256. The second is the dangerous one -- a value smaller than the truth passes a sanity check, where an inflated one overruns -- which is why the big-endian half went unnoticed. A full read is untouched at every width and order, since the padding is only ever consulted when the buffer falls short (#604). +- **Fixed** -- as a consequence of the above, the unhandled `MemoryError` at `pcapkit/protocols/protocol.py:1016` on a truncated PCAP-NG capture. `dhcp_little_endian.pcapng` cut to 161, 641 or 1389 octets left a one-octet read of a little-endian 32-bit block length, which `rjust()` turned into `0x78000000` or `0x84000000` -- 1.88 to 2.06 GiB -- and which was then passed straight to `self._file.read()` as an allocation size, from a 1772-octet file. Under a 1 GiB address-space cap all three raise `MemoryError` there; with more address space the allocation succeeds and the parse goes on to fail anyway, so the visible symptom depended on how much memory the process could get. With `ljust()` the same reads report 120 and 132, no large allocation is attempted, and all three end in the ordinary, already-handled parse failure instead (#604). +- **Added** -- `FieldBaseShortReadPaddingSideTests`, ten cases over 133 subtests covering both byte orders at 2, 4 and 8 octets and at every truncation point: the two figures #604 reports as literals, the value-preserving property stated over every width and shortfall rather than as a table, the two failure directions as inequalities, a signed field, a byte-string field, and an unpack-then-pack cycle. Eight of the ten fail on the unfixed tree. The other two must pass on both and are the guard rails -- a full read at every width and order, which may not move, and a read against an entirely empty buffer, which pads to all zeros either way and is the #431 behaviour the option and list loops depend on (#604). - **Added** -- `CITATION.cff`, citation metadata in Citation File Format 1.2.0, which GitHub renders as the repository's "Cite this repository" button and which citation managers and dependency inventories read directly. It is the machine-readable half of the attribution BSD-3-Clause already asks for, so credit carries into a paper or a bill of materials rather than depending on a reader opening `LICENSE`. Validated with `cffconvert --validate` and against the published 1.2.0 schema; `doi` and `orcid` are omitted rather than invented, since neither exists for this project today and both are checked formats, so a wrong value would still validate. The licence itself is deliberately unchanged -- still BSD-3-Clause, no `NOTICE` file, no change to its terms. Alongside it the copyright line moves from `2018-2023` to `2018-2026` -- `LICENSE` was its only occurrence in the tree, since `docs/source/conf.py` already derives its own from the current year -- and a stray `s` after the closing `DAMAGE.` of the licence text, present since the Mozilla-to-BSD relicence, is removed, so the wording now matches canonical BSD-3-Clause exactly (#615). - **Fixed** -- reading a big-endian classic PCAP byte-swapped every record header field, and then crashed. `Frame.unpack` seeded the file's declared byte order under the key `bytesorder` where the frame schema's `byteorder_callback` reads `byteorder`, so the lookup never found it and always fell back to `sys.byteorder` -- the reading host's order rather than the file's. On a little-endian host reading a little-endian capture that fallback gives the right answer by coincidence, and every capture in this repository was little-endian, so the wrong code path has always produced correct results. Against a big-endian capture, measured before the fix, frame 1 of `big_endian.pcap` read `ts_sec=3106905`, `ts_usec=1088553216` and `incl_len=1241513984` for a record whose real values are `1500000000`, `123456` and `74`, dating the frame to 1970-02-05 rather than to 2017-07-14. `incl_len` is the payload length, so that first record then consumed the whole file and the second was read with a negative payload length, raising `ValueError: read length must be non-negative or -1` out of the schema -- which is the reported crash, and it is the *second* symptom rather than the first. The sibling `Frame.pack` eleven lines earlier spelled the key correctly, which is what marks this as a slip rather than a second key, and the fallback is what made a misspelled key indistinguishable from an absent one; `byteorder_callback` now records that it is the definition of the key and why the fallback hides a typo (#605). - **Added** -- `examples/generators/endian.py`, and the byte-order tests that read what it writes. There was no big-endian `.pcap` in the repository at all, which is why #605 survived its own code review: the one-character fix leaves the corrected path exactly as untested as the broken one. The generator writes three captures -- `big_endian.pcap` (magic `a1 b2 c3 d4`), `big_endian_nanosecond.pcap` (`a1 b2 3c 4d`, the first fixture to take that branch of the magic-number table) and `little_endian.pcap` (`d4 c3 b2 a1`) -- carrying the *same three records* in each container, so the tests can assert that the byte order makes no difference to what is read out rather than only that the big-endian file matches numbers written down in a test. Frame 3 is captured short, 1200 octets on the wire cut to a 96-octet `snaplen`, so `incl_len` and `orig_len` differ and cannot both be satisfied by one byte-swapped value. `test_frame_endian_runtime.py` drives all three through `extract()` and walks each file's record chain with `struct` to derive its own expectations; a unit-tier case in `test_header_frame_unit.py` builds a two-record big-endian capture in memory instead, so the regression is also caught by the fixture-free selection CI runs on every push. All four fail on the unfixed tree -- the three fixture-backed ones by that `ValueError`, the in-memory one by `AssertionError: 3106905 != 1500000000` -- while the little-endian twin passes on both trees, which is what shows the records themselves are not the variable (#605). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index d2edfa714..a89c5852e 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -81,7 +81,7 @@ pull requests between #326 and #509. 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 + actually holds, pinning that it still parses, with the short read zero-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 @@ -814,6 +814,44 @@ pull requests between #326 and #509. well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). +* **Fixed** -- ``FieldBase.unpack`` padded a short read on the wrong side. When a + field's buffer falls short of its declared length -- the deliberate + accommodation that lets a snapshot-truncated capture parse (#431) -- the + shortfall was zero-filled with ``rjust()``, which places the zeros at the + *front*. That asserts the octets never read were the leading ones, and a short + read has lost the trailing ones: the buffer ran out. It is now ``ljust()``, + which is correct for **both** byte orders rather than only for big-endian + fields, so the correction is not byte-order-conditional. The two directions + fail differently and only one was loud. Measured: one octet of a four-octet + little-endian ``120`` read as ``2013265920``, inflated by ``2 ** 24``; three + octets of a four-octet big-endian ``0x01020304`` read as ``0x10203``, scaled + *down* by 256. The second is the dangerous one -- a value smaller than the + truth passes a sanity check, where an inflated one overruns -- which is why the + big-endian half went unnoticed. A full read is untouched at every width and + order, since the padding is only ever consulted when the buffer falls short + (#604). +* **Fixed** -- as a consequence of the above, the unhandled ``MemoryError`` at + ``pcapkit/protocols/protocol.py:1016`` on a truncated PCAP-NG capture. + ``dhcp_little_endian.pcapng`` cut to 161, 641 or 1389 octets left a one-octet + read of a little-endian 32-bit block length, which ``rjust()`` turned into + ``0x78000000`` or ``0x84000000`` -- 1.88 to 2.06 GiB -- and which was then + passed straight to ``self._file.read()`` as an allocation size, from a + 1772-octet file. Under a 1 GiB address-space cap all three raise + ``MemoryError`` there; with more address space the allocation succeeds and the + parse goes on to fail anyway, so the visible symptom depended on how much + memory the process could get. With ``ljust()`` the same reads report 120 and + 132, no large allocation is attempted, and all three end in the ordinary, + already-handled parse failure instead (#604). +* **Added** -- ``FieldBaseShortReadPaddingSideTests``, ten cases over 133 + subtests covering both byte orders at 2, 4 and 8 octets and at every + truncation point: the two figures #604 reports as literals, the + value-preserving property stated over every width and shortfall rather than as + a table, the two failure directions as inequalities, a signed field, a + byte-string field, and an unpack-then-pack cycle. Eight of the ten fail on the + unfixed tree. The other two must pass on both and are the guard rails -- a full + read at every width and order, which may not move, and a read against an + entirely empty buffer, which pads to all zeros either way and is the #431 + behaviour the option and list loops depend on (#604). * **Added** -- ``CITATION.cff``, citation metadata in Citation File Format 1.2.0, which GitHub renders as the repository's "Cite this repository" button and which citation managers and dependency inventories read directly. It is diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 8935346f0..81ee440b2 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -424,8 +424,8 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': # ``secrets_data: BytesField(length=lambda pkt: pkt['__length__'])``) -- # and is thus attacker-controlled: a corrupt or hostile capture can # declare an arbitrarily large one. Past :data:`_MAX_ZERO_PAD_LENGTH`, a - # length short of what ``buffer`` holds is provably bogus: ``rjust()`` - # cannot recover data that was never in the buffer, only zero-pad for + # length short of what ``buffer`` holds is provably bogus: the padding + # cannot recover data that was never in the buffer, only zero-fill for # it, and no field this large is legitimate to begin with. Honouring it # would allocate and zero-fill up to ``length`` octets on nothing but # the packet's own say-so. C.f. #554. @@ -503,7 +503,28 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': ) ledger[1] += padding - value = struct.unpack(self.template, buffer[:length].rjust(length, b'\x00'))[0] + # NOTE: ``ljust()``, not ``rjust()``. A short read has lost the *trailing* + # octets of the field -- the buffer ran out, so what is missing is + # whatever came after what was read -- so the zeros belong at the end, + # where the unread octets were. ``rjust()`` instead put them at the + # front, which asserts that the octets never read were the *leading* + # ones, and that is wrong for every byte order rather than only for + # little-endian ones. Measured on the tree that padded with ``rjust()``: + # one octet of a four-octet little-endian 120 (``0x78``) read as + # 2,013,265,920, and three octets of a four-octet big-endian + # ``0x01020304`` read as ``0x10203``. ``ljust()`` answers 120 and + # ``0x1020300``. The big-endian error is the more dangerous of the two -- + # it scales the value *down*, so it passes a sanity check far more easily + # than the inflated little-endian one, which is why the defect went + # unnoticed there. See #604. + # + # This changes what a truncated field *reports*, which is the point, and + # not *whether* a truncated capture parses, which must not change: the + # accommodation itself is deliberate (#431) and the budget above is built + # around preserving it. An entirely empty buffer pads to all zeros either + # way, so the end-of-option-list and ``Pad1`` reads the option and list + # loops depend on are unaffected. + value = struct.unpack(self.template, buffer[:length].ljust(length, b'\x00'))[0] return self.post_process(value, packet) diff --git a/tests/corekit/test_fields_field.py b/tests/corekit/test_fields_field.py index d36207923..dde1b6975 100644 --- a/tests/corekit/test_fields_field.py +++ b/tests/corekit/test_fields_field.py @@ -65,15 +65,21 @@ def test_small_field_short_by_one_octet_still_zero_pads(self) -> None: This is the #431 mechanism the option and list loops depend on: a type or progress-check field reading past a truncated area gets a - short buffer and must still decode -- as zero, on the missing - high-order octets -- rather than raise, or every capture cut short + short buffer and must still decode -- as zero, on the octets that + were never read -- rather than raise, or every capture cut short by its snapshot length would start failing to parse instead of reporting the tail as padding. This field is nowhere near :data:`~pcapkit.corekit.fields.field._MAX_ZERO_PAD_LENGTH`, so the shortfall must still be padded exactly as before #554. + + The zeros land on the *tail*, which is where the unread octets were: + a short read has lost the end of the buffer, not the start of it. + Before #604 they were placed at the front instead, which corrupted + the value rather than merely padding it -- see + :class:`FieldBaseShortReadPaddingSideTests` below. """ field = self.BytesField(length=4) - self.assertEqual(field.unpack(b'\x01\x02\x03', {}), b'\x00\x01\x02\x03') + self.assertEqual(field.unpack(b'\x01\x02\x03', {}), b'\x01\x02\x03\x00') def test_small_field_over_an_entirely_empty_buffer_still_zero_pads(self) -> None: """The extreme case: nothing at all left to read. @@ -97,11 +103,12 @@ def test_field_at_the_ceiling_still_zero_pads(self) -> None: field = self.BytesField(length=self.ceiling) result = field.unpack(b'A', {}) - # rjust() right-justifies: the one real octet ends up at the end, with - # the padding -- not the data -- at the front. + # ljust() left-justifies: the one real octet stays where it was read, + # at the front, and the padding fills the tail that was never read. + # Before #604 this was the other way round. self.assertEqual(len(result), self.ceiling) - self.assertEqual(result[-1:], b'A') - self.assertEqual(result[:-1], b'\x00' * (self.ceiling - 1)) + self.assertEqual(result[:1], b'A') + self.assertEqual(result[1:], b'\x00' * (self.ceiling - 1)) def test_field_one_past_the_ceiling_with_insufficient_buffer_is_rejected(self) -> None: """Boundary: one octet past the ceiling, with data missing, is rejected. @@ -500,7 +507,7 @@ def test_the_budget_refusal_does_not_wedge_the_rest_of_the_parse(self) -> None: with time_limit(5): self.assertEqual(self.BytesField(length=1).unpack(b'', {}), b'\x00') self.assertEqual(self.BytesField(length=4).unpack(b'\x01\x02\x03', {}), - b'\x00\x01\x02\x03') + b'\x01\x02\x03\x00') def test_the_refusal_message_names_the_padding_the_ledger_and_the_allowance(self) -> None: declared = self.ceiling - 2 @@ -533,3 +540,318 @@ def test_a_field_that_is_not_short_is_never_refused_by_the_budget(self) -> None: self.assertEqual(self.BytesField(length=size).unpack(buffer, {}), buffer) self.assertEqual(ledger[1], 0) self.assertEqual(ledger[0], 500 * size) + + +class FieldBaseShortReadPaddingSideTests(unittest.TestCase): + """Which *side* the short-read zero padding goes on, for both byte orders. + + The two classes above are about *how much* padding + :meth:`FieldBase.unpack ` will + synthesise (#554, #573). This one is about where it lands, which is a + different defect with a different symptom: it corrupts values rather than + exhausting memory, and it does so silently -- no exception, no warning, just + a plausible integer that is wrong. + + Before #604 the padding was applied with ``rjust()``, which places the zeros + at the *front* of the buffer. That asserts that the octets which were never + read were the *leading* ones. A short read asserts the opposite: the buffer + ran out, so what is missing is whatever came *after* what was read. The zeros + therefore belong at the end, which is ``ljust()``. + + The correction is **not** byte-order-conditional, and that is the point worth + pinning. ``rjust()`` is wrong for a big-endian field exactly as it is for a + little-endian one; the two merely fail in opposite directions: + + * little-endian, one octet of a four-octet ``120`` (``0x78``) -- + ``rjust()`` gives ``00 00 00 78`` read little-endian, i.e. 2,013,265,920, + inflating the value by ``2 ** 24``. ``ljust()`` gives ``78 00 00 00``, + i.e. 120. + * big-endian, three octets of a four-octet ``0x01020304`` -- + ``rjust()`` gives ``00 01 02 03``, i.e. ``0x10203``, scaling the value + *down* by 256. ``ljust()`` gives ``01 02 03 00``, i.e. ``0x1020300``. + + The big-endian direction is the dangerous one. An inflated length is loud: it + overruns, or asks for an allocation nothing will grant. A length scaled + *down* by a factor of 256 is a smaller, entirely plausible number that passes + a sanity check and truncates real data instead. That asymmetry is why the + little-endian symptom is the one that got reported and the big-endian one sat + unnoticed -- and why a big-endian field must be tested with a value whose + high octets are *not* zero. Given a small big-endian value such as 120, whose + wire form is ``00 00 00 78``, ``rjust()`` restores exactly the leading zeros + that were lost and answers correctly by accident. + + Every case here is also checked at more than one width, because a single + width cannot distinguish "pads on the correct side" from "happens to agree + for four octets", and the widths are what a regression would most plausibly + get selectively wrong. + + What this must **not** change is *whether* a truncated capture parses. The + short-read accommodation is deliberate (#431), the budget above is built + around preserving it, and third-party PR #571 was declined for breaking it. + So the cases that the option and list loops depend on -- above all a read + against an entirely empty buffer, which pads to all zeros either way -- are + asserted here too, unchanged. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + from pcapkit.corekit.fields.numbers import (Int32Field, UInt16Field, UInt32Field, + UInt64Field) + from pcapkit.corekit.fields.strings import BytesField + + self.BytesField = BytesField + self.Int32Field = Int32Field + self.UInt16Field = UInt16Field + self.UInt32Field = UInt32Field + self.UInt64Field = UInt64Field + + def truncate(self, value: int, width: int, byteorder: str, kept: int) -> bytes: + """The first ``kept`` octets of ``value`` as it appears on the wire. + + Args: + value: The value the capture actually held. + width: Field width, in octets. + byteorder: ``'big'`` or ``'little'``. + kept: How many octets of the field were captured. + + Returns: + The octets a short read would be handed. + + Truncation cuts the *end* of a buffer, so this is a prefix of the wire + form -- which is what makes the missing octets the trailing ones whatever + the byte order is. + + """ + return value.to_bytes(width, byteorder)[:kept] # type: ignore[arg-type] + + def expected(self, value: int, width: int, byteorder: str, kept: int) -> int: + """What a short read of ``kept`` octets should report. + + The unread octets are unknown and assumed zero, so the answer is the + truncated wire form zero-filled back to ``width`` and decoded. Derived + from the wire form rather than written out, so it states the property + under test instead of restating the implementation's arithmetic. + """ + wire = self.truncate(value, width, byteorder, kept).ljust(width, b'\x00') + return int.from_bytes(wire, byteorder) # type: ignore[arg-type] + + def test_the_issue_s_little_endian_figure(self) -> None: + """Exactly the case #604 reports, pinned as a literal. + + One octet of a four-octet little-endian 120. ``rjust()`` answered + 2,013,265,920 -- wrong by seven orders of magnitude, with no exception + and no warning. The literals are written out rather than computed so that + this test states the issue's own measured figures and cannot drift with a + helper. + """ + field = self.UInt32Field(byteorder='little') + + self.assertEqual(field.unpack(b'\x78', {}), 120) + self.assertNotEqual(field.unpack(b'\x78', {}), 2013265920) + + def test_the_issue_s_big_endian_figure(self) -> None: + """The other half of #604, which the issue title understated. + + Three octets of a four-octet big-endian ``0x01020304``. ``rjust()`` + answered ``0x10203``, scaling the value down by 256 -- the direction that + passes a sanity check, which is why it went unnoticed. ``ljust()`` + answers ``0x1020300``: the three octets that were read, in place, with + the one that was not assumed zero. + """ + field = self.UInt32Field(byteorder='big') + + self.assertEqual(field.unpack(b'\x01\x02\x03', {}), 0x1020300) + self.assertNotEqual(field.unpack(b'\x01\x02\x03', {}), 0x10203) + + def test_a_short_read_is_value_preserving_at_every_width_and_order(self) -> None: + """The property, rather than a table of expected numbers. + + For every width, both byte orders, and every possible shortfall, a short + read must report the octets it was given *in the positions they occupy on + the wire*, with the octets it was not given assumed zero. That is one + statement covering 2-, 4- and 8-octet fields at every truncation point, + and it is the statement ``rjust()`` violates. + + The value is chosen so that no octet is zero and every octet differs, so + a padding side that is wrong -- or a width read with the wrong endianness + -- cannot coincide with the right answer. + """ + cases = [ + (2, self.UInt16Field, 0x0102), + (4, self.UInt32Field, 0x01020304), + (8, self.UInt64Field, 0x0102030405060708), + ] + + for width, cls, value in cases: + for byteorder in ('big', 'little'): + field = cls(byteorder=byteorder) # type: ignore[call-arg] + for kept in range(width + 1): + buffer = self.truncate(value, width, byteorder, kept) + with self.subTest(width=width, byteorder=byteorder, kept=kept): + self.assertEqual( + field.unpack(buffer, {}), + self.expected(value, width, byteorder, kept), + ) + + def test_a_full_read_is_untouched_at_every_width_and_order(self) -> None: + """The regression guard: nothing about a complete field may move. + + The padding side is only ever consulted when the buffer falls short, so a + field that got all its octets must answer exactly as it did before #604 -- + for a big-endian field especially, since that is the one whose short-read + behaviour this change alters and whose full-read behaviour must not. + """ + cases = [ + (2, self.UInt16Field, 0x0102), + (4, self.UInt32Field, 0x01020304), + (8, self.UInt64Field, 0x0102030405060708), + ] + + for width, cls, value in cases: + for byteorder in ('big', 'little'): + field = cls(byteorder=byteorder) # type: ignore[call-arg] + wire = value.to_bytes(width, byteorder) # type: ignore[arg-type] + with self.subTest(width=width, byteorder=byteorder): + self.assertEqual(field.unpack(wire, {}), value) + # and trailing octets beyond the field are not this field's + self.assertEqual(field.unpack(wire + b'\xff\xff', {}), value) + + def test_a_little_endian_short_read_is_never_inflated(self) -> None: + """The little-endian failure direction, stated as an inequality. + + ``rjust()`` on a little-endian field moves every octet read into a + *higher* position than it occupies on the wire, so the reported value + exceeds the truth -- by ``2 ** 24`` in the reported case. Whatever the + width or shortfall, a short read can only ever drop information, so it + must never report more than the value actually held. + """ + for width, cls, value in ((2, self.UInt16Field, 0x0102), + (4, self.UInt32Field, 0x01020304), + (8, self.UInt64Field, 0x0102030405060708)): + field = cls(byteorder='little') # type: ignore[call-arg] + for kept in range(width): + buffer = self.truncate(value, width, 'little', kept) + with self.subTest(width=width, kept=kept): + self.assertLessEqual(field.unpack(buffer, {}), value) + + def test_a_big_endian_short_read_keeps_the_octets_it_read_in_place(self) -> None: + """The big-endian failure direction: the octets must not slide down. + + ``rjust()`` shifted every octet read towards the low end, dividing the + value by 256 per missing octet -- so ``0x01020304`` truncated to three + octets read as ``0x10203`` rather than ``0x1020300``. Asserting the + leading octets survive at full magnitude is what catches that: the value + read must still be at least the truth with the unread octets zeroed, + which for a big-endian field means the most significant ones are intact. + """ + for width, cls, value in ((2, self.UInt16Field, 0x0102), + (4, self.UInt32Field, 0x01020304), + (8, self.UInt64Field, 0x0102030405060708)): + field = cls(byteorder='big') # type: ignore[call-arg] + for kept in range(1, width): + buffer = self.truncate(value, width, 'big', kept) + read = field.unpack(buffer, {}) + with self.subTest(width=width, kept=kept): + # the octets read are the high ones and keep their magnitude + self.assertEqual(read >> (8 * (width - kept)), + value >> (8 * (width - kept))) + # which the rjust() answer, scaled down by the shortfall, + # was not + self.assertNotEqual(read, value >> (8 * (width - kept))) + + def test_a_signed_field_pads_on_the_same_side(self) -> None: + """Signedness is orthogonal to the padding side, and must stay so. + + The sign lives in the most significant octet, so on a big-endian field it + is read first and survives a short read; on a little-endian one it is the + last octet and is exactly what a truncated capture loses. Under + ``rjust()`` a truncated little-endian negative value silently became a + small positive one, because the octet carrying the sign was synthesised + as zero *and* the octets that were read were moved into its place. + """ + value = -2 # 0xfffffffe + for byteorder in ('big', 'little'): + field = self.Int32Field(byteorder=byteorder) # type: ignore[call-arg] + wire = value.to_bytes(4, byteorder, signed=True) # type: ignore[arg-type] + + with self.subTest(byteorder=byteorder, kept=4): + self.assertEqual(field.unpack(wire, {}), value) + + for kept in range(1, 4): + expected = int.from_bytes( + wire[:kept].ljust(4, b'\x00'), byteorder, signed=True # type: ignore[arg-type] + ) + with self.subTest(byteorder=byteorder, kept=kept): + self.assertEqual(field.unpack(wire[:kept], {}), expected) + + def test_a_byte_string_field_keeps_its_octets_at_the_front(self) -> None: + """A truncated blob loses its tail, so the zeros go on the tail. + + :class:`~pcapkit.corekit.fields.strings.BytesField` unpacks through the + same line with an ``Ns`` template, so the padding side decides where the + real octets sit in the value handed to the caller. ``rjust()`` returned + ``b'\\x00\\x01\\x02\\x03'`` for three octets of a four-octet field, + which claims a leading zero octet that was never on the wire and hides + the fact that the tail is what is missing. + """ + for width in (2, 4, 8): + data = bytes(range(1, width + 1)) + for kept in range(width + 1): + field = self.BytesField(length=width) + with self.subTest(width=width, kept=kept): + self.assertEqual(field.unpack(data[:kept], {}), + data[:kept] + b'\x00' * (width - kept)) + + def test_an_entirely_empty_buffer_still_reads_as_zero(self) -> None: + """The #431 invariant, which this change must not disturb. + + ``OptionField.unpack`` and ``ListField.unpack`` read a fixed-width, + few-octet field past the end of a truncated option area and need it to + decode as ``0`` -- that is how an over-long ``ihl`` or a capture cut + short by its snapshot length reads as end-of-option-list or ``Pad1`` + rather than raising. With nothing at all in the buffer the whole field is + padding, so both ``rjust()`` and ``ljust()`` produce all zeros and the + answer is the same before and after #604. Asserted explicitly, because it + is the property a fix to the padding side could most easily have broken + and the one that declined #571. + """ + for byteorder in ('big', 'little'): + for cls in (self.UInt16Field, self.UInt32Field, self.UInt64Field): + field = cls(byteorder=byteorder) # type: ignore[call-arg] + with self.subTest(byteorder=byteorder, field=cls.__name__): + self.assertEqual(field.unpack(b'', {}), 0) + + for width in (1, 4, 16): + with self.subTest(bytes_width=width): + self.assertEqual(self.BytesField(length=width).unpack(b'', {}), + b'\x00' * width) + + def test_a_short_read_round_trips_back_to_the_padded_wire_form(self) -> None: + """``pack`` of what ``unpack`` reported must reproduce the padded buffer. + + :meth:`FieldBase.pack ` + writes the field's full width, so the only wire form a short read's value + can legitimately correspond to is the octets that were read followed by + the zeros that were synthesised for the ones that were not. Under + ``rjust()`` it did not: the value packed back to the *padding* in front of + the data, so an unpack/pack cycle moved the real octets. This is the + cheapest statement that the reported value and the buffer agree about + which octets were missing. + """ + cases = [ + (2, self.UInt16Field, 0x0102), + (4, self.UInt32Field, 0x01020304), + (8, self.UInt64Field, 0x0102030405060708), + ] + + for width, cls, value in cases: + for byteorder in ('big', 'little'): + field = cls(byteorder=byteorder) # type: ignore[call-arg] + for kept in range(width + 1): + buffer = self.truncate(value, width, byteorder, kept) + read = field.unpack(buffer, {}) + with self.subTest(width=width, byteorder=byteorder, kept=kept): + self.assertEqual(field.pack(read, {}), + buffer.ljust(width, b'\x00')) diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index f1b6753fe..cdfbd38c8 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -2023,8 +2023,11 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: ``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. + pads the short read with zero octets rather than raising -- the + same accommodation as the TCP case, reached the same way. The zeros go + on the *tail*, where the octets that were never read would have been; + before #604 they were placed at the front, which for a numeric field + corrupted the value outright. ``ihl`` has to declare *more* than the 8 octets actually present, though, which the TCP case does not need. Unlike TCP, @@ -2033,7 +2036,7 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: consumed -- and raises ``IPv4: invalid format`` once its loop over the parsed options finishes, if that sum exceeds the declared option area. Declaring exactly 8 does not skip the accommodation -- the short - ``data`` field is still read and left-padded as above -- it just trips + ``data`` field is still read and tail-padded as above -- it just trips that check afterwards, discarding the result before the test can assert on it. Declaring 16 leaves headroom, at the cost of a second effect: once the option loop's 16-octet budget outlives the 8 octets @@ -2069,7 +2072,12 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: ) 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) + # the six octets actually behind the option come first, where they were + # read, and the four synthesised for the ones that were not follow them. + # Before #604 they arrived the other way round. See #604 and + # ``FieldBaseShortReadPaddingSideTests`` in + # ``tests/corekit/test_fields_field.py``. + self.assertEqual(unassigned.data, trailing + b'\x00\x00\x00\x00') if __name__ == '__main__':