From: Illia Volochii Date: Tue, 17 Oct 2023 19:35:39 +0300 Subject: Merge pull request from GHSA-g4mx-q9vg-27p4 Origin: https://github.com/urllib3/urllib3/commit/b594c5ceaca38e1ac215f916538fb128e3526a36 Bug: https://github.com/urllib3/urllib3/security/advisories/GHSA-g4mx-q9vg-27p4 Bug-Debian: https://security-tracker.debian.org/tracker/CVE-2023-45803 Bug-Debian: https://bugs.debian.org/1054226 --- dummyserver/handlers.py | 7 +++++++ src/urllib3/_collections.py | 18 ++++++++++++++++++ src/urllib3/connectionpool.py | 5 +++++ src/urllib3/poolmanager.py | 7 +++++-- test/with_dummyserver/test_connectionpool.py | 11 +++++++++++ test/with_dummyserver/test_poolmanager.py | 15 +++++++++++++++ 6 files changed, 61 insertions(+), 2 deletions(-) diff --git a/dummyserver/handlers.py b/dummyserver/handlers.py index f8bdf25..7d3e413 100644 --- a/dummyserver/handlers.py +++ b/dummyserver/handlers.py @@ -186,6 +186,8 @@ class TestingApp(RequestHandler): status = request.params.get("status", "303 See Other") if len(status) == 3: status = "%s Redirect" % status.decode("latin-1") + elif isinstance(status, bytes): + status = status.decode("latin-1") headers = [("Location", target)] return Response(status=status, headers=headers) @@ -264,6 +266,11 @@ class TestingApp(RequestHandler): def headers(self, request): return Response(json.dumps(dict(request.headers))) + def headers_and_params(self, request): + return Response( + json.dumps({"headers": dict(request.headers), "params": request.params}) + ) + def successful_retry(self, request): """Handler which will return an error and then success diff --git a/src/urllib3/_collections.py b/src/urllib3/_collections.py index 527c1fe..6bc6e2c 100644 --- a/src/urllib3/_collections.py +++ b/src/urllib3/_collections.py @@ -268,6 +268,24 @@ class HTTPHeaderDict(MutableMapping): else: return vals[1:] + def _prepare_for_method_change(self): + """ + Remove content-specific header fields before changing the request + method to GET or HEAD according to RFC 9110, Section 15.4. + """ + content_specific_headers = [ + "Content-Encoding", + "Content-Language", + "Content-Location", + "Content-Type", + "Content-Length", + "Digest", + "Last-Modified", + ] + for header in content_specific_headers: + self.discard(header) + return self + # Backwards compatibility for httplib getheaders = getlist getallmatchingheaders = getlist diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index 2d50f3f..06aa8b8 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -12,6 +12,7 @@ from socket import timeout as SocketTimeout import six from six.moves import queue +from ._collections import HTTPHeaderDict from .connection import ( BaseSSLError, BrokenPipeError, @@ -833,7 +834,11 @@ class HTTPConnectionPool(ConnectionPool, RequestMethods): redirect_location = redirect and response.get_redirect_location() if redirect_location: if response.status == 303: + # Change the method according to RFC 9110, Section 15.4.4. method = "GET" + # And lose the body not to transfer anything sensitive. + body = None + headers = HTTPHeaderDict(headers)._prepare_for_method_change() try: retries = retries.increment(method, url, response=response, _pool=self) diff --git a/src/urllib3/poolmanager.py b/src/urllib3/poolmanager.py index ca305ec..87467ad 100644 --- a/src/urllib3/poolmanager.py +++ b/src/urllib3/poolmanager.py @@ -4,7 +4,7 @@ import collections import functools import logging -from ._collections import RecentlyUsedContainer +from ._collections import HTTPHeaderDict, RecentlyUsedContainer from .connectionpool import HTTPConnectionPool, HTTPSConnectionPool, port_by_scheme from .exceptions import ( LocationValueError, @@ -382,9 +382,12 @@ class PoolManager(RequestMethods): # Support relative URLs for redirecting. redirect_location = urljoin(url, redirect_location) - # RFC 7231, Section 6.4.4 if response.status == 303: + # Change the method according to RFC 9110, Section 15.4.4. method = "GET" + # And lose the body not to transfer anything sensitive. + kw["body"] = None + kw["headers"] = HTTPHeaderDict(kw["headers"])._prepare_for_method_change() retries = kw.get("retries") if not isinstance(retries, Retry): diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 2177c24..7fc0787 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -411,6 +411,17 @@ class TestConnectionPool(HTTPDummyServerTestCase): assert r.status == 200 assert r.data == b"Dummy server!" + def test_303_redirect_makes_request_lose_body(self): + with HTTPConnectionPool(self.host, self.port) as pool: + response = pool.request( + "POST", + "/redirect", + fields={"target": "/headers_and_params", "status": "303 See Other"}, + ) + data = json.loads(response.data) + assert data["params"] == {} + assert "Content-Type" not in HTTPHeaderDict(data["headers"]) + def test_bad_connect(self): with HTTPConnectionPool("badhost.invalid", self.port) as pool: with pytest.raises(MaxRetryError) as e: diff --git a/test/with_dummyserver/test_poolmanager.py b/test/with_dummyserver/test_poolmanager.py index 02a3811..509daf2 100644 --- a/test/with_dummyserver/test_poolmanager.py +++ b/test/with_dummyserver/test_poolmanager.py @@ -5,6 +5,7 @@ import pytest from dummyserver.server import HAS_IPV6 from dummyserver.testcase import HTTPDummyServerTestCase, IPv6HTTPDummyServerTestCase +from urllib3._collections import HTTPHeaderDict from urllib3.connectionpool import port_by_scheme from urllib3.exceptions import MaxRetryError, URLSchemeUnknown from urllib3.poolmanager import PoolManager @@ -236,6 +237,20 @@ class TestPoolManager(HTTPDummyServerTestCase): assert r._pool.num_connections == 1 assert len(http.pools) == 1 + def test_303_redirect_makes_request_lose_body(self): + with PoolManager() as http: + response = http.request( + "POST", + "%s/redirect" % self.base_url, + fields={ + "target": "%s/headers_and_params" % self.base_url, + "status": "303 See Other", + }, + ) + data = json.loads(response.data) + assert data["params"] == {} + assert "Content-Type" not in HTTPHeaderDict(data["headers"]) + def test_unknown_scheme(self): with PoolManager() as http: unknown_scheme = "unknown"