diff --git a/CHANGELOG.md b/CHANGELOG.md index 91a33a81e..112747cf5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- constant lookups that rejected a value the registry defines. `RouterAlert(0)` is the only value [RFC 2113](https://datatracker.ietf.org/doc/html/rfc2113) defines and the one IGMP, RSVP and MLD actually send, and it was discarded because the vendor crawler skipped a header row IANA's CSV does not have; IPX `Socket(0)` is that protocol's own default, so `bytes(IPX(...))` crashed on its own defaults; and two FTP `_missing_` overrides were plain methods rather than classmethods, so every unregistered value raised `TypeError` instead of extending the enumeration (#492, #503). - **Fixed** -- `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did. - **Fixed** -- 45 places where a documentation page contradicted the code (#413), ambiguous cross-references and five autodoc signature failures (#416), and `Extractor`'s documented exception plus 40 phantom or stale `Args:` labels (#501). +- **Fixed** -- `FieldBase.unpack` zero-padded straight up to a field's declared `length` with `rjust()`, regardless of how little data `buffer` actually held; a ~40-octet PCAP-NG Decryption Secrets Block with a bogus inner length was enough to force a multi-gigabyte allocation, since `length` is frequently wire-derived and so attacker-controlled. A declared length past 262144 octets -- libpcap's own `MAXIMUM_SNAPLEN`, and this package's own default `snaplen` -- that the buffer cannot back now raises `FieldValueError` instead of padding for it; the option and list loops' own tolerance for a short read past a truncated area (#431) is far under that ceiling and is untouched (#554). - **Fixed** -- `TCP._make_mptcp_addaddr` could not build an `ADD_ADDR` option end to end: its `kind=`/`length=` arguments were rejected with `UnknownFieldWarning` and silently dropped, and `.pack()` then raised `KeyError: 'length'` from `port`'s own condition, `pkt['length'] in (10, 22)`. The cause was one layer up -- `MPTCP`, the base class every Multipath TCP subtype schema inherits, declared `kind` and `length` only under `typing.TYPE_CHECKING` rather than as real fields, unlike `Option`, which every non-Multipath TCP option schema inherits instead. That silently dropped `kind=`/`length=` for every `_make_mptcp_*` constructor, not only `ADD_ADDR`'s, so `MPTCP` now declares both for real, the same way `Option` already did (#541). The same missing fields broke parsing too: with no `kind`/`length` fields ahead of it, a Multipath TCP subtype schema's own leading field read the `kind` octet itself rather than the octet meant for it, an off-by-two in field alignment rather than a wire-format change -- a correct sender's octets were always right, only this library's reading of them was shifted. Spec-correct `ADD_ADDR` and `MP_PRIO` options failed to parse with `FieldError: TCP: [OptNo 30] 3 invalid IP version` and `KeyError: 'length'` respectively; both parse correctly now. - **Fixed** -- which exception a malformed TCP SACK option raised depended on unrelated process state: a clean interpreter raised `ProtocolError` as documented, but a process that had already popped `pcapkit.corekit.fields.misc` from `sys.modules` -- which the `#439` ABC-cache regression tests do in every case's `setUp`/`tearDown` -- raised `FieldValueError` instead, from a different layer entirely, before the documented check was even reached (#525). The cause was `ListField.unpack` resolving `SchemaField` through a function-local import re-run on every call; a module popped and reimported mid-process comes back as a second, distinct class, so `isinstance` against it silently misclassified the field and billed each item by its declared length instead of by what it actually consumed. **Any caller relying on the previously-observed** `FieldValueError` **for this case now gets** `ProtocolError` **instead, deterministically**, matching the method's own docstring. Fixed by importing at module level instead. - **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index e50ac80b4..d9b6d6bab 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -299,6 +299,16 @@ pull requests between #326 and #509. (#413), ambiguous cross-references and five autodoc signature failures (#416), and ``Extractor``'s documented exception plus 40 phantom or stale ``Args:`` labels (#501). +* **Fixed** -- ``FieldBase.unpack`` zero-padded straight up to a field's + declared ``length`` with ``rjust()``, regardless of how little data + ``buffer`` actually held; a ~40-octet PCAP-NG Decryption Secrets Block with a + bogus inner length was enough to force a multi-gigabyte allocation, since + ``length`` is frequently wire-derived and so attacker-controlled. A declared + length past 262144 octets -- libpcap's own ``MAXIMUM_SNAPLEN``, and this + package's own default ``snaplen`` -- that the buffer cannot back now raises + ``FieldValueError`` instead of padding for it; the option and list loops' + own tolerance for a short read past a truncated area (#431) is far under + that ceiling and is untouched (#554). * **Fixed** -- ``TCP._make_mptcp_addaddr`` could not build an ``ADD_ADDR`` option end to end: its ``kind=``/``length=`` arguments were rejected with ``UnknownFieldWarning`` and silently dropped, and ``.pack()`` then raised diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index e6f191c6a..b1c622edb 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -7,7 +7,7 @@ from typing import TYPE_CHECKING, Generic, TypeVar, cast from pcapkit.utilities.compat import final -from pcapkit.utilities.exceptions import NoDefaultValue +from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue __all__ = ['Field'] @@ -33,6 +33,30 @@ def __bool__(self) -> 'Literal[False]': #: NoValueType: Default value for :attr:`FieldBase.default`. NoValue = NoValueType() +#: int: Ceiling on the zero-padding :meth:`FieldBase.unpack` will still perform +#: for a field whose declared length outruns its buffer. +#: +#: This is libpcap's own ``MAXIMUM_SNAPLEN`` -- the point past which libpcap +#: itself treats a capture's declared snapshot length as corrupt or +#: byte-order-swapped rather than real -- and it is the same figure this +#: package's own PCAP writer defaults ``snaplen`` to +#: (:meth:`pcapkit.protocols.misc.pcap.header.Header.make`). No single field +#: within one captured packet is legitimately larger than the largest packet +#: libpcap itself is willing to believe, so nothing this library parses +#: should ever declare a length past it. +#: +#: The bound is deliberately *not* "any declared length beyond what the +#: buffer holds": :meth:`ListField.unpack ` and :meth:`OptionField.unpack ` depend on reading a short, sometimes +#: empty, tail past a truncated area and having it decode as zero -- that is +#: how an over-long ``ihl``, or a capture cut short by the snapshot length, +#: reads as end-of-option-list or ``Pad1`` instead of wedging or raising (see +#: #431). Every such read is of a fixed-width, few-octet field, always far +#: under this ceiling, so it is untouched; only a length past it -- which no +#: fixed-width field ever legitimately is -- gets refused. +_MAX_ZERO_PAD_LENGTH = 0x40_000 + class FieldMeta(abc.ABCMeta, Generic[_T]): """Meta class to add dynamic support to :class:`FieldBase`. @@ -233,6 +257,10 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': Returns: Unpacked field value. + Raises: + FieldValueError: If ``buffer`` holds fewer octets than :attr:`length` + declares, and ``length`` is past :data:`_MAX_ZERO_PAD_LENGTH`. + """ # NOTE: ``length`` recomputes struct.calcsize() on every read, so the # three reads this method used to make were three calcsize() calls for @@ -241,6 +269,25 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': if not isinstance(buffer, bytes): buffer = buffer.read(length) + + # NOTE: ``length`` is frequently wire-derived -- resolved by a + # ``_length_callback`` against the very packet being parsed, per + # :meth:`Field.__call__` below, or by a schema's own selector building a + # field from a value it just read off the wire (e.g. ``DecryptionSecretsBlock``'s + # ``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 + # 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. + if length > _MAX_ZERO_PAD_LENGTH and len(buffer) < length: + raise FieldValueError( + f'Field {self.name} declares a length of {length} octet(s), ' + f'but only {len(buffer)} octet(s) are available.' + ) + value = struct.unpack(self.template, buffer[:length].rjust(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 new file mode 100644 index 000000000..4914cedfa --- /dev/null +++ b/tests/corekit/test_fields_field.py @@ -0,0 +1,175 @@ +from __future__ import annotations + +import unittest + +from tests._support import purge_modules, time_limit + + +class FieldBaseUnpackBoundsTests(unittest.TestCase): + """Bounds checking in :meth:`FieldBase.unpack `. + + ``length`` there is frequently wire-derived -- resolved by a + ``_length_callback`` against the very packet under parse, or built by a + schema selector from a value it just read off the wire -- and is thus + attacker- or corruption-controlled. Before #554, + ``buffer[:length].rjust(length, b'\\x00')`` zero-padded up to ``length`` + octets regardless of how little data ``buffer`` actually held, so a + short, otherwise unremarkable capture could declare a multi-gigabyte + field and force that allocation. + + The fix is deliberately *not* "reject any length the buffer falls short + of": :meth:`ListField.unpack ` + and :meth:`OptionField.unpack ` + depend on a short, sometimes entirely empty, tail read past a truncated + option area decoding as zero -- that is how an over-long ``ihl``, or a + capture cut short by the snapshot length, reads as end-of-option-list or + ``Pad1`` instead of wedging or raising (#431). Rejecting every shortfall + regardless of size would turn every one of those into a hard failure. + + So the guard only fires past :data:`~pcapkit.corekit.fields.field._MAX_ZERO_PAD_LENGTH` + -- libpcap's own ``MAXIMUM_SNAPLEN`` and this package's own default + ``snaplen`` -- which every fixed-width field ``ListField``/``OptionField`` + read past EOF is nowhere near, and which no legitimate single field is + past either. + + :class:`~pcapkit.corekit.fields.strings.BytesField` is used throughout as + the concrete field under test: a real, user-facing field type that goes + through :meth:`FieldBase.unpack` unchanged, rather than a hand-rolled + stand-in. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + from pcapkit.corekit.fields import field as field_module + from pcapkit.corekit.fields.strings import BytesField + from pcapkit.utilities.exceptions import FieldValueError + + self.BytesField = BytesField + self.FieldValueError = FieldValueError + self.ceiling = field_module._MAX_ZERO_PAD_LENGTH + + def test_exact_length_buffer_unpacks_unchanged(self) -> None: + field = self.BytesField(length=4) + self.assertEqual(field.unpack(b'\x01\x02\x03\x04', {}), b'\x01\x02\x03\x04') + + def test_buffer_with_trailing_extra_bytes_is_still_accepted(self) -> None: + """More data than declared is fine -- only the declared prefix belongs to this field.""" + field = self.BytesField(length=4) + self.assertEqual(field.unpack(b'\x01\x02\x03\x04\xff\xff', {}), b'\x01\x02\x03\x04') + + def test_small_field_short_by_one_octet_still_zero_pads(self) -> None: + """A short read on an ordinary, small field is tolerated, not rejected. + + 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 + 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. + """ + field = self.BytesField(length=4) + self.assertEqual(field.unpack(b'\x01\x02\x03', {}), b'\x00\x01\x02\x03') + + def test_small_field_over_an_entirely_empty_buffer_still_zero_pads(self) -> None: + """The extreme case: nothing at all left to read. + + Exactly what ``OptionField.unpack`` does at the tail of a truncated + option area -- ``file.read(field.length)`` past EOF returns ``b''``, + and the one-octet type field it hands to :meth:`FieldBase.unpack` + must decode that as ``0`` (end-of-option-list, or ``Pad1``) rather + than raise. + """ + field = self.BytesField(length=1) + self.assertEqual(field.unpack(b'', {}), b'\x00') + + def test_field_at_the_ceiling_still_zero_pads(self) -> None: + """Boundary: a declared length exactly at the ceiling is not rejected. + + Catches a ceiling comparison written the lenient-for-attackers way + around (``>=`` instead of ``>``), which would reject this legitimate + boundary value along with the genuinely oversized ones. + """ + 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. + self.assertEqual(len(result), self.ceiling) + 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. + + Catches a ceiling comparison off by one in the other direction -- + e.g. a stray ``+ 1`` or ``- 1`` on the threshold -- by exercising the + single value the exact threshold has to get right. + """ + field = self.BytesField(length=self.ceiling + 1) + with self.assertRaises(self.FieldValueError): + field.unpack(b'A', {}) + + def test_the_ceiling_is_262144_octets(self) -> None: + """Pins the actual chosen figure, not just the module's own copy of it. + + 262144 (``0x40_000``) is libpcap's own ``MAXIMUM_SNAPLEN`` and this + package's own default ``snaplen`` + (:meth:`pcapkit.protocols.misc.pcap.header.Header.make`). A test that + only ever reads the constant back from the module would still pass + if that figure were quietly changed to something unjustified. + """ + self.assertEqual(self.ceiling, 262144) + + def test_field_past_the_ceiling_with_a_full_buffer_is_accepted(self) -> None: + """A field past the ceiling is rejected only when it would actually pad. + + If the buffer genuinely holds that much data there is nothing to + zero-fill and nothing to refuse -- the guard fires on the padding + :meth:`FieldBase.unpack` would have to perform, not merely on the + field being large. + """ + size = self.ceiling + 10 + field = self.BytesField(length=size) + buffer = b'\xaa' * size + + self.assertEqual(field.unpack(buffer, {}), buffer) + + def test_rejection_message_names_the_declared_and_available_counts(self) -> None: + field = self.BytesField(length=self.ceiling + 1) + with self.assertRaises(self.FieldValueError) as ctx: + field.unpack(b'A', {}) + + message = str(ctx.exception) + self.assertIn(str(self.ceiling + 1), message) + self.assertIn('1', message) + + def test_wire_declared_length_far_larger_than_the_buffer_is_rejected_without_allocating(self) -> None: + """The scenario #554 describes: a hostile or corrupt capture can make + ``length`` whatever it likes. + + A ~40-octet PCAP-NG Decryption Secrets Block with a bogus inner + length was enough to force a multi-gigabyte ``rjust()`` under the + unfixed code. ``length`` here is set to 16 GiB -- large enough that, + if the guard were absent, comparing against the wrong thing, or + otherwise not actually reached, this test would try to allocate that + much and either hang or fail with :exc:`MemoryError` rather than + raising the expected, bounded exception. It runs under a wall-clock + deadline rather than trusting the assertion alone to fail promptly, + in case a future regression reintroduces the allocation instead of + just moving the threshold. + """ + huge_declared_length = 2 ** 34 # 16 GiB; must never actually be allocated. + field = self.BytesField(length=huge_declared_length) + + with time_limit(5): + with self.assertRaises(self.FieldValueError): + field.unpack(b'AB', {}) + + def test_zero_length_field_over_an_empty_buffer_is_unaffected(self) -> None: + """A field declaring no octets at all is not a shortfall against an empty buffer.""" + field = self.BytesField(length=0) + self.assertEqual(field.unpack(b'', {}), b'')