From e1cce76ccebff9ba88962c5866ced305cbd1fa6a Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 6 Oct 2026 15:28:24 -0400 Subject: [PATCH] fix(httpv2)!: frame Length counts the 9-octet header, so RFC 9113 frames are rejected ``HTTP.make`` wrote Length as ``_make_http_length(...) + 9`` and ``read`` compared the declared value against the whole buffer, so every per-frame check was stated as payload + 9 -- and ``_read_http_priority`` checked 9, rejecting even the 14 ``make`` wrote for it. Length is now the frame payload alone, the header excluded (RFC 9113 4.1): PRIORITY 5, RST_STREAM 4, PING 8, WINDOW_UPDATE 4, PUSH_PROMISE >= 4, SETTINGS a multiple of 6 and 0 under ACK, and ``read`` requires the buffer to back nine octets plus the declared payload. Breaking: ``make`` emits different Length octets, and a capture written to the old convention no longer parses. Closes #1121 --- .../pcapkit/protocols/application/httpv2.rst | 2 +- pcapkit/protocols/application/httpv2.py | 29 ++-- pcapkit/protocols/data/application/httpv2.py | 2 +- .../protocols/schema/application/httpv2.py | 2 +- tests/protocols/application/test_http_unit.py | 126 ++++++++---------- .../test_httpv2_frame_length_1121_unit.py | 121 +++++++++++++++++ .../test_httpv2_negative_length_unit.py | 8 +- .../test_httpv2_payload_length_unit.py | 20 ++- tests/protocols/test_option_roundtrip_unit.py | 13 +- .../transport/test_tcp_http_dispatch_unit.py | 12 +- 10 files changed, 219 insertions(+), 116 deletions(-) create mode 100644 tests/protocols/application/test_httpv2_frame_length_1121_unit.py diff --git a/docs/source/pcapkit/protocols/application/httpv2.rst b/docs/source/pcapkit/protocols/application/httpv2.rst index 0041cea729..d3ca4b12c0 100644 --- a/docs/source/pcapkit/protocols/application/httpv2.rst +++ b/docs/source/pcapkit/protocols/application/httpv2.rst @@ -12,7 +12,7 @@ below: ======= ========= ===================== ========================== Octets Bits Name Description ======= ========= ===================== ========================== - 0 0 ``http.length`` Length + 0 0 ``http.length`` Payload Length 3 24 ``http.type`` Type 4 32 ``http.flags`` Flags 5 40 Reserved diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index 777e54a763..92bc6e0678 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -13,7 +13,7 @@ ======= ========= ===================== ========================== Octets Bits Name Description ======= ========= ===================== ========================== - 0 0 ``http.length`` Length + 0 0 ``http.length`` Payload Length 3 24 ``http.type`` Type 4 32 ``http.flags`` Flags 5 40 Reserved @@ -290,12 +290,11 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP': # than it claims report a length nothing backs (e.g. a nine-octet buffer # declaring 16777215). ``length`` is the number of octets actually # available for this frame (``Protocol.__len__`` returns - # ``len(self._data)``, and an explicit ``length`` means the same), so - # both have to clear the minimum header size *and* the declared value - # must not exceed what is available. This library's convention (see - # ``_make_http_length``) is that ``length`` counts the whole frame, - # header included, so the two are directly comparable. - if schema.length < 9 or length < 9 or schema.length > length: + # ``len(self._data)``, and an explicit ``length`` means the same), so it + # has to clear the 9-octet header *and* back the declared value. The + # declared value counts the frame payload only, the header excluded + # (:rfc:`9113#section-4.1`), so ``length`` must hold both. + if length < 9 or schema.length + 9 > 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') @@ -364,7 +363,7 @@ def make(self, # type: ignore[override] for bit in range(8): flags_val[f'bit_{bit}'] = (int(flags) & (1 << bit)) >> bit - length = self._make_http_length(frame_val, flags) + 9 + length = self._make_http_length(frame_val, flags) return Schema_HTTP( length=length, @@ -634,7 +633,7 @@ def _read_http_priority(self, schema: 'Schema_PriorityFrame', *, ProtocolError: If the packet is malformed. """ - if header.length != 9: + if header.length != 5: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') data = Data_PriorityFrame( @@ -677,7 +676,7 @@ def _read_http_rst_stream(self, schema: 'Schema_RSTStreamFrame', *, ProtocolError: If the packet is malformed. """ - if header.length != 13: + if header.length != 4: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') data = Data_RSTStreamFrame( @@ -721,7 +720,7 @@ def _read_http_settings(self, schema: 'Schema_SettingsFrame', *, ProtocolError: If the packet is malformed. """ - if (header.length - 9) % 6 != 0 or header.stream['sid'] != 0: + if header.length % 6 != 0 or header.stream['sid'] != 0: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') flag = Data_SettingsFrameFlags( @@ -731,7 +730,7 @@ def _read_http_settings(self, schema: 'Schema_SettingsFrame', *, '__value__': schema.__flags__, }) - if flag.ACK and header.length - 9 != 0: + if flag.ACK and header.length != 0: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') sets = OrderedMultiDict() # type: OrderedMultiDict[Enum_Setting, int] @@ -782,7 +781,7 @@ def _read_http_push_promise(self, schema: 'Schema_PushPromiseFrame', *, ProtocolError: If the packet is malformed. """ - if header.length < 13: + if header.length < 4: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') flag = Data_PushPromiseFrameFlags( @@ -836,7 +835,7 @@ def _read_http_ping(self, schema: 'Schema_PingFrame', *, ProtocolError: If the packet is malformed. """ - if header.length != 17: + if header.length != 8: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') flag = Data_PingFrameFlags( @@ -925,7 +924,7 @@ def _read_http_window_update(self, schema: 'Schema_WindowUpdateFrame', *, ProtocolError: If the packet is malformed. """ - if header.length != 13: + if header.length != 4: raise ProtocolError(f'HTTP/2: [Type {header.type}] invalid format') data = Data_WindowUpdateFrame( diff --git a/pcapkit/protocols/data/application/httpv2.py b/pcapkit/protocols/data/application/httpv2.py index a67c9bdf8f..3c4d821b0e 100644 --- a/pcapkit/protocols/data/application/httpv2.py +++ b/pcapkit/protocols/data/application/httpv2.py @@ -42,7 +42,7 @@ class Flags(Data): class HTTP(Protocol): """Data model for HTTP/2 protocol.""" - #: Length. + #: Length of the frame payload, the 9-octet header excluded. length: 'int' #: Frame type. type: 'Frame' diff --git a/pcapkit/protocols/schema/application/httpv2.py b/pcapkit/protocols/schema/application/httpv2.py index 623117cacb..8ed2162536 100644 --- a/pcapkit/protocols/schema/application/httpv2.py +++ b/pcapkit/protocols/schema/application/httpv2.py @@ -88,7 +88,7 @@ def http_frame_selector(pkt: 'dict[str, Any]') -> 'Field': class HTTP(Schema): """Header schema for HTTP/2 packet.""" - #: Length. + #: Length of the frame payload, the 9-octet header excluded. length: 'int' = NumberField(length=3, signed=False) #: Frame type. type: 'Enum_Frame' = EnumField(length=1, namespace=Enum_Frame) diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index d377ff1ac2..dd32867f1d 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -35,15 +35,12 @@ def http2_schema(**kwargs: object) -> SimpleNamespace: def http2_frame_bytes(type_: int, flags: int, sid: int, payload: bytes) -> bytes: """Build the wire bytes of one HTTP/2 frame. - The length field counts the *whole* frame, header included -- this - library's convention rather than :rfc:`9113#section-4.1`'s, as the NOTE in - :meth:`test_unregistered_frame_type_does_not_mutate_the_class_registry` - spells out. ``make`` writes ``payload + 9`` and the readers recover the - payload as ``length - 9``. + The length field counts the frame payload only, the 9-octet header + excluded [:rfc:`9113#section-4.1`]. """ return ( - (len(payload) + 9).to_bytes(3, 'big') + len(payload).to_bytes(3, 'big') + bytes([type_, flags]) + sid.to_bytes(4, 'big') + payload @@ -550,8 +547,8 @@ def test_guess_version_identifies_the_http2_connection_preface(self) -> None: * the parsed frame is *the SETTINGS frame*, byte-identical to reading that frame on its own -- which is what proves the preface was skipped rather than consumed as framing; - * the reported declared length is the frame's real 9, not a number the - preface's ASCII happens to spell. + * the reported declared length is the empty SETTINGS frame's real 0, + not a number the preface's ASCII happens to spell. """ import io @@ -579,7 +576,7 @@ def test_guess_version_identifies_the_http2_connection_preface(self) -> None: self.assertEqual(guessed.length, len(preface) + frame_only.length) self.assertEqual(guessed.info, frame_only.info) self.assertEqual(guessed.info.packet, b'') - self.assertEqual(guessed.info.length, 9) + self.assertEqual(guessed.info.length, 0) self.assertNotEqual(guessed.info.length, 5265993) self.assertEqual([w for w in caught if issubclass(w.category, ProtocolWarning)], []) @@ -912,8 +909,8 @@ def test_guess_version_still_reports_an_unknown_http_version(self) -> None: arms fail with :class:`~pcapkit.utilities.exceptions.ProtocolError`, so it needs a payload that ``httpv2.HTTP`` rejects too -- a frame whose declared length contradicts its type. A PING frame carries eight octets - of opaque data and so must declare 17 by this library's whole-frame - convention; 9 is refused by ``_read_http_ping``. + of opaque data and so must declare 8 [:rfc:`9113#section-6.7`]; 0 is + refused by ``_read_http_ping``. A frame is used as the probe rather than short garbage because it exercises the intended route: both arms declining with @@ -930,7 +927,7 @@ def test_guess_version_still_reports_an_unknown_http_version(self) -> None: from pcapkit.protocols.application.http import HTTP from pcapkit.utilities.exceptions import ProtocolError - raw = http2_frame_bytes(0x06, 0x00, 0, b'') # PING, declared length 9 + raw = http2_frame_bytes(0x06, 0x00, 0, b'') # PING, declared length 0 with self.assertRaises(ProtocolError) as ctx: HTTP(io.BytesIO(raw), len(raw)) @@ -1531,8 +1528,8 @@ def test_guess_version_no_longer_leaks_a_bare_struct_error_for_an_inner_field_sh from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 from pcapkit.utilities.exceptions import BaseError, ProtocolError - # GOAWAY (type 0x07), sid 0, declared length 0x15 (21, this library's - # whole-frame convention) -- but only sixteen octets actually follow. + # GOAWAY (type 0x07), sid 0, declared length 0x15 (21 octets of + # payload) -- but only sixteen octets are present, header included. raw = b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7 self.assertEqual(len(raw), 16) @@ -1827,7 +1824,7 @@ def test_httpv2_id_length_make_bytes_and_register_frame_warning(self) -> None: self.assertEqual(proto.version, '2') self.assertEqual(proto.length, 9) self.assertEqual(proto.__length_hint__(), 9) - self.assertEqual(schema.length, 13) + self.assertEqual(schema.length, 4) self.assertEqual(schema.type, Frame.DATA) self.assertEqual(schema.flags['bit_0'], 1) self.assertEqual(schema.stream['sid'], 3) @@ -1856,18 +1853,10 @@ def test_unregistered_frame_type_does_not_mutate_the_class_registry(self) -> Non from pcapkit.const.http.frame import Frame from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 - # Length 13, type 0xF0, no flags, stream 1, then four octets of payload. - # - # 13 is the *whole* frame, header included, which is this library's - # convention rather than :rfc:`9113#section-4.1`'s -- that one counts the - # payload alone, so a real frame with four octets of payload declares 4. - # ``make`` writes ``payload + 9`` (httpv2.py:292) and the readers recover - # the payload as ``length - 9`` (httpv2.py:658,668), and ``read`` rejects - # anything under 9 outright, so a wire-accurate 4 raises here. Declaring - # 13 is what reaches the registry lookup; the mismatch with the RFC is a - # separate defect and not this test's to assert. - packet = bytes.fromhex('00000d' 'f0' '00' '00000001' '61626364') - self.assertEqual(int.from_bytes(packet[:3], 'big'), len(packet)) + # Length 4, type 0xF0, no flags, stream 1, then four octets of payload. + # The length counts the payload only [:rfc:`9113#section-4.1`]. + packet = bytes.fromhex('000004' 'f0' '00' '00000001' '61626364') + self.assertEqual(int.from_bytes(packet[:3], 'big'), len(packet) - 9) registry = HTTPv2.__dict__['__frame__'] before = set(registry) @@ -1898,7 +1887,7 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: with mock.patch('pcapkit.protocols.application.httpv2.warn') as warn: none = proto._read_http_none( http2_schema(data=b'unknown'), - header=http2_header(10, 250, flags=0x01, sid=3), + header=http2_header(7, 250, flags=0x01, sid=3), ) self.assertEqual(none.to_dict()['data'], b'unknown') self.assertIsNone(none.flags) @@ -1907,14 +1896,14 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: with mock.patch('pcapkit.protocols.application.httpv2.warn') as warn: none_clean = proto._read_http_none( http2_schema(data=b'clean'), - header=SimpleNamespace(length=9, type=251, flags={}, stream={'sid': 4}), + header=SimpleNamespace(length=5, type=251, flags={}, stream={'sid': 4}), ) self.assertEqual(none_clean.data, b'clean') warn.assert_not_called() data = proto._read_http_data( http2_schema(pad_len=2, data=b'data', __flags__=0x09), - header=http2_header(16, Frame.DATA, flags=0x09, sid=1), + header=http2_header(7, Frame.DATA, flags=0x09, sid=1), ) self.assertTrue(data.flags.to_dict()['END_STREAM']) self.assertTrue(data.flags.to_dict()['PADDED']) @@ -1929,7 +1918,7 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: fragment=b'headers', __flags__=0x2D, ), - header=http2_header(22, Frame.HEADERS, flags=0x2D, sid=1), + header=http2_header(14, Frame.HEADERS, flags=0x2D, sid=1), ) self.assertTrue(headers.flags.to_dict()['END_STREAM']) self.assertTrue(headers.flags.to_dict()['END_HEADERS']) @@ -1941,7 +1930,7 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: priority = proto._read_http_priority( http2_schema(stream={'exclusive': 1, 'sid': 5}, weight=9), - header=http2_header(9, Frame.PRIORITY, sid=1), + header=http2_header(5, Frame.PRIORITY, sid=1), ) self.assertTrue(priority.to_dict()['excl_dependency']) self.assertEqual(priority.to_dict()['stream_dependency'], 5) @@ -1949,7 +1938,7 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: rst = proto._read_http_rst_stream( http2_schema(error=ErrorCode.NO_ERROR), - header=http2_header(13, Frame.RST_STREAM, sid=1), + header=http2_header(4, Frame.RST_STREAM, sid=1), ) self.assertEqual(rst.to_dict()['error'], ErrorCode.NO_ERROR) @@ -1958,7 +1947,7 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: SimpleNamespace(id=Setting.HEADER_TABLE_SIZE, value=4096), SimpleNamespace(id=Setting.ENABLE_PUSH, value=0), ], __flags__=0), - header=http2_header(21, Frame.SETTINGS, sid=0), + header=http2_header(12, Frame.SETTINGS, sid=0), ) self.assertFalse(settings.flags.to_dict()['ACK']) self.assertEqual(settings.settings[Setting.HEADER_TABLE_SIZE], 4096) @@ -1966,7 +1955,7 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: push = proto._read_http_push_promise( http2_schema(pad_len=1, stream={'sid': 11}, fragment=b'push', __flags__=0x0C), - header=http2_header(18, Frame.PUSH_PROMISE, flags=0x0C, sid=1), + header=http2_header(10, Frame.PUSH_PROMISE, flags=0x0C, sid=1), ) self.assertTrue(push.flags.to_dict()['END_HEADERS']) self.assertTrue(push.flags.to_dict()['PADDED']) @@ -1975,27 +1964,27 @@ def test_httpv2_frame_readers_cover_successful_frames(self) -> None: ping = proto._read_http_ping( http2_schema(data=b'12345678', __flags__=0x01), - header=http2_header(17, Frame.PING, flags=0x01, sid=0), + header=http2_header(8, Frame.PING, flags=0x01, sid=0), ) self.assertTrue(ping.flags.to_dict()['ACK']) self.assertEqual(ping.to_dict()['data'], b'12345678') goaway = proto._read_http_goaway( http2_schema(stream={'sid': 13}, error=ErrorCode.NO_ERROR, debug=b'bye'), - header=http2_header(20, Frame.GOAWAY, sid=0), + header=http2_header(11, Frame.GOAWAY, sid=0), ) self.assertEqual(goaway.to_dict()['last_sid'], 13) self.assertEqual(goaway.to_dict()['debug_data'], b'bye') window = proto._read_http_window_update( http2_schema(size={'incr': 65535}), - header=http2_header(13, Frame.WINDOW_UPDATE, sid=1), + header=http2_header(4, Frame.WINDOW_UPDATE, sid=1), ) self.assertEqual(window.to_dict()['increment'], 65535) continuation = proto._read_http_continuation( http2_schema(fragment=b'cont', __flags__=0x04), - header=http2_header(13, Frame.CONTINUATION, flags=0x04, sid=1), + header=http2_header(4, Frame.CONTINUATION, flags=0x04, sid=1), ) self.assertTrue(continuation.flags.to_dict()['END_HEADERS']) self.assertEqual(continuation.to_dict()['fragment'], b'cont') @@ -2009,25 +1998,25 @@ def test_httpv2_frame_readers_reject_malformed_lengths(self) -> None: with self.assertRaises(ProtocolError): proto._read_http_priority(http2_schema(stream={'exclusive': 0, 'sid': 1}, weight=1), - header=http2_header(10, Frame.PRIORITY)) + header=http2_header(1, Frame.PRIORITY)) with self.assertRaises(ProtocolError): proto._read_http_rst_stream(http2_schema(error=0), - header=http2_header(12, Frame.RST_STREAM)) + header=http2_header(3, Frame.RST_STREAM)) with self.assertRaises(ProtocolError): proto._read_http_settings(http2_schema(settings=[], __flags__=0), - header=http2_header(14, Frame.SETTINGS, sid=0)) + header=http2_header(5, Frame.SETTINGS, sid=0)) with self.assertRaises(ProtocolError): proto._read_http_settings(http2_schema(settings=[], __flags__=0x01), - header=http2_header(15, Frame.SETTINGS, flags=0x01, sid=0)) + header=http2_header(6, Frame.SETTINGS, flags=0x01, sid=0)) with self.assertRaises(ProtocolError): proto._read_http_push_promise(http2_schema(pad_len=0, stream={'sid': 1}, fragment=b''), - header=http2_header(12, Frame.PUSH_PROMISE)) + header=http2_header(3, Frame.PUSH_PROMISE)) with self.assertRaises(ProtocolError): proto._read_http_ping(http2_schema(data=b'bad', __flags__=0), - header=http2_header(16, Frame.PING, sid=0)) + header=http2_header(7, Frame.PING, sid=0)) with self.assertRaises(ProtocolError): proto._read_http_window_update(http2_schema(size={'incr': 1}), - header=http2_header(12, Frame.WINDOW_UPDATE)) + header=http2_header(3, Frame.WINDOW_UPDATE)) def test_httpv2_read_validates_common_frame_header_rules(self) -> None: from pcapkit.const.http.frame import Frame @@ -2044,17 +2033,17 @@ def test_httpv2_read_validates_common_frame_header_rules(self) -> None: with self.assertRaises(ProtocolError): proto.read(length=17) - proto.__header__ = http2_header(9, Frame.DATA, sid=1) + proto.__header__ = http2_header(0, Frame.DATA, sid=1) proto.__header__.frame = http2_schema(pad_len=0, data=b'', __flags__=0) self.assertEqual(proto.read(length=9).to_dict()['type'], Frame.DATA) proto._data = b'\x00' * 9 proto.__cached__ = {} self.assertEqual(proto.read().to_dict()['type'], Frame.DATA) - # #799: a well-formed *declared* length (9, i.e. clears the header-only - # guard on its own) must still be refused if the *available* buffer - # does not also clear nine -- ``read``'s guard now checks both. - proto.__header__ = http2_header(9, Frame.DATA, sid=1) + # #799: a well-formed *declared* length (0, an empty payload) must + # still be refused if the *available* buffer does not clear the + # nine-octet header -- ``read``'s guard checks both. + proto.__header__ = http2_header(0, Frame.DATA, sid=1) proto.__header__.frame = http2_schema(pad_len=0, data=b'', __flags__=0) with self.assertRaises(ProtocolError): proto.read(length=8) @@ -2114,8 +2103,8 @@ def test_httpv2_truncated_frame_is_rejected_across_frame_types_and_buffer_length """The uniformity holds for other frame types and other buffer sizes too. The ``SETTINGS``-specific sweep above shows the exact reported shape, - but the guard fix (``schema.length < 9 or length < 9 or schema.length - > length``) is general: it must reject a too-short buffer for *any* + but the guard (``length < 9 or schema.length + 9 > length``) is + general: it must reject a too-short buffer for *any* frame type, and at every buffer length under nine, not only four. """ @@ -2172,19 +2161,20 @@ def frame(declared: int, buflen: int, type_: int = 0x00) -> bytes: with self.assertRaises(ProtocolError): HTTPv2(io.BytesIO(raw), len(raw)) - # The boundary itself: declared exactly equal to the buffer is fine - # (covered by other tests too), one octet past it is not. - exact = frame(9, 9) + # The boundary itself: a declared payload that exactly fills the buffer + # after the header is fine (covered by other tests too), one octet past + # it is not. + exact = frame(0, 9) HTTPv2(io.BytesIO(exact), len(exact)) # must not raise - one_over = frame(10, 9) + one_over = frame(1, 9) with self.assertRaises(ProtocolError): HTTPv2(io.BytesIO(one_over), len(one_over)) def test_httpv2_boundary_buffer_exactly_nine_octets(self) -> None: """The boundary itself: a buffer of exactly nine octets must parse. - Nine is the header's own size, so a ``DATA`` frame declaring nine (an + Nine is the header's own size, so a ``DATA`` frame declaring zero (an empty payload) backed by exactly nine real octets is the smallest legitimate HTTP/2 frame there is. One octet short of that must still be refused -- pinning that the fix's ``>= 9`` is not an off-by-one ``> 9``. @@ -2198,7 +2188,7 @@ def test_httpv2_boundary_buffer_exactly_nine_octets(self) -> None: exactly_nine = http2_frame_bytes(0x00, 0x00, 1, b'') self.assertEqual(len(exactly_nine), 9) info = HTTPv2(io.BytesIO(exactly_nine), len(exactly_nine)).info - self.assertEqual(info.length, 9) + self.assertEqual(info.length, 0) self.assertEqual(info.data, b'') one_short = exactly_nine[:8] @@ -2208,8 +2198,8 @@ def test_httpv2_boundary_buffer_exactly_nine_octets(self) -> None: def test_httpv2_well_formed_frame_still_parses(self) -> None: """The fix must not refuse a frame whose buffer backs its declared length. - A ``DATA`` frame declaring 21 (nine octets of header plus twelve of - payload) backed by all 21 real octets is exactly the well-formed case + A ``DATA`` frame declaring 12 (its payload, the nine header octets + excluded) backed by all 21 real octets is exactly the well-formed case the guard must keep accepting. """ @@ -2221,7 +2211,7 @@ def test_httpv2_well_formed_frame_still_parses(self) -> None: raw = http2_frame_bytes(0x00, 0x00, 1, payload) info = HTTPv2(io.BytesIO(raw), len(raw)).info - self.assertEqual(info.length, len(raw)) + self.assertEqual(info.length, len(payload)) self.assertEqual(info.data, payload) def test_httpv2_unpack_rejects_a_short_buffer_before_the_schema_layer_crashes(self) -> None: @@ -2450,13 +2440,13 @@ def test_httpv2_frame_constructors_cover_all_frame_types_and_branches(self) -> N self.assertEqual(made_from_dict.type, Frame.HEADERS) self.assertEqual(made_from_dict.flags['bit_2'], 1) made_from_bytes = proto.make(type=Frame.DATA, sid=1, frame=b'raw') - self.assertEqual(made_from_bytes.length, 12) + self.assertEqual(made_from_bytes.length, 3) self.assertEqual(made_from_bytes.frame, b'raw') made_from_schema = proto.make(type=Frame.DATA, sid=1, frame=schema_httpv2.UnassignedFrame(data=b'schema')) - self.assertEqual(made_from_schema.length, 15) + self.assertEqual(made_from_schema.length, 6) ping_data = PingFrame( - length=17, + length=8, type=Frame.PING, flags=PingFrameFlags(ACK=True), sid=0, @@ -2560,15 +2550,15 @@ def make_frame(frame=None, *, data=b''): HTTPv2.register_frame(custom, (read_frame, make_frame)) warn.assert_not_called() - proto.__header__ = http2_header(9, custom, sid=3) + proto.__header__ = http2_header(4, custom, sid=3) proto.__header__.frame = SimpleNamespace(data=b'read') - parsed = proto.read(length=9) + parsed = proto.read(length=13) self.assertEqual(parsed.data, b'read') self.assertEqual(parsed.sid, 3) made = proto.make(type=custom, sid=3, frame={'data': b'made'}) self.assertEqual(made.frame.data, b'made') - self.assertEqual(made.length, 13) + self.assertEqual(made.length, 4) finally: if original is None: frame_registry.pop(custom, None) diff --git a/tests/protocols/application/test_httpv2_frame_length_1121_unit.py b/tests/protocols/application/test_httpv2_frame_length_1121_unit.py new file mode 100644 index 0000000000..22e15f37a6 --- /dev/null +++ b/tests/protocols/application/test_httpv2_frame_length_1121_unit.py @@ -0,0 +1,121 @@ +# -*- coding: utf-8 -*- +"""HTTP/2 frame Length counts the payload only, per :rfc:`9113#section-4.1`. + +GitHub issue #1121. ``HTTP.make`` wrote Length as payload + 9 and the readers +checked the same, so every RFC-conformant frame with a fixed-size payload was +rejected, and ``_read_http_priority`` checked ``!= 9`` and so rejected even the +14 that ``make`` wrote for it. + +The fixtures below are real HTTP/2 octets, not a re-statement of pcapkit's own +arithmetic: each one is byte-identical to what scapy's independent +:mod:`scapy.contrib.http2` packs for the same frame (checked when this module +was written; scapy is not a test dependency, so it is not imported here). + +""" + +from __future__ import annotations + +import importlib.util +import io +import unittest + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: ``name -> (wire octets, make() keyword arguments)``. Length is the first +#: three octets and counts the payload alone. +RFC_FRAMES = { + 'PRIORITY': ( + bytes.fromhex('000005' '02' '00' '00000003' '00000001' '0f'), + {'type': 0x02, 'sid': 3, 'frame': {'sid_dep': 1, 'weight': 16}}, + ), + 'RST_STREAM': ( + bytes.fromhex('000004' '03' '00' '00000003' '00000008'), + {'type': 0x03, 'sid': 3, 'frame': {'error': 8}}, + ), + 'SETTINGS': ( + bytes.fromhex('000006' '04' '00' '00000000' '0001' '00001000'), + {'type': 0x04, 'sid': 0, 'frame': {'settings': [(1, 4096)]}}, + ), + 'SETTINGS/ACK': ( + bytes.fromhex('000000' '04' '01' '00000000'), + {'type': 0x04, 'sid': 0, 'frame': {'ack': True, 'settings': []}}, + ), + 'PING': ( + bytes.fromhex('000008' '06' '00' '00000000' '0102030405060708'), + {'type': 0x06, 'sid': 0, 'frame': {'opaque_data': bytes.fromhex('0102030405060708')}}, + ), + 'GOAWAY': ( + bytes.fromhex('00000a' '07' '00' '00000000' '00000005' '00000000' 'dead'), + {'type': 0x07, 'sid': 0, 'frame': {'last_sid': 5, 'error': 0, 'debug_data': b'\xde\xad'}}, + ), + 'WINDOW_UPDATE': ( + bytes.fromhex('000004' '08' '00' '00000001' '000003e8'), + {'type': 0x08, 'sid': 1, 'frame': {'incr': 1000}}, + ), + 'DATA': ( + bytes.fromhex('000005' '00' '00' '00000001') + b'hello', + {'type': 0x00, 'sid': 1, 'frame': {'data': b'hello'}}, + ), + 'PUSH_PROMISE': ( + bytes.fromhex('000004' '05' '04' '00000001' '00000002'), + {'type': 0x05, 'sid': 1, 'frame': {'end_headers': True, 'promised_sid': 2}}, + ), +} + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class HTTPv2FrameLengthUnitTests(unittest.TestCase): + """Read and write both treat Length as the payload length.""" + + def test_rfc_frames_parse_and_report_the_payload_length(self) -> None: + """Each RFC frame parses, and ``info.length`` is its payload length.""" + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + + for name, (raw, _) in RFC_FRAMES.items(): + with self.subTest(frame=name): + info = HTTPv2(io.BytesIO(raw), len(raw)).info + self.assertEqual(info.length, len(raw) - 9) + + def test_make_writes_the_rfc_octets(self) -> None: + """``make`` packs exactly the RFC octets, Length included.""" + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + + for name, (raw, kwargs) in RFC_FRAMES.items(): + with self.subTest(frame=name): + self.assertEqual(bytes(HTTPv2(**kwargs)), raw) + + def test_parsed_frames_rebuild_to_the_same_octets(self) -> None: + """parse -> ``make`` reproduces the wire octets.""" + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + + for name, (raw, _) in RFC_FRAMES.items(): + with self.subTest(frame=name): + info = HTTPv2(io.BytesIO(raw), len(raw)).info + self.assertEqual(bytes(HTTPv2(type=info.type, sid=info.sid, frame=info)), raw) + + def test_the_old_header_inclusive_length_is_rejected(self) -> None: + """A fixed-size frame declaring payload + 9 is now a size error.""" + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.utilities.exceptions import ProtocolError + + for name in ('PRIORITY', 'RST_STREAM', 'PING', 'WINDOW_UPDATE'): + raw, _ = RFC_FRAMES[name] + # Declare payload + 9, and pad the buffer so it still backs that. + wrong = (len(raw)).to_bytes(3, 'big') + raw[3:] + b'\x00' * 9 + with self.subTest(frame=name): + with self.assertRaises(ProtocolError): + HTTPv2(io.BytesIO(wrong), len(wrong)) + + def test_declared_payload_must_fit_after_the_header(self) -> None: + """The buffer has to back the 9-octet header plus the declared payload.""" + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.utilities.exceptions import ProtocolError + + raw, _ = RFC_FRAMES['DATA'] + with self.assertRaises(ProtocolError): + HTTPv2(io.BytesIO(raw[:-1]), len(raw) - 1) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/application/test_httpv2_negative_length_unit.py b/tests/protocols/application/test_httpv2_negative_length_unit.py index cfa2825217..55b1017af8 100644 --- a/tests/protocols/application/test_httpv2_negative_length_unit.py +++ b/tests/protocols/application/test_httpv2_negative_length_unit.py @@ -17,7 +17,7 @@ :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 +``read()``'s own ``schema.length + 9 > 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 @@ -47,12 +47,12 @@ def http2_frame_bytes(type_: 'int', flags: 'int', sid: 'int', payload: 'bytes') 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. + The packed frame, its 3-octet length field counting the payload only, + the 9-octet header excluded [:rfc:`9113#section-4.1`]. """ return ( - (len(payload) + 9).to_bytes(3, 'big') + len(payload).to_bytes(3, 'big') + bytes([type_, flags]) + sid.to_bytes(4, 'big') + payload diff --git a/tests/protocols/application/test_httpv2_payload_length_unit.py b/tests/protocols/application/test_httpv2_payload_length_unit.py index 19ae9240cb..a4a039f3fc 100644 --- a/tests/protocols/application/test_httpv2_payload_length_unit.py +++ b/tests/protocols/application/test_httpv2_payload_length_unit.py @@ -170,10 +170,8 @@ def http2_frame_bytes(type_: 'int', flags: 'int', sid: 'int', payload: 'bytes') -> 'bytes': """Build the wire octets of one HTTP/2 frame. - The length field counts the *whole* frame, header included -- this library's - convention rather than :rfc:`9113#section-4.1`'s, which counts the payload - alone. ``HTTP.make`` writes ``payload + 9`` and the readers recover the - payload as ``length - 9``. + The length field counts the frame payload only, the 9-octet header + excluded [:rfc:`9113#section-4.1`]. Duplicated from :func:`tests.protocols.application.test_http_unit.http2_frame_bytes` rather @@ -193,7 +191,7 @@ def http2_frame_bytes(type_: 'int', flags: 'int', sid: 'int', payload: 'bytes') """ return ( - (len(payload) + 9).to_bytes(3, 'big') + len(payload).to_bytes(3, 'big') + bytes([type_, flags]) + sid.to_bytes(4, 'big') + payload @@ -728,9 +726,9 @@ def assertDeclaresWhatItWrites(self, raw: 'bytes', payload: 'bytes') -> 'None': """ self.assertEqual( - int.from_bytes(raw[:3], 'big'), len(raw), - 'the declared length must match the octets actually written -- this ' - 'library counts the whole frame, header included' + int.from_bytes(raw[:3], 'big'), len(raw) - 9, + 'the declared length must match the payload octets actually written ' + '-- the 9-octet header excluded, per RFC 9113 section 4.1' ) self.assertIn(payload, raw, 'the payload octets must reach the wire') @@ -762,9 +760,9 @@ def test_an_unpadded_continuation_frame_is_the_control(self) -> None: """``CONTINUATION`` packed its fragment correctly pre-fix as well. The plain-form control for the construct path, matching - :class:`HTTPv2PlainLengthFormControlUnitTests` on the parse path: it - declared 29 and wrote 29 throughout, which is what shows the construct - machinery was never the problem either. + :class:`HTTPv2PlainLengthFormControlUnitTests` on the parse path: its + length field matched what it wrote throughout, which is what shows the + construct machinery was never the problem either. """ raw = self.build(type=0x09, sid=1, frame={'fragment': FRAGMENT}) diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index ad9f6dcc0c..1e9b89cb10 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -446,15 +446,10 @@ class Gap(NamedTuple): # ``item_type=SettingPair`` (the raw schema class) instead of # ``SchemaField(schema=SettingPair)``. Filed as #459, fixed and merged # as #462. - - # ``make`` writes ``length = payload + 9`` and a PRIORITY payload is five - # octets, so the constructed header always says 14 -- while the reader - # demands exactly 9. Its siblings all check payload+9 (RST_STREAM 13, - # WINDOW_UPDATE 13, PING 17), so 9 looks like the outlier. - 'httpv2-frame/PRIORITY': Gap( - 'CONSTRUCT', 'HTTP/2: [Type 2] invalid format', - 'pcapkit/protocols/application/httpv2.py:572 -- reads length != 9 for a ' - 'frame make() always builds with length 14'), + # + # PRIORITY needs no entry either: ``make`` declared payload + 9 while the + # reader demanded 9, until #1121 moved both to RFC 9113's payload-only + # Length. # -- PCAP-NG -------------------------------------------------------------- diff --git a/tests/protocols/transport/test_tcp_http_dispatch_unit.py b/tests/protocols/transport/test_tcp_http_dispatch_unit.py index 23b372a2a6..9af2235b82 100644 --- a/tests/protocols/transport/test_tcp_http_dispatch_unit.py +++ b/tests/protocols/transport/test_tcp_http_dispatch_unit.py @@ -14,9 +14,8 @@ segments below are built in memory, and the HTTP/2 payload is the *verbatim* nine octets that :file:`examples/captures/options-transport.pcap` frame 34 carries -- a fixture this library's own ``httpv2.HTTP.make`` produced -- rather -than a hand-rolled frame, because HTTP/2's declared length is whole-frame here -and a synthetic frame written to the payload-only convention would fail for a -reason that has nothing to do with dispatch. +than a hand-rolled frame, so the frame is whatever the library itself writes and +cannot fail for a reason that has nothing to do with dispatch. """ @@ -36,9 +35,10 @@ #: The same, to port 8080. _TCP_TO_8080 = bytes.fromhex('c3501f9000000001000000015010ffff00000000') -#: A nine-octet HTTP/2 ``DATA`` frame: declared length 9, type 0, no flags, -#: stream 0. Copied verbatim from ``options-transport.pcap`` frame 34. -_HTTP2_FRAME = bytes.fromhex('000009000000000000') +#: A nine-octet HTTP/2 ``DATA`` frame: declared length 0 (the payload, the +#: header excluded), type 0, no flags, stream 0. Copied verbatim from +#: ``options-transport.pcap`` frame 34. +_HTTP2_FRAME = bytes.fromhex('000000000000000000') #: A minimal HTTP/1.1 request, which must keep decoding as HTTP/1.1. _HTTP1_REQUEST = b'GET / HTTP/1.1\r\nHost: example.invalid\r\n\r\n'