163 lines
6.6 KiB
Diff
163 lines
6.6 KiB
Diff
From 2bdcc44d1e163fb5cc48a8662425e35e15adfe6a Mon Sep 17 00:00:00 2001
|
|
From: Illia Volochii <illia.volochii@gmail.com>
|
|
Date: Thu, 7 May 2026 18:39:03 +0300
|
|
Subject: [PATCH] Merge commit from fork
|
|
|
|
* Avoid any decoding in `HTTPResponse.drain_conn`
|
|
|
|
* Add a comment
|
|
|
|
* Simplify `drain_conn`
|
|
|
|
* Add tests
|
|
|
|
* Add additional checks to the test
|
|
|
|
* Fix full decompression on the 2nd small read from response using Brotli
|
|
|
|
* Add a changelog entry
|
|
|
|
* Inverse the order in the changelog entry
|
|
|
|
* Mention `stream` call
|
|
---
|
|
changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst | 7 +++++++
|
|
src/urllib3/response.py | 17 +++++++++++------
|
|
test/test_response.py | 24 +++++++++++++++++++++---
|
|
test/with_dummyserver/test_connection.py | 19 +++++++++++++++++++
|
|
4 files changed, 58 insertions(+), 9 deletions(-)
|
|
create mode 100644 changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst
|
|
|
|
diff --git a/changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst b/changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst
|
|
new file mode 100644
|
|
index 0000000000..ac70af825a
|
|
--- /dev/null
|
|
+++ b/changelog/GHSA-mf9v-mfxr-j63j.bugfix.rst
|
|
@@ -0,0 +1,7 @@
|
|
+Fixed two high-severity security issues where decompression-bomb safeguards of the streaming API were bypassed:
|
|
+
|
|
+
|
|
+1. When ``HTTPResponse.drain_conn()`` was called after the response had been read and decompressed partially.
|
|
+2. During the second ``HTTPResponse.read(amt=N)`` or ``HTTPResponse.stream(amt=N)`` call when the response was decompressed using the official `Brotli <https://pypi.org/project/brotli/>`__ library.
|
|
+
|
|
+See `GHSA-mf9v-mfxr-j63j <https://github.com/urllib3/urllib3/security/advisories/GHSA-mf9v-mfxr-j63j>`__ for details.
|
|
diff --git a/src/urllib3/response.py b/src/urllib3/response.py
|
|
index 521c31b282..e9246b75e3 100644
|
|
--- a/src/urllib3/response.py
|
|
+++ b/src/urllib3/response.py
|
|
@@ -798,13 +798,14 @@ def drain_conn(self) -> None:
|
|
Unread data in the HTTPResponse connection blocks the connection from being released back to the pool.
|
|
"""
|
|
try:
|
|
- self.read(
|
|
- # Do not spend resources decoding the content unless
|
|
- # decoding has already been initiated.
|
|
- decode_content=self._has_decoded_content,
|
|
- )
|
|
+ self._raw_read()
|
|
except (HTTPError, OSError, BaseSSLError, HTTPException):
|
|
pass
|
|
+ if self._has_decoded_content:
|
|
+ # `_raw_read` skips decompression, so we should clean up the
|
|
+ # decoder to avoid keeping unnecessary data in memory.
|
|
+ self._decoded_buffer = BytesQueueBuffer()
|
|
+ self._decoder = None
|
|
|
|
@property
|
|
def data(self) -> bytes:
|
|
@@ -1091,7 +1092,11 @@ def read(
|
|
elif amt is not None:
|
|
cache_content = False
|
|
|
|
- if self._decoder and self._decoder.has_unconsumed_tail:
|
|
+ if (
|
|
+ self._decoder
|
|
+ and self._decoder.has_unconsumed_tail
|
|
+ and len(self._decoded_buffer) < amt
|
|
+ ):
|
|
decoded_data = self._decode(
|
|
b"",
|
|
decode_content,
|
|
diff --git a/test/test_response.py b/test/test_response.py
|
|
index c70262c04e..da521137bf 100644
|
|
--- a/test/test_response.py
|
|
+++ b/test/test_response.py
|
|
@@ -858,22 +858,33 @@ def test_memory_usage_decode_with_max_length(
|
|
pytest.skip(f"Proper {request.node.callspec.id} decoder is not available")
|
|
|
|
name, compressed_data = data
|
|
- limit = 1024 * 1024 # 1 MiB
|
|
+ limit1 = 1024 * 1024 # 1 MiB
|
|
+ # We test with two read calls because the second call may be
|
|
+ # able to use the internal buffer filled by the first call, and
|
|
+ # we want to ensure that full decompression is never triggered
|
|
+ # by the second call. The limit for the second call is lowered
|
|
+ # to make sure that the internal buffer is used for the Brotli
|
|
+ # case specifically https://github.com/google/brotli/issues/1396
|
|
+ limit2 = 1024 # 1 KiB
|
|
if read_method in ("read_chunked", "stream"):
|
|
httplib_r = httplib.HTTPResponse(MockSock) # type: ignore[arg-type]
|
|
+ httplib_r.chunked = True
|
|
+ httplib_r.chunk_left = 1
|
|
httplib_r.fp = MockChunkedEncodingResponse([compressed_data]) # type: ignore[assignment]
|
|
r = HTTPResponse(
|
|
httplib_r,
|
|
preload_content=False,
|
|
headers={"transfer-encoding": "chunked", "content-encoding": name},
|
|
)
|
|
- next(getattr(r, read_method)(amt=limit, decode_content=True))
|
|
+ for limit in (limit1, limit2):
|
|
+ next(getattr(r, read_method)(amt=limit, decode_content=True))
|
|
else:
|
|
fp = BytesIO(compressed_data)
|
|
r = HTTPResponse(
|
|
fp, headers={"content-encoding": name}, preload_content=False
|
|
)
|
|
- getattr(r, read_method)(amt=limit, decode_content=True)
|
|
+ for limit in (limit1, limit2):
|
|
+ getattr(r, read_method)(amt=limit, decode_content=True)
|
|
|
|
# Check that the internal decoded buffer is empty unless brotli
|
|
# is used.
|
|
@@ -883,6 +894,13 @@ def test_memory_usage_decode_with_max_length(
|
|
if name != "br" or brotli.__name__ == "brotlicffi":
|
|
assert len(r._decoded_buffer) == 0
|
|
|
|
+ # Check that memory usage is still within the limit while the
|
|
+ # connection is being drained, meaning that the call does not
|
|
+ # decompress the whole content.
|
|
+ r.drain_conn()
|
|
+ assert r._decoder is None
|
|
+ assert len(r._decoded_buffer) == 0
|
|
+
|
|
def test_multi_decoding_deflate_deflate(self) -> None:
|
|
data = zlib.compress(zlib.compress(b"foo"))
|
|
|
|
diff --git a/test/with_dummyserver/test_connection.py b/test/with_dummyserver/test_connection.py
|
|
index b9c547c00f..940815dac2 100644
|
|
--- a/test/with_dummyserver/test_connection.py
|
|
+++ b/test/with_dummyserver/test_connection.py
|
|
@@ -138,3 +138,22 @@ def test_invalid_tunnel_scheme(pool: HTTPConnectionPool) -> None:
|
|
str(e.value)
|
|
== "Invalid proxy scheme for tunneling: 'socks', must be either 'http' or 'https'"
|
|
)
|
|
+
|
|
+
|
|
+def test_response_after_drain_conn(pool: HTTPConnectionPool) -> None:
|
|
+ """
|
|
+ Test that a connection can be reused after calling `drain_conn` on
|
|
+ an unread response.
|
|
+ """
|
|
+ conn = pool._get_conn()
|
|
+
|
|
+ conn.request("GET", "/", preload_content=False)
|
|
+ response = conn.getresponse()
|
|
+ assert response.status == 200
|
|
+ response.drain_conn()
|
|
+
|
|
+ conn.request("GET", "/", preload_content=False)
|
|
+ response = conn.getresponse()
|
|
+ assert response.status == 200
|
|
+
|
|
+ conn.close()
|