fix(corekit): bound FieldBase.unpack's rjust() padding to a sane ceiling (#554)

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).

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

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 <pcapkit.corekit.fields.collections.
+#: ListField.unpack>` and :meth:`OptionField.unpack <pcapkit.corekit.fields.
+#: collections.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)
 

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 <pcapkit.corekit.fields.field.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 <pcapkit.corekit.fields.collections.ListField.unpack>`
+    and :meth:`OptionField.unpack <pcapkit.corekit.fields.collections.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'')