From 288cf469ba800a071fcddca61434d6ab9dcf689d Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 8 Aug 2026 22:10:51 +0100 Subject: [PATCH] Limit number of queued WS fragments (#13352) --- CHANGES/13352.bugfix.rst | 1 + THREAT_MODEL.md | 9 ++++++- aiohttp/_websocket/reader_c.pxd | 1 + aiohttp/_websocket/reader_py.py | 9 +++++++ tests/test_websocket_parser.py | 44 +++++++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 CHANGES/13352.bugfix.rst diff --git a/CHANGES/13352.bugfix.rst b/CHANGES/13352.bugfix.rst new file mode 100644 index 00000000000..a10a0d7767d --- /dev/null +++ b/CHANGES/13352.bugfix.rst @@ -0,0 +1 @@ +Capped the number of per-read payload fragments the WebSocket reader retains in memory -- by :user:`Dreamsorcerer`. diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 422cb7a2993..5ad9d210368 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -548,7 +548,7 @@ client-side, the writer adds masks to outgoing frames. | 3.3 | RSV bits | `reader_py.py:WebSocketReader._feed_data` gates RSV1 on the PMCE-negotiated `_compress` flag; RSV2/3 always rejected. | None. | | 3.4 | Unknown opcode | Rejected. | None. | | 3.5โ€“3.7 | Control-frame and fragmentation rules | All enforced at reader. | None. | -| 3.8 | Fragment memory bound | `max_msg_size` enforced pre-FIN and at assembly. Default 4 MiB. | **User**: set a smaller `max_msg_size` for protocols where messages are bounded (e.g. chat); the 4 MiB default suits arbitrary payloads. | +| 3.8 | Fragment memory bound | (a) Declared byte size โ€” `max_msg_size` enforced pre-FIN and at assembly (default 4 MiB). (b) `WebSocketReader.__init__` caps the total fragment count at `max(1024, max_msg_size // 256)` and pauses reading for backpressure once exceeded. | **User**: set a smaller `max_msg_size` for protocols where messages are bounded (e.g. chat); the 4 MiB default suits arbitrary payloads. | | 3.9 | PMCE decompression bomb | `WebSocketReader._handle_frame` decompresses with a `max_length` of `max_msg_size + 1` and checks the result; on overflow, raises `MESSAGE_TOO_BIG` (1009). This `max_length` post-decompress check was introduced by PR #11898 (v3.13.3). | **Documented known limitation.** Some backends (notably `isal_zlib`) do not strictly honour `max_length` in `decompress()` and may overshoot by up to one zlib block before the post-decompress size check fires. The post-check still catches it before the bytes reach the application, but a transient over-allocation is possible. Document and monitor. | | 3.10 | PMCE context retention | Default extensions request context takeover (per RFC 7692 default); user can negotiate `server_no_context_takeover` / `client_no_context_takeover` via handshake. | Documented design decision: keep the RFC 7692 default (context takeover). **Document the memory tradeoff in user-facing WebSocket docs.** **User**: configure no-context-takeover on long-lived sessions running on memory-constrained hosts. | | 3.11 | UTF-8 validation | Strict `bytes.decode("utf-8")` post-assembly. | None. | @@ -575,6 +575,13 @@ client-side, the writer adds masks to outgoing frames. RFC 6455 ยง5.2 requires failing such frames). Fixed by passing `compress=bool(compress)` in `client.py:_ws_connect` and removing the `compress` / `decode_text` defaults on `WebSocketReader.__init__`. +- **PR #13350** (follow-up to CVE-2026-54274) โ€” the per-frame + `max_msg_size` byte cap still let a size-legal frame dribbled in tiny + transport reads pin ~28x its on-wire size in per-read `bytes` objects + (`_payload_fragments`). `WebSocketReader` now caps the retained + fragment count (`max(1024, max_msg_size // 256)`) and pauses reading for + backpressure once exceeded, mirroring the HTTP chunk-splits limit in + `StreamReader` (PR #11894). --- diff --git a/aiohttp/_websocket/reader_c.pxd b/aiohttp/_websocket/reader_c.pxd index 7e5e46f13c7..94c8f18ef2d 100644 --- a/aiohttp/_websocket/reader_c.pxd +++ b/aiohttp/_websocket/reader_c.pxd @@ -77,6 +77,7 @@ cdef class WebSocketReader: cdef bint _frame_fin cdef int _frame_opcode cdef list _payload_fragments + cdef Py_ssize_t _max_fragments cdef Py_ssize_t _frame_payload_len cdef bytes _tail diff --git a/aiohttp/_websocket/reader_py.py b/aiohttp/_websocket/reader_py.py index 6e2b99ff491..b84dfac5298 100644 --- a/aiohttp/_websocket/reader_py.py +++ b/aiohttp/_websocket/reader_py.py @@ -158,6 +158,9 @@ def __init__( self._frame_fin = False self._frame_opcode: int = OP_CODE_NOT_SET self._payload_fragments: list[bytes] = [] + # Limit number of fragments, so a large number of tiny fragments + # doesn't exceed reasonable memory usage. + self._max_fragments = max(1024, max_msg_size // 256) if max_msg_size else 0 self._frame_payload_len = 0 self._tail: bytes = b"" @@ -493,6 +496,12 @@ def _feed_data(self, data: bytes) -> None: # If we don't have a complete frame, we need to save the # data for the next call to feed_data. self._payload_fragments.append(data_cstr[f_start_pos:f_end_pos]) + if ( + self._max_fragments + and len(self._payload_fragments) > self._max_fragments + and not self.queue._protocol._reading_paused + ): + self.queue._protocol.pause_reading() break payload: bytes | bytearray diff --git a/tests/test_websocket_parser.py b/tests/test_websocket_parser.py index 8045cf7956a..10cec0cc662 100644 --- a/tests/test_websocket_parser.py +++ b/tests/test_websocket_parser.py @@ -816,3 +816,47 @@ def test_flow_control_multi_byte_text( data=large_payload_text, size=large_payload_size, extra="" ) assert protocol._reading_paused is True + + +async def test_incomplete_frame_pauses_when_fragment_limit_exceeded( + protocol: BaseProtocol, +) -> None: + max_msg_size = 64 * 1024 + loop = asyncio.get_running_loop() + out = WebSocketDataQueue(protocol, 2**16, loop=loop) + parser = WebSocketReader(out, max_msg_size, compress=False, decode_text=False) + + payload_len = 32 * 1024 + parser.feed_data(PACK_LEN2(0x80 | WSMsgType.BINARY, 126, payload_len)) + assert protocol._reading_paused is False + + # Feed the payload two bytes per read so the pause is + # driven purely by the fragment count, not the total byte count. + paused_after = None + for i in range(payload_len // 2 - 1): # pragma: no branch + parser.feed_data(b"xx") + if protocol._reading_paused: + paused_after = i + 1 # type: ignore[unreachable] + break + + assert paused_after is not None + # Paused long before the frame could complete (16384 two-byte reads). + assert paused_after < payload_len // 2 # type: ignore[unreachable] + + +async def test_incomplete_frame_not_paused_for_normal_reads( + protocol: BaseProtocol, +) -> None: + max_msg_size = 64 * 1024 + loop = asyncio.get_running_loop() + out = WebSocketDataQueue(protocol, 2**16, loop=loop) + parser = WebSocketReader(out, max_msg_size, compress=False, decode_text=False) + + # Normal traffic (a frame delivered in a handful of reasonably sized reads) + # must never trip the fragment-limit backpressure. + payload_len = 32 * 1024 + parser.feed_data(PACK_LEN2(0x80 | WSMsgType.BINARY, 126, payload_len)) + # 32 KiB in 4 KiB reads -> 8 fragments, far below the cap. + for _ in range(payload_len // 4096): + parser.feed_data(b"x" * 4096) + assert protocol._reading_paused is False