Skip to content

Commit

Permalink
Merge branch 'GHSA-v6wp-4m6f-gcjg' into master
Browse files Browse the repository at this point in the history
This patch fixes an open redirect vulnerability bug in
`aiohttp.web_middlewares.normalize_path_middleware` by
making sure that there's at most one slash at the
beginning of the `Location` header value.

Refs:
* https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html
* GHSA-v6wp-4m6f-gcjg
  • Loading branch information
webknjaz committed Feb 25, 2021
2 parents f2afa2f + d2f9a83 commit 2545222
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 0 deletions.
9 changes: 9 additions & 0 deletions CHANGES/5497.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
**(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.

Thanks to `Beast Glatisant <https://github.com/g147>`__ for
finding the firstinstance of this issue and `Jelmer Vernooij
<https://jelmer.uk/>`__ for reporting and tracking it down
in aiohttp.
1 change: 1 addition & 0 deletions aiohttp/web_middlewares.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ async def impl(request: Request, handler: _Handler) -> StreamResponse:
paths_to_check.append(merged_slashes[:-1])

for path in paths_to_check:
path = re.sub("^//+", "/", path) # SECURITY: GHSA-v6wp-4m6f-gcjg
resolves, request = await _check_request_resolves(request, path)
if resolves:
raise redirect_class(request.raw_path + query)
Expand Down
32 changes: 32 additions & 0 deletions tests/test_web_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,38 @@ async def test_cannot_remove_and_add_slash(self) -> None:
with pytest.raises(AssertionError):
web.normalize_path_middleware(append_slash=True, remove_slash=True)

@pytest.mark.parametrize(
["append_slash", "remove_slash"],
[
(True, False),
(False, True),
(False, False),
],
)
async def test_open_redirects(
self, append_slash: bool, remove_slash: bool, aiohttp_client: Any
) -> None:
async def handle(request: web.Request) -> web.StreamResponse:
pytest.fail(
msg="Security advisory 'GHSA-v6wp-4m6f-gcjg' test handler "
"matched unexpectedly",
pytrace=False,
)

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["Location"] == "/google.com"
assert resp.url.query == URL("//google.com").query


async def test_bug_3669(aiohttp_client: Any):
async def paymethod(request):
Expand Down

3 comments on commit 2545222

@utkarsh2102
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @webknjaz,

Just to be sure, this isn't affecting v1.2.0 of this library, no? 🙈
I know this is a very old version but with my LTS team hat on, I want to ensure that the version of python-aiohttp in Debian Stretch is not affected. Please let me know what you think!?

Thanks for your work on this! ❤️

@webknjaz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @utkarsh2102, it shouldn't since it was added in v1.3.0: 8c44b21. But that version is so outdated that nobody should be using it because it's been unsupported for years.

@utkarsh2102
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you so much!

Even though it's not being used directly by developers, we support all the packages in Debian Stretch as a part of LTS maintenance, so we tend to fix most of the security issues by backporting the patches to stable and oldstable releases. 😄

Please sign in to comment.