Merge pull request #116568 from mweinelt/20.09/python/aiohttp/CVE-2021-21330

This commit is contained in:
Martin Weinelt
2021-03-23 02:38:19 +01:00
committed by GitHub
3 changed files with 484 additions and 4 deletions

View File

@@ -34,8 +34,8 @@ in python.pkgs.buildPythonPackage {
};
postPatch = ''
# yarl 1.4+ only requires Python 3.6+
sed -iE "s/yarl==1.3.0//" requirements.txt
substituteInPlace requirements.txt --replace \
"aiohttp==3.6.2" "aiohttp>=3.6.2"
'';
propagatedBuildInputs = with python.pkgs; [

View File

@@ -0,0 +1,475 @@
From a35f0dfe794fb0440c161d6acc072c07e53596a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Wed, 10 Feb 2021 04:12:46 +0000
Subject: [PATCH 01/15] Add test reproducing open redirects with
normalize_path_middleware().
---
tests/test_web_middleware.py | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index 2eba995d..11e646c2 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -275,6 +275,19 @@ class TestNormalizePathMiddleware:
with pytest.raises(AssertionError):
web.normalize_path_middleware(append_slash=True, remove_slash=True)
+ async def test_open_redirects(
+ self, aiohttp_client
+ ) -> None:
+ async def handle(request: web.Request) -> web.StreamResponse:
+ return web.Response(text="hello")
+
+ app = web.Application(middlewares=[web.normalize_path_middleware()])
+ app.add_routes([web.get('/', handle)])
+ client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
+ resp = await client.get('//google.com', allow_redirects=False)
+ assert resp.status == 404
+ assert resp.url.query == URL('//google.com').query
+
async def test_old_style_middleware(loop, aiohttp_client) -> None:
async def handler(request):
--
2.30.1
From 920d87a0d9f6f33d5907d0661b07034f1fff4fa5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Wed, 10 Feb 2021 04:33:11 +0000
Subject: [PATCH 02/15] Prevent open redirects from normalize_path_middleware.
---
CHANGES/openredirects.bugfix | 1 +
aiohttp/web_middlewares.py | 4 ++--
tests/test_web_middleware.py | 10 ++++------
3 files changed, 7 insertions(+), 8 deletions(-)
create mode 100644 CHANGES/openredirects.bugfix
diff --git a/CHANGES/openredirects.bugfix b/CHANGES/openredirects.bugfix
new file mode 100644
index 00000000..91ac9db4
--- /dev/null
+++ b/CHANGES/openredirects.bugfix
@@ -0,0 +1 @@
+Prevent open redirects from normalize_path_middleware.
diff --git a/aiohttp/web_middlewares.py b/aiohttp/web_middlewares.py
index 2c7bf977..bf2c6e4c 100644
--- a/aiohttp/web_middlewares.py
+++ b/aiohttp/web_middlewares.py
@@ -90,9 +90,9 @@ def normalize_path_middleware(
if merge_slashes:
paths_to_check.append(re.sub('//+', '/', path))
if append_slash and not request.path.endswith('/'):
- paths_to_check.append(path + '/')
+ paths_to_check.append(re.sub("^//+", "/", path + "/"))
if remove_slash and request.path.endswith('/'):
- paths_to_check.append(path[:-1])
+ paths_to_check.append(re.sub("^//+", "/", path[:-1]))
if merge_slashes and append_slash:
paths_to_check.append(
re.sub('//+', '/', path + '/'))
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index 11e646c2..257c186e 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -275,18 +275,16 @@ class TestNormalizePathMiddleware:
with pytest.raises(AssertionError):
web.normalize_path_middleware(append_slash=True, remove_slash=True)
- async def test_open_redirects(
- self, aiohttp_client
- ) -> None:
+ async def test_open_redirects(self, aiohttp_client) -> None:
async def handle(request: web.Request) -> web.StreamResponse:
return web.Response(text="hello")
app = web.Application(middlewares=[web.normalize_path_middleware()])
- app.add_routes([web.get('/', handle)])
+ app.add_routes([web.get("/", handle)])
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
- resp = await client.get('//google.com', allow_redirects=False)
+ resp = await client.get("//google.com", allow_redirects=False)
assert resp.status == 404
- assert resp.url.query == URL('//google.com').query
+ assert resp.url.query == URL("//google.com").query
async def test_old_style_middleware(loop, aiohttp_client) -> None:
--
2.30.1
From ef120208d2e1f50a2c849caeb02e18d1ee14abcc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Wed, 10 Feb 2021 14:16:25 +0000
Subject: [PATCH 03/15] Update CHANGES/openredirects.bugfix
Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
---
CHANGES/openredirects.bugfix | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/CHANGES/openredirects.bugfix b/CHANGES/openredirects.bugfix
index 91ac9db4..c031b71d 100644
--- a/CHANGES/openredirects.bugfix
+++ b/CHANGES/openredirects.bugfix
@@ -1 +1,4 @@
-Prevent open redirects from normalize_path_middleware.
+**(SECURITY BUG)** Started preventing open redirects in the
+``aiohttp.web.normalize_path_middleware`` middleware. For
+more details, see
+https://github.com/aio-libs/aiohttp/security/advisories/GHSA-v6wp-4m6f-gcjg.
--
2.30.1
From 453537de5d39392dc72c70b42a087e447382fade Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Wed, 10 Feb 2021 14:17:06 +0000
Subject: [PATCH 04/15] Explicitly check for Location in headers.
Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
---
tests/test_web_middleware.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index 257c186e..b1e32425 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -284,6 +284,7 @@ class TestNormalizePathMiddleware:
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
resp = await client.get("//google.com", allow_redirects=False)
assert resp.status == 404
+ assert 'Location' not in resp.headers
assert resp.url.query == URL("//google.com").query
--
2.30.1
From 2ee49154ae5953b48e55e58b078e4ff7f1412c2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Thu, 18 Feb 2021 01:17:37 +0000
Subject: [PATCH 05/15] Update tests/test_web_middleware.py
Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
---
tests/test_web_middleware.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index b1e32425..4eace13e 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -277,7 +277,11 @@ class TestNormalizePathMiddleware:
async def test_open_redirects(self, aiohttp_client) -> None:
async def handle(request: web.Request) -> web.StreamResponse:
- return web.Response(text="hello")
+ pytest.fail(
+ msg="Security advisory 'GHSA-v6wp-4m6f-gcjg' test handler "
+ "matched unexpectedly",
+ pytrace=False,
+ )
app = web.Application(middlewares=[web.normalize_path_middleware()])
app.add_routes([web.get("/", handle)])
--
2.30.1
From 39957d11491bfab76526831c7b5891561aab785c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Thu, 18 Feb 2021 01:39:05 +0000
Subject: [PATCH 06/15] Use parametrize to test append_slash and remove_slash.
Strip extra trailing slashes just once.
---
aiohttp/web_middlewares.py | 5 +++--
tests/test_web_middleware.py | 14 ++++++++++++--
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/aiohttp/web_middlewares.py b/aiohttp/web_middlewares.py
index bf2c6e4c..5210b445 100644
--- a/aiohttp/web_middlewares.py
+++ b/aiohttp/web_middlewares.py
@@ -90,9 +90,9 @@ def normalize_path_middleware(
if merge_slashes:
paths_to_check.append(re.sub('//+', '/', path))
if append_slash and not request.path.endswith('/'):
- paths_to_check.append(re.sub("^//+", "/", path + "/"))
+ paths_to_check.append(path + "/")
if remove_slash and request.path.endswith('/'):
- paths_to_check.append(re.sub("^//+", "/", path[:-1]))
+ paths_to_check.append(path[:-1])
if merge_slashes and append_slash:
paths_to_check.append(
re.sub('//+', '/', path + '/'))
@@ -101,6 +101,7 @@ def normalize_path_middleware(
paths_to_check.append(merged_slashes[:-1])
for path in paths_to_check:
+ path = re.sub("^//+", "/", path)
resolves, request = await _check_request_resolves(
request, path)
if resolves:
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index 4eace13e..cd76883e 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -275,7 +275,16 @@ class TestNormalizePathMiddleware:
with pytest.raises(AssertionError):
web.normalize_path_middleware(append_slash=True, remove_slash=True)
- async def test_open_redirects(self, aiohttp_client) -> None:
+ @pytest.mark.parametrize(
+ "append_slash, remove_slash",
+ [
+ (True, False),
+ (False, True),
+ ],
+ )
+ async def test_open_redirects(
+ self, append_slash: bool, remove_slash: bool,
+ aiohttp_client) -> None:
async def handle(request: web.Request) -> web.StreamResponse:
pytest.fail(
msg="Security advisory 'GHSA-v6wp-4m6f-gcjg' test handler "
@@ -283,7 +292,8 @@ class TestNormalizePathMiddleware:
pytrace=False,
)
- app = web.Application(middlewares=[web.normalize_path_middleware()])
+ app = web.Application(middlewares=[web.normalize_path_middleware(
+ append_slash=append_slash, remove_slash=remove_slash)])
app.add_routes([web.get("/", handle)])
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
resp = await client.get("//google.com", allow_redirects=False)
--
2.30.1
From 65a83539779b3dce8b4d8a0fb2a22a7461ff63b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Thu, 18 Feb 2021 01:45:50 +0000
Subject: [PATCH 07/15] Add existing URL, but check for redirect Location.
---
tests/test_web_middleware.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index cd76883e..af62e218 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -294,11 +294,11 @@ class TestNormalizePathMiddleware:
app = web.Application(middlewares=[web.normalize_path_middleware(
append_slash=append_slash, remove_slash=remove_slash)])
- app.add_routes([web.get("/", handle)])
+ app.add_routes([web.get("/", handle), web.get("/google.com", handle)])
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
resp = await client.get("//google.com", allow_redirects=False)
- assert resp.status == 404
- assert 'Location' not in resp.headers
+ assert resp.status == 308
+ assert resp.headers.get('Location') == '/google.com'
assert resp.url.query == URL("//google.com").query
--
2.30.1
From 924517e328d6d0edf90b83b89e6ff2190aa8cbe9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jelmer=20Vernoo=C4=B3?= <jelmer@jelmer.uk>
Date: Sun, 21 Feb 2021 18:12:09 +0000
Subject: [PATCH 08/15] Credit 'Beast Glatisant' for finding the original issue
on https://janitor.kali.org/.
---
CHANGES/openredirects.bugfix | 2 ++
1 file changed, 2 insertions(+)
diff --git a/CHANGES/openredirects.bugfix b/CHANGES/openredirects.bugfix
index c031b71d..7fa556c8 100644
--- a/CHANGES/openredirects.bugfix
+++ b/CHANGES/openredirects.bugfix
@@ -2,3 +2,5 @@
``aiohttp.web.normalize_path_middleware`` middleware. For
more details, see
https://github.com/aio-libs/aiohttp/security/advisories/GHSA-v6wp-4m6f-gcjg.
+
+Thanks to 'Beast Glatisant' for finding the first instance of this issue.
--
2.30.1
From 6bde784138102492688e396c4a849a5857d0ee73 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko <sviat@redhat.com>
Date: Sun, 21 Feb 2021 21:34:08 +0100
Subject: [PATCH 09/15] Add a test case with append_slash and remove_slash both
disabled
---
tests/test_web_middleware.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index af62e218..c405cb7e 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -280,6 +280,7 @@ class TestNormalizePathMiddleware:
[
(True, False),
(False, True),
+ (False, False),
],
)
async def test_open_redirects(
--
2.30.1
From df5b64059969b4ee4366e452e16d788edff638ba Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko <sviat@redhat.com>
Date: Sun, 21 Feb 2021 21:34:32 +0100
Subject: [PATCH 10/15] Use list for parametrize params
---
tests/test_web_middleware.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index c405cb7e..6e1da9da 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -276,7 +276,7 @@ class TestNormalizePathMiddleware:
web.normalize_path_middleware(append_slash=True, remove_slash=True)
@pytest.mark.parametrize(
- "append_slash, remove_slash",
+ ["append_slash", "remove_slash"],
[
(True, False),
(False, True),
--
2.30.1
From cadef2699326f3e8eee33f92f9e79ec65909d2b6 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
Date: Sun, 21 Feb 2021 21:35:08 +0100
Subject: [PATCH 11/15] Autoformat tests
---
tests/test_web_middleware.py | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index 6e1da9da..beac6ea3 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -284,8 +284,8 @@ class TestNormalizePathMiddleware:
],
)
async def test_open_redirects(
- self, append_slash: bool, remove_slash: bool,
- aiohttp_client) -> None:
+ self, append_slash: bool, remove_slash: bool, aiohttp_client
+ ) -> None:
async def handle(request: web.Request) -> web.StreamResponse:
pytest.fail(
msg="Security advisory 'GHSA-v6wp-4m6f-gcjg' test handler "
@@ -293,13 +293,18 @@ class TestNormalizePathMiddleware:
pytrace=False,
)
- app = web.Application(middlewares=[web.normalize_path_middleware(
- append_slash=append_slash, remove_slash=remove_slash)])
+ app = web.Application(
+ middlewares=[
+ web.normalize_path_middleware(
+ append_slash=append_slash, remove_slash=remove_slash
+ )
+ ]
+ )
app.add_routes([web.get("/", handle), web.get("/google.com", handle)])
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
resp = await client.get("//google.com", allow_redirects=False)
assert resp.status == 308
- assert resp.headers.get('Location') == '/google.com'
+ assert resp.headers.get("Location") == "/google.com"
assert resp.url.query == URL("//google.com").query
--
2.30.1
From d2b720ab10f63e4e040f976812d73d9b3a859ef8 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko <sviat@redhat.com>
Date: Sun, 21 Feb 2021 21:48:22 +0100
Subject: [PATCH 12/15] Use unshielded getitem in the test
---
tests/test_web_middleware.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/test_web_middleware.py b/tests/test_web_middleware.py
index beac6ea3..69b53080 100644
--- a/tests/test_web_middleware.py
+++ b/tests/test_web_middleware.py
@@ -304,7 +304,7 @@ class TestNormalizePathMiddleware:
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
resp = await client.get("//google.com", allow_redirects=False)
assert resp.status == 308
- assert resp.headers.get("Location") == "/google.com"
+ assert resp.headers["Location"] == "/google.com"
assert resp.url.query == URL("//google.com").query
--
2.30.1
From a67c0994f6d7031f7e8fe5fe90d2118c3c1e7917 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko <sviat@redhat.com>
Date: Sun, 21 Feb 2021 21:52:29 +0100
Subject: [PATCH 13/15] Add a security note to the fix
---
aiohttp/web_middlewares.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/aiohttp/web_middlewares.py b/aiohttp/web_middlewares.py
index 5210b445..f4d6e506 100644
--- a/aiohttp/web_middlewares.py
+++ b/aiohttp/web_middlewares.py
@@ -101,7 +101,7 @@ def normalize_path_middleware(
paths_to_check.append(merged_slashes[:-1])
for path in paths_to_check:
- path = re.sub("^//+", "/", path)
+ path = re.sub("^//+", "/", path) # SECURITY: GHSA-v6wp-4m6f-gcjg
resolves, request = await _check_request_resolves(
request, path)
if resolves:
--
2.30.1
From aa476d4283162a67d182666d2815ef6bdce8ab66 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko <sviat@redhat.com>
Date: Thu, 25 Feb 2021 02:58:45 +0100
Subject: [PATCH 14/15] Update credits in the change fragment
---
CHANGES/openredirects.bugfix | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/CHANGES/openredirects.bugfix b/CHANGES/openredirects.bugfix
index 7fa556c8..e7dcd8d7 100644
--- a/CHANGES/openredirects.bugfix
+++ b/CHANGES/openredirects.bugfix
@@ -3,4 +3,4 @@
more details, see
https://github.com/aio-libs/aiohttp/security/advisories/GHSA-v6wp-4m6f-gcjg.
-Thanks to 'Beast Glatisant' for finding the first instance of this issue.
+Thanks to `Beast Glatisant <https://github.com/g147>`__ for finding the first instance of this issue and `Jelmer Vernooij <https://jelmer.uk/>`__ for reporting and tracking it down in aiohttp.
--
2.30.1

View File

@@ -25,15 +25,20 @@
buildPythonPackage rec {
pname = "aiohttp";
version = "3.6.2";
version = "3.6.3";
# https://github.com/aio-libs/aiohttp/issues/4525 python3.8 failures
disabled = pythonOlder "3.5";
src = fetchPypi {
inherit pname version;
sha256 = "09pkw6f1790prnrq0k8cqgnf1qy57ll8lpmc6kld09q7zw4vi6i5";
sha256 = "698cd7bc3c7d1b82bb728bae835724a486a8c376647aec336aa21a60113c3645";
};
patches = [
# Backport of https://github.com/aio-libs/aiohttp/commit/2545222a3853e31ace15d87ae0e2effb7da0c96b
./CVE-2021-21330.patch
];
checkInputs = [
pytestrunner pytestCheckHook gunicorn async_generator pytest_xdist
pytest-mock pytestcov trustme brotlipy freezegun