From bb001679ce540e13853d7a66cef55156d4451340 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 25 Sep 2026 18:05:52 -0400 Subject: [PATCH] fix(fields): raise ProtocolError, not struct.error, on a negative resolved field length (#805) - FieldBase.length (pcapkit/corekit/fields/field.py) now catches struct.error from struct.calcsize and re-raises ProtocolError. A length=lambda pkt: pkt['__length__']-style callback can resolve negative once Schema.unpack's running counter has been overdrawn by a preceding field, building a template such as '-5s' that struct.calcsize cannot size -- pre-fix this escaped as a bare struct.error, uncatchable by ordinary caller code. This is the one choke point every __length__-keyed field in the ten affected schema modules shares (application/ftp.py, httpv1.py, httpv2.py, ngap.py; internet/hip.py, ipv6_route.py, mh.py; link/ethernet.py; misc/pcapng.py; transport/sctp.py), so no other module needs a change, and success is untouched: the property still returns the same value for every non-negative length. - Left schema.py's running-counter warning alone: measured that converting it would reject a SETTINGS frame with a short trailing entry, which parses successfully today while only warning. - Updated test_http_unit.py's existing pinning test, which documented the direct-construction leak as expected pre-#805 behaviour, to assert ProtocolError instead; the guess-path's struct.error suppression stays as defence in depth. New: tests/corekit/test_fields_field.py::FieldBaseLengthNegativeResolvedLengthTests and tests/protocols/application/test_httpv2_negative_length_unit.py, each shown failing with a bare struct.error on stock code and passing after this change. Build/test: coverage run -m unittest over the touched files and their existing suites, 117 tests green; field.py's new lines fully covered, http.py and httpv2.py 100%. --- pcapkit/corekit/fields/field.py | 23 ++- tests/corekit/test_fields_field.py | 67 ++++++++ tests/protocols/application/test_http_unit.py | 58 +++---- .../test_httpv2_negative_length_unit.py | 153 ++++++++++++++++++ 4 files changed, 271 insertions(+), 30 deletions(-) create mode 100644 tests/protocols/application/test_httpv2_negative_length_unit.py diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index ffc543db9..20bc55303 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -8,7 +8,7 @@ from typing import TYPE_CHECKING, Generic, TypeVar, cast from pcapkit.utilities.compat import final -from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue +from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue, ProtocolError __all__ = ['Field'] @@ -269,8 +269,25 @@ def template(self) -> 'str': @property def length(self) -> 'int': - """Field size.""" - return struct.calcsize(self.template) + """Field size. + + Raises: + ProtocolError: If :attr:`template` resolves to a negative count + (e.g. ``'-5s'``, from a ``length`` callback such as + ``lambda pkt: pkt['__length__']`` resolving below zero once + the buffer ran short of what the schema declared). + :func:`struct.calcsize` cannot size such a template and would + otherwise raise a bare :exc:`struct.error`, uncatchable as a + pcapkit-specific error. See #805. + + """ + try: + return struct.calcsize(self.template) + except struct.error as error: + raise ProtocolError( + f'Field {self.name} resolved to a negative length; ' + f'template={self.template!r}' + ) from error @property def optional(self) -> 'bool': diff --git a/tests/corekit/test_fields_field.py b/tests/corekit/test_fields_field.py index dde1b6975..b00465cec 100644 --- a/tests/corekit/test_fields_field.py +++ b/tests/corekit/test_fields_field.py @@ -1,5 +1,6 @@ from __future__ import annotations +import struct import threading import unittest @@ -855,3 +856,69 @@ def test_a_short_read_round_trips_back_to_the_padded_wire_form(self) -> None: with self.subTest(width=width, byteorder=byteorder, kept=kept): self.assertEqual(field.pack(read, {}), buffer.ljust(width, b'\x00')) + + +class FieldBaseLengthNegativeResolvedLengthTests(unittest.TestCase): + """:attr:`FieldBase.length ` + on a negative resolved length. + + A ``length=lambda pkt: pkt['__length__']``-style callback is + attacker/corruption-controlled the same way :attr:`FieldBase.unpack`'s + ``length`` argument is (see the class above): :meth:`Schema.unpack + ` decrements + ``packet['__length__']`` by each field's nominal width regardless of how + many octets the buffer actually held, and a subsequent field's callback + can resolve to that negative remainder. :class:`~pcapkit.corekit.fields. + strings._TextField.__call__` then builds a template such as ``'-5s'`` + from it with no lower bound, and :func:`struct.calcsize` cannot size + that -- pre-fix this surfaced as a bare, uncatchable :exc:`struct.error` + (GitHub issue #805). + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + from pcapkit.corekit.fields.strings import BytesField + from pcapkit.utilities.exceptions import ProtocolError + + self.BytesField = BytesField + self.ProtocolError = ProtocolError + + def test_a_negative_resolved_length_raises_protocolerror_not_structerror(self) -> None: + """The crash reproduction from #805, isolated to the field property. + + Pre-fix, ``struct.error: bad char in struct format`` -- a bare + stdlib exception, not even a :exc:`ValueError`, uncatchable by + ordinary caller code -- escaped here. This is the exact shape + HTTP/2's ``GoawayFrame.debug``/``PushPromiseFrame.fragment`` fields + hit once their fixed-width siblings have already driven + ``pkt['__length__']`` negative. + """ + field = self.BytesField(length=lambda pkt: pkt['__length__'])({'__length__': -5}) + + self.assertEqual(field.template, '-5s') + with self.assertRaises(self.ProtocolError) as ctx: + field.length # noqa: B018 -- property access is the point + + # must not be a bare struct.error: ProtocolError is a ValueError + # subclass in this library's hierarchy, never struct.error itself. + self.assertIsInstance(ctx.exception, ValueError) + self.assertNotIsInstance(ctx.exception, struct.error) + + def test_a_zero_or_positive_resolved_length_is_unaffected(self) -> None: + """The fix must not change the answer for any non-negative length. + + The property is a thin ``try/except`` around the same + :func:`struct.calcsize` call as before; a resolved length that never + raises must return exactly what it always returned. + """ + for resolved in (0, 1, 5, 1024): + with self.subTest(resolved=resolved): + field = self.BytesField(length=lambda pkt: pkt['__length__'])( + {'__length__': resolved}) + self.assertEqual(field.length, resolved) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index 1af1164a3..2e3a575bf 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -951,15 +951,16 @@ def test_guess_version_does_not_suppress_struct_error_on_the_http1_arm(self) -> #799 closed the *outer*-header slice of the class this suppression exists for (``httpv2.HTTP.unpack`` now rejects a buffer under nine - octets before the schema layer runs), but did not retire the - suppression: a buffer that clears nine octets can still carry a frame - type -- ``GOAWAY``, ``PUSH_PROMISE``, a padded ``DATA``/``HEADERS`` -- - whose own fixed-width fields exceed what is left, and that still raises - a bare :exc:`struct.error` one field further in. See + octets before the schema layer runs), and #805 closed the *inner* + slice (a resolved field length going negative now raises + ``ProtocolError`` rather than a bare :exc:`struct.error`), but neither + retired the suppression: it stays as defence in depth for any + ``__length__``-keyed field #805 did not individually verify, across + the ten schema modules that share the pattern. See ``test_guess_version_reports_unknown_version_for_a_short_payload`` for - the outer-header case #799 did close, and - ``test_guess_version_still_leaks_a_bare_struct_error_for_an_inner_field_shortfall`` - below for the class it did not. + the outer-header case #799 closed, and + ``test_guess_version_no_longer_leaks_a_bare_struct_error_for_an_inner_field_shortfall`` + below for the inner one #805 closed. It is also unnecessary: nothing reaches a :exc:`struct.error` through ``httpv1.HTTP``. Nine byte patterns over lengths 0-24, on both the direct @@ -999,24 +1000,26 @@ def test_guess_version_does_not_suppress_struct_error_on_the_http1_arm(self) -> with self.assertRaises(struct.error): HTTP(io.BytesIO(raw), len(raw)) - def test_guess_version_still_leaks_a_bare_struct_error_for_an_inner_field_shortfall(self) -> None: - """A buffer that clears nine octets can still crash one field further in. + def test_guess_version_no_longer_leaks_a_bare_struct_error_for_an_inner_field_shortfall(self) -> None: + """A buffer that clears nine octets used to still crash one field further in. #799's ``httpv2.HTTP.unpack`` guard only protects the fixed nine-octet *outer* header. A ``GOAWAY`` frame's own fixed ``stream`` (4 octets) and ``error`` (4 octets) fields consume eight more octets before ``debug`` is even reached, so a sixteen-octet buffer -- nine for the header, seven - for the rest -- drives ``pkt['__length__']`` to ``-1`` at ``debug`` and - still raises a bare :exc:`struct.error` straight through ``httpv2.HTTP``. - Filed as its own issue (#805) rather than fixed here: the actual root is - generic ``Schema.unpack``/``FieldBase.length`` machinery shared by at - least ten schema modules, not something httpv2-specific. - - This is exactly why ``_guess_version``'s last arm keeps suppressing - :exc:`struct.error` alongside ``ProtocolError`` -- narrowing it, as an - earlier revision of this fix did, regressed the direct ``HTTP()`` guess - path: the same bytes came back a bare :exc:`struct.error` instead of - ``ProtocolError``, which a caller catching protocol errors cannot catch. + for the rest -- drives ``pkt['__length__']`` to ``-1`` at ``debug``. + Pre-#805, that raised a bare :exc:`struct.error` straight through + ``httpv2.HTTP``; #805 closed the actual root, generic + ``FieldBase.length`` (``struct.calcsize`` on a negative-count template), + shared by at least ten schema modules, not something httpv2-specific -- + so direct construction now raises :exc:`ProtocolError` too. + + ``_guess_version``'s last arm keeps suppressing :exc:`struct.error` + alongside ``ProtocolError`` regardless: narrowing it, as an earlier + revision of this fix did, regressed the direct ``HTTP()`` guess path + for a *different* input class (the sub-nine-octet outer-header one), + and #805 does not touch every ``__length__``-keyed field in every one + of those ten modules, so the suppression stays as defence in depth. This test drives real wire bytes through the actual guess path -- no ``mock.patch`` -- specifically because a mocked fault cannot see a regression in the *un-mocked* route the fault is meant to stand in for. @@ -1033,15 +1036,16 @@ def test_guess_version_still_leaks_a_bare_struct_error_for_an_inner_field_shortf raw = b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7 self.assertEqual(len(raw), 16) - # Direct construction is documented to leak the unwrapped struct.error - # (see the module docstring above and #805) -- pinned so a fix to #805 - # is noticed here rather than silently changing this contract too. + # Direct construction now raises ProtocolError, not a bare + # struct.error -- #805's fix, pinned here so a regression is noticed. import struct - with self.assertRaises(struct.error): + with self.assertRaises(ProtocolError) as direct_ctx: HTTPv2(io.BytesIO(raw), len(raw)) + self.assertNotIsInstance(direct_ctx.exception, struct.error) - # The guess path must not repeat that leak: _guess_version's arm 2 - # suppresses struct.error precisely so this comes back catchable. + # The guess path must answer the same way: _guess_version's arm 2 + # suppresses struct.error/ProtocolError precisely so this comes back + # catchable either way. for label, kwargs in (('guessed', {}), ('explicit version=2', {'version': 2})): with self.subTest(path=label): with self.assertRaises(ProtocolError) as ctx: diff --git a/tests/protocols/application/test_httpv2_negative_length_unit.py b/tests/protocols/application/test_httpv2_negative_length_unit.py new file mode 100644 index 000000000..cfa282521 --- /dev/null +++ b/tests/protocols/application/test_httpv2_negative_length_unit.py @@ -0,0 +1,153 @@ +# -*- coding: utf-8 -*- +"""A negative resolved field length must raise ``ProtocolError``, not ``struct.error``. + +GitHub issue #805. :meth:`~pcapkit.protocols.schema.schema.Schema.unpack` +decrements ``packet['__length__']`` by each field's *nominal* width +regardless of how many octets the buffer actually had, and a field whose own +``length=lambda pkt: pkt['__length__']``-style callback resolves to that +negative remainder builds a struct template such as ``'-5s'``. +:func:`struct.calcsize` cannot size that and raised a bare +:exc:`struct.error` for it -- not a :exc:`ProtocolError`, not even a +:exc:`ValueError`, uncatchable by ordinary caller code. + +Scope, per the issue's own correction of its body: the fix belongs at the +*resolved field length* -- :attr:`~pcapkit.corekit.fields.field.FieldBase.length` +-- not at the running-counter warning (``pcapkit/protocols/schema/schema.py`` +around ``packet['__length__'] < 0``), which this module does not touch. See +:mod:`tests.corekit.test_fields_field`'s ``FieldBaseLengthNegativeResolvedLengthTests`` +for the property-level unit tests and the unaffected non-negative control. + +``read()``'s own ``schema.length > length`` guard (`httpv2.py`) cannot see +this class of input: the crash happens while resolving the *inner* frame's +own fields, during :meth:`Schema.unpack`, before that comparison ever runs. +So these are built by constructing :class:`HTTP` (HTTP/2) directly, which is +how the issue itself reproduces the defect. + +""" + +from __future__ import annotations + +import importlib.util +import io +import struct +import unittest +import warnings + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +def http2_frame_bytes(type_: 'int', flags: 'int', sid: 'int', payload: 'bytes') -> 'bytes': + """Build the wire octets of one HTTP/2 frame. + + Args: + type_: Frame type octet. + flags: Frame flags octet. + sid: Stream identifier. + payload: The frame payload, header excluded. + + Returns: + The packed frame, its 3-octet length field counting the whole frame + (header included), this library's convention. + + """ + return ( + (len(payload) + 9).to_bytes(3, 'big') + + bytes([type_, flags]) + + sid.to_bytes(4, 'big') + + payload + ) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class NegativeResolvedLengthUnitTests(unittest.TestCase): + """The #805 reproduction, and the well-formed inputs it must not touch.""" + + def test_goaway_at_sixteen_octets_raises_protocolerror_not_structerror(self) -> None: + """The issue's own repro: a 16-octet ``GOAWAY`` frame. + + ``stream``+``error`` alone are eight fixed octets, so a 16-octet + buffer (7 octets of frame-specific payload after the 9-octet header) + drives ``debug``'s ``length=lambda pkt: pkt['__length__']`` to -1. + Pre-fix: ``struct.error: bad char in struct format``. + """ + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.utilities.exceptions import ProtocolError + + data = b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7 + self.assertEqual(len(data), 16) + + with self.assertRaises(ProtocolError) as ctx: + HTTPv2(io.BytesIO(data), 16) + self.assertNotIsInstance(ctx.exception, struct.error) + + def test_goaway_buflens_nine_through_sixteen_all_raise_protocolerror(self) -> None: + """Every ``GOAWAY`` buffer length the issue names as an escape. + + ``buflen`` 9 through 16: the fixed ``stream``/``error`` fields alone + need eight payload octets, so every one of these drives ``debug`` + negative. + """ + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.utilities.exceptions import ProtocolError + + for buflen in range(9, 17): + payload = b'\x00' * (buflen - 9) + raw = http2_frame_bytes(0x07, 0x00, 0, payload) + with self.subTest(buflen=buflen): + with self.assertRaises(ProtocolError) as ctx: + HTTPv2(io.BytesIO(raw), buflen) + self.assertNotIsInstance(ctx.exception, struct.error) + + def test_push_promise_buflens_nine_through_twelve_all_raise_protocolerror(self) -> None: + """Every ``PUSH_PROMISE`` buffer length the issue names as an escape. + + The promised stream identifier alone is four octets, so buffers 9 + through 12 leave ``fragment`` negative. + """ + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.utilities.exceptions import ProtocolError + + for buflen in range(9, 13): + payload = b'\x00' * (buflen - 9) + raw = http2_frame_bytes(0x05, 0x04, 1, payload) + with self.subTest(buflen=buflen): + with self.assertRaises(ProtocolError) as ctx: + HTTPv2(io.BytesIO(raw), buflen) + self.assertNotIsInstance(ctx.exception, struct.error) + + def test_an_over_padded_data_frame_raises_protocolerror(self) -> None: + """``pad_len`` exceeding the payload area, for a ``DATA`` frame.""" + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.utilities.exceptions import ProtocolError + + payload = bytes([20]) # PADDED, pad_len=20, no data and no padding octets follow + raw = http2_frame_bytes(0x00, 0x08, 1, payload) + + with self.assertRaises(ProtocolError) as ctx: + HTTPv2(io.BytesIO(raw), len(raw)) + self.assertNotIsInstance(ctx.exception, struct.error) + + def test_a_well_formed_goaway_frame_still_parses_cleanly(self) -> None: + """Control: a ``GOAWAY`` frame whose buffer matches its declared length. + + The fix is a ``try``/``except struct.error`` around the same + :func:`struct.calcsize` call as before, so a resolved length that + never goes negative must parse identically to pre-fix -- no + exception, no warning. + """ + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + + debug = b'ok' + payload = (5).to_bytes(4, 'big') + (0).to_bytes(4, 'big') + debug + raw = http2_frame_bytes(0x07, 0x00, 1, payload) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + info = HTTPv2(io.BytesIO(raw), len(raw)).info + + self.assertEqual(info.debug_data, debug) + self.assertEqual([str(w.message) for w in caught], []) + +if __name__ == '__main__': + unittest.main()