fix(http): check the buffer's length, not just the declared one, in HTTP/2's frame guard

pcapkit/protocols/application/http.py

@@ -216,33 +216,40 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP':
         # NOTE: The two arms suppress different sets, and the asymmetry is
         # deliberate. Only the *last* arm additionally suppresses
         # :exc:`struct.error`, because a payload too short to hold HTTP/2's
-        # nine-octet frame header usually fails inside the schema machinery with
-        # that stdlib exception rather than with a protocol error --
-        # ``FieldBase.length`` calls :func:`struct.calcsize` on a template built
-        # from a negative length -- and it is neither a ``ProtocolError`` nor a
-        # :exc:`ValueError`, so it used to leave this method uncatchable by any
-        # caller: ``HTTP(io.BytesIO(b'\x00' * 8), 8)`` raised a bare
-        # :exc:`struct.error` where the closing ``raise`` below is the documented
-        # answer. ("Usually" because ``httpv2.HTTP``'s own guard tests the
-        # *declared* length, not the buffer's, so a four-octet payload declaring
-        # fifteen parses instead of failing.) Suppressing it on the last arm is
-        # safe in the sense that matters here -- no arm follows, so nothing can
-        # answer in place of the error.
+        # nine-octet frame header, or one whose frame-specific fields exceed
+        # what a slightly-longer buffer holds, fails inside the schema
+        # machinery with that stdlib exception rather than with a protocol
+        # error -- ``FieldBase.length`` calls :func:`struct.calcsize` on a
+        # template built from a negative length -- and it is neither a
+        # ``ProtocolError`` nor a :exc:`ValueError`, so it left this method
+        # uncatchable by any caller: ``HTTP(io.BytesIO(b'\x00' * 8), 8)`` raised
+        # a bare :exc:`struct.error` where the closing ``raise`` below is the
+        # documented answer. Suppressing it on the last arm is safe in the
+        # sense that matters here -- no arm follows, so nothing can answer in
+        # place of the error.
         #
-        # The residual, which is real and is not zero: a genuine ``httpv2``
-        # schema defect that raised :exc:`struct.error` on well-formed HTTP/2
-        # bytes would now be reported as ``unknown HTTP version`` rather than
-        # crashing loudly, so a bug in that schema is quieter than it was. That is
-        # accepted because the alternative is a dispatcher no caller can catch,
-        # and it is bounded: ``httpv2.HTTP`` stays reachable directly, where
-        # nothing is suppressed, and that is the documented route for a caller who
-        # wants the unwrapped failure.
-        #
-        # This is keyed on being *last*, not on being the HTTP/2 arm. Inserting an
-        # arm after this one would silently make the reasoning false, and the
-        # regression test guards arm 1 specifically, so it would not catch that:
-        # the new arm must take the :exc:`struct.error` suppression and this one
-        # must give it up.
+        # #799 closed the *outer*-header slice of this: ``httpv2.HTTP.unpack``
+        # now rejects a buffer under nine octets before the schema layer runs
+        # at all, and ``read`` requires the declared length, the available
+        # buffer, and their consistency (``schema.length <= length``) all to
+        # hold. That did *not* retire this suppression, only shrink what it
+        # has to catch: a buffer that clears nine octets can still carry a
+        # frame type whose own fixed-width fields exceed what is left after
+        # the header -- a ``GOAWAY`` at 9-16 octets (``stream`` and ``error``
+        # alone are eight), a ``PUSH_PROMISE`` at 9-12, or any ``PADDED``
+        # ``DATA``/``HEADERS``/``PUSH_PROMISE`` whose ``pad_len`` exceeds the
+        # remainder -- and those still drive ``pkt['__length__']`` negative one
+        # field further in, past this guard's reach. Measured: a 16-octet
+        # ``GOAWAY`` (``b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7``)
+        # still raises a bare :exc:`struct.error` through ``httpv2.HTTP``
+        # directly. Closing that class needs the fix at its actual root --
+        # a negative field length raising :exc:`~pcapkit.utilities.\
+        # exceptions.ProtocolError` in :meth:`Schema.unpack
+        # <pcapkit.protocols.schema.schema.Schema.unpack>` /
+        # :attr:`FieldBase.length <pcapkit.corekit.fields.field.FieldBase.length>`
+        # instead of the bare warning those currently emit -- which is generic
+        # across every schema in the tree and is out of this change's scope;
+        # tracked as #805 rather than attempted here.
         #
         # Widening the *first* arm the same way was measured and reverted, as it
         # buys nothing and costs a great deal. Nothing reaches a

pcapkit/protocols/application/httpv2.py

@@ -188,6 +188,78 @@ def version(self) -> 'Literal["2"]':
     # Methods.
     ##########################################################################
 
+    def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP':
+        """Unpack (parse) packet data.
+
+        Args:
+            length: Length of packet data.
+            **kwargs: Arbitrary keyword arguments.
+
+        Returns:
+            Parsed packet data.
+
+        Raises:
+            ProtocolError: If the packet is malformed.
+
+        Notes:
+            This guards ahead of :meth:`Schema.unpack
+            <pcapkit.protocols.schema.schema.Schema.unpack>` rather than
+            leaving the same check to :meth:`read`, because a buffer shorter
+            than the fixed 9-octet header is not merely invalid -- it can
+            crash the schema layer outright. ``pkt['__length__']`` there is
+            decremented by each field's *nominal* width regardless of how
+            many octets the buffer actually had, so it goes negative, and the
+            frame payload's length-derived fields (e.g. an unpadded ``DATA``
+            frame's ``data: BytesField(length=lambda pkt: pkt['__length__'])``)
+            resolve to that negative number. A negative field length becomes
+            a struct template such as ``'-5s'``, and :func:`struct.calcsize`
+            raises :exc:`struct.error` for it -- uncaught, since nothing
+            downstream expects a :exc:`ProtocolError` to be spelled that way.
+            Rejecting here, before :meth:`Schema.unpack` ever runs, keeps a
+            frame too short to hold a header from reaching that arithmetic at
+            all. This closes the *outer*-header class only -- an inner payload
+            field can still drive ``pkt['__length__']`` negative on a buffer
+            that clears nine (e.g. a ``GOAWAY`` frame at 9-16 octets, whose
+            fixed ``stream`` and ``error`` fields alone consume eight); that
+            residual is why :meth:`HTTP._guess_version
+            <pcapkit.protocols.application.http.HTTP._guess_version>` still
+            suppresses :exc:`struct.error` on its last arm. See #799.
+
+            ``length`` is resolved against :func:`len` only for *this*
+            method's own check, and the *original* argument -- ``None``
+            included -- is what is actually forwarded to :meth:`Protocol.unpack
+            <pcapkit.protocols.protocol.ProtocolBase.unpack>`. Collapsing
+            ``None`` to a concrete ``0`` before forwarding would change what
+            :meth:`Schema.unpack`'s own ``prepare`` decorator does with a
+            now-exhausted stream: it raises
+            :exc:`~pcapkit.utilities.exceptions.StreamEOFError` (an
+            :exc:`EOFError`, the documented "no more packets" signal) only when
+            the *caller* left ``length`` unspecified, and forwarding a resolved
+            ``0`` instead would report an exhausted stream as a malformed
+            packet. A *non-zero* short buffer is unambiguously malformed either
+            way, so it is rejected here regardless of whether ``length`` was
+            given explicitly.
+
+        """
+        if length is None:
+            # ``0 < ... < 9`` rather than ``... < 9``: an exhausted stream
+            # (``len(self) == 0``) is left alone here, so ``length`` reaches
+            # :meth:`Protocol.unpack <pcapkit.protocols.protocol.ProtocolBase.unpack>`
+            # still ``None`` and its ``prepare`` decorator raises
+            # :exc:`StreamEOFError` as documented above -- only a *non-empty*
+            # short stream is rejected as malformed here.
+            if 0 < len(self) < 9:
+                raise ProtocolError(f'HTTP/2: invalid format, packet ({len(self)} octet(s)) '
+                                    f'shorter than the 9-octet frame header')
+        elif length < 9:
+            # An *explicit* length, zero included, is a caller assertion about
+            # how much data this frame has -- not the "figure it out"
+            # ``None`` above -- so every value under nine is rejected the same
+            # way, with no exhaustion signal to preserve.
+            raise ProtocolError(f'HTTP/2: invalid format, packet ({length} octet(s)) '
+                                f'shorter than the 9-octet frame header')
+        return super().unpack(length, **kwargs)
+
     def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP':
         """Read Hypertext Transfer Protocol (HTTP/2).
 
@@ -220,7 +292,23 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP':
             length = len(self)
         schema = self.__header__
 
-        if schema.length < 9:
+        # NOTE: ``schema.length`` is the *declared* length off the wire -- the
+        # 24-bit field a hostile or truncated capture controls outright -- and
+        # checking only it lets a frame whose buffer holds far fewer octets than
+        # it claims sail through this guard and report a length nothing backs.
+        # ``length`` is the actual number of octets available for this frame
+        # (``Protocol.__len__`` returns ``len(self._data)``, and a caller that
+        # supplies ``length`` explicitly means the same thing by it), so both
+        # have to clear the minimum header size for the frame to be viable at
+        # all, *and* the declared value must not exceed what is actually
+        # available -- this library's convention (see ``_make_http_length``)
+        # is that ``length`` counts the whole frame, header included, so the
+        # two are directly comparable. Without the third clause a frame that
+        # declares far more than its buffer holds -- the headline case in
+        # #799, e.g. a nine-octet buffer declaring 16777215 -- still passed
+        # this guard and reported the declared, attacker-controlled length as
+        # if the capture actually contained it. See #799.
+        if schema.length < 9 or length < 9 or schema.length > length:
             raise ProtocolError(f'HTTP/2: [Type {schema.type}] invalid format')
         if schema.type in (Enum_Frame.SETTINGS, Enum_Frame.PING) and schema.stream['sid'] != 0:
             raise ProtocolError(f'HTTP/2: [Type {schema.type}] invalid format')