bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

hamzaavvan · 2021-03-13T21:13:53Z

Due to no protection against multiple (/) at the beginning of a url an attacker could achieve an open redirection by crafting a malformed URI followed by an existing directory.

Payload: http://127.0.0.1:8000//attacker.com/..%2f..%2f../anyexistingdirectory

The main reason behind open redirection is that there's no (/) at the end of anyexistingdirectory because the server is checking for the path supplied is a valid directory at send_head() method from Lib/http/server.py. Right after that, it's checking for the path ending with a (/) or not. So, if there's no (/) at the end of the path then the server will issue a Location header to redirect the web client to that specific directory.

While issuing the redirection, this part //attacker.com/..%2f..%2f../anyexistingdirectory will be sent to the Location header's value due to which any web client or browser will consider it as a new url because of multiple (/) at the beginning of the path.

So to mitigate this issue I decided to use regex to replace all the occurrences of (/) from the beginning of the path.

 self.path = re.sub(r'(^(/|\\)+)', '/', self.path)

This regex will replace multiple entries (if present) of (/) or (\) from the beginning of the path. So that the path would be:

//attacker.com/..%2f..%2f../anyexistingdirectory -> /attacker.com/..%2f..%2f../anyexistingdirectory
//\attacker.com/..%2f..%2f../anyexistingdirectory -> /attacker.com/..%2f..%2f../anyexistingdirectory
\//\attacker.com/..%2f..%2f../anyexistingdirectory -> /attacker.com/..%2f..%2f../anyexistingdirectory

And according to these test cases there was no redirection issued from the server after implementing the fix.

https://bugs.python.org/issue43223

Lib/http/server.py

Misc/NEWS.d/next/Security/2021-03-13-21-25-29.bpo-43223.ieBVWq.rst

Lib/http/server.py

vstinner

Can you please try to write an unit test?

Lib/http/server.py

github-actions · 2021-04-18T00:10:32Z

This PR is stale because it has been open for 30 days with no activity.

hamzaavvan · 2021-05-06T12:32:43Z

Sorry I was busy with my other projects.
Will be pushing the test case shortly.

hamzaavvan · 2021-05-06T12:35:32Z

Since I'm new to writing test cases, please help me in correcting the code if something went wrong.

Misc/NEWS.d/next/Security/2021-03-13-21-25-29.bpo-43223.ieBVWq.rst

Fix an open redirection vulnerability in the HTTP server when a URL contains ``//``. Added test case for bpo-43223 patch

hamzaavvan · 2021-06-06T12:00:23Z

@vstinner please have a look at the unit test.

ambv · 2021-07-12T11:50:03Z

Lib/test/test_http/test_http.py

+
+    def test_http_parse_request(self):
+        self.assertEqual(re.sub(r'^/+', '/', '//test.com'), '/test.com', '//test.com should be converted to a proper relative path')
+        self.assertEqual(re.sub(r'^/+', '/', '///test.com'), '/test.com', '///test.com should be converted to a proper relative path')


This doesn't test anything useful. It only checks that some regular expression matches two strings. Since your code change is in http.server, your test should execute the http.server path that you changed.

A good test should fail without your code change to http.server, and pass with your change.

Sorry I've been away due to my workload. But now as I've got some spare time I can do it better. As I've already said earlier that I'm new to writing these test cases and after checking out your comment I'm still confused about your requirements

Can you please elaborate these two statements:

Since your code change is in http.server, your test should execute the http.server path that you changed.

and this one:

A good test should fail without your code change to http.server, and pass with your change.

@hamzaavvan, to put it in the simplest terms:

on a clean checkout of the main branch, write a test that fails because it triggers the problem described in BPO-43223; this will prove we have a problem;

then include your fix to prove that the fix solved the problem.

The test will have to actually run the HTTP server to be useful.

bedevere-bot · 2021-07-12T11:50:12Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

jhadvig · 2022-04-08T13:26:22Z

@hamzaavvan ping :)

hamzaavvan · 2022-04-08T13:51:52Z

I've lost access to my account 😥 replying from email..

…

On Fri, Apr 8, 2022, 6:26 PM Jakub Hadvig ***@***.***> wrote: @hamzaavvan <https://github.com/hamzaavvan> ping :) — Reply to this email directly, view it on GitHub <#24848 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNY3RNMO7QIH6RKTIMDD53VEAXYXANCNFSM4ZEGXF4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gpshead · 2022-06-15T20:13:49Z

This PR has been replaced by #93879

the-knights-who-say-ni added the CLA signed label Mar 13, 2021

bedevere-bot added the awaiting review label Mar 13, 2021

hamzaavvan changed the title ~~bpo-43223: Patched Open Redirection In SimpleHTTPServer Module~~ bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module Mar 13, 2021

vstinner reviewed Mar 16, 2021

View reviewed changes

Lib/http/server.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Security/2021-03-13-21-25-29.bpo-43223.ieBVWq.rst Outdated Show resolved Hide resolved

hamzaavvan force-pushed the fix-issue-43223 branch 2 times, most recently from e2137b6 to 403490c Compare March 16, 2021 17:42

vstinner reviewed Mar 16, 2021

View reviewed changes

Lib/http/server.py Outdated Show resolved Hide resolved

hamzaavvan force-pushed the fix-issue-43223 branch 3 times, most recently from f61138a to 0fe6eed Compare March 16, 2021 18:09

vstinner reviewed Mar 18, 2021

View reviewed changes

Lib/http/server.py Outdated Show resolved Hide resolved

Lib/http/server.py Outdated Show resolved Hide resolved

github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 18, 2021

hamzaavvan force-pushed the fix-issue-43223 branch from 0fe6eed to 3717f3f Compare May 6, 2021 12:34

github-actions bot removed the stale Stale PR or inactive for long period of time. label May 7, 2021

ned-deily reviewed May 7, 2021

View reviewed changes

Misc/NEWS.d/next/Security/2021-03-13-21-25-29.bpo-43223.ieBVWq.rst Outdated Show resolved Hide resolved

bpo-43223: Fix Open Redirection In http.server module

42eb552

Fix an open redirection vulnerability in the HTTP server when a URL contains ``//``. Added test case for bpo-43223 patch

hamzaavvan force-pushed the fix-issue-43223 branch from 3717f3f to 42eb552 Compare May 7, 2021 22:59

hamzaavvan requested review from vstinner and ned-deily May 8, 2021 14:32

ned-deily removed their request for review May 21, 2021 22:03

ambv requested changes Jul 12, 2021

View reviewed changes

bedevere-bot removed the awaiting review label Jul 12, 2021

bedevere-bot added the awaiting changes label Jul 12, 2021

hamzaavvan mannequin mentioned this pull request Apr 10, 2022

[security] CVE-2021-28861: http.server: Open Redirection if the URL path starts with // #87389

Closed

gpshead mentioned this pull request Jun 15, 2022

gh-87389: Fix an open redirection vulnerability in http.server. #93879

Merged

gpshead closed this Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

hamzaavvan commented Mar 13, 2021 •

edited

Loading

vstinner left a comment

github-actions bot commented Apr 18, 2021

hamzaavvan commented May 6, 2021

hamzaavvan commented May 6, 2021

hamzaavvan commented Jun 6, 2021

ambv Jul 12, 2021

hamzaavvan Aug 21, 2021 •

edited

Loading

ambv Aug 29, 2021

bedevere-bot commented Jul 12, 2021

jhadvig commented Apr 8, 2022

hamzaavvan commented Apr 8, 2022 via email

gpshead commented Jun 15, 2022

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module #24848

Conversation

hamzaavvan commented Mar 13, 2021 • edited Loading

vstinner left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2021

hamzaavvan commented May 6, 2021

hamzaavvan commented May 6, 2021

hamzaavvan commented Jun 6, 2021

ambv Jul 12, 2021

Choose a reason for hiding this comment

hamzaavvan Aug 21, 2021 • edited Loading

Choose a reason for hiding this comment

ambv Aug 29, 2021

Choose a reason for hiding this comment

bedevere-bot commented Jul 12, 2021

jhadvig commented Apr 8, 2022

hamzaavvan commented Apr 8, 2022 via email

gpshead commented Jun 15, 2022

hamzaavvan commented Mar 13, 2021 •

edited

Loading

hamzaavvan Aug 21, 2021 •

edited

Loading