◐ Shell
reader mode source ↗
Skip to content

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

Closed
hamzaavvan wants to merge 1 commit into
python:mainfrom
hamzaavvan:fix-issue-43223
Closed

bpo-43223: [SECURITY] Patched Open Redirection In SimpleHTTPServer Module#24848
hamzaavvan wants to merge 1 commit into
python:mainfrom
hamzaavvan:fix-issue-43223

Conversation

@hamzaavvan

@hamzaavvan hamzaavvan commented Mar 13, 2021

Copy link
Copy Markdown

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:

  1. //attacker.com/..%2f..%2f../anyexistingdirectory -> /attacker.com/..%2f..%2f../anyexistingdirectory
  2. //\attacker.com/..%2f..%2f../anyexistingdirectory -> /attacker.com/..%2f..%2f../anyexistingdirectory
  3. \//\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

@hamzaavvan hamzaavvan changed the title bpo-43223: Patched Open Redirection In SimpleHTTPServer Module Mar 13, 2021
@hamzaavvan hamzaavvan force-pushed the fix-issue-43223 branch 2 times, most recently from e2137b6 to 403490c Compare March 16, 2021 17:42
@hamzaavvan hamzaavvan force-pushed the fix-issue-43223 branch 3 times, most recently from f61138a to 0fe6eed Compare March 16, 2021 18:09

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Can you please try to write an unit test?

@github-actions

Copy link
Copy Markdown

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

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

Copy link
Copy Markdown
Author

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

@hamzaavvan

Copy link
Copy Markdown
Author

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

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 7, 2021
Fix an open redirection vulnerability in the HTTP server when a URL contains ``//``.
Added test case for bpo-43223 patch
@hamzaavvan hamzaavvan requested review from ned-deily and vstinner May 8, 2021 14:32
@ned-deily ned-deily removed their request for review May 21, 2021 22:03
@hamzaavvan

Copy link
Copy Markdown
Author

@vstinner please have a look at the unit test.

@bedevere-bot

Copy link
Copy Markdown

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

jhadvig commented Apr 8, 2022

Copy link
Copy Markdown

@hamzaavvan ping :)

@hamzaavvan

hamzaavvan commented Apr 8, 2022 via email

Copy link
Copy Markdown
Author

182 hidden items Load more…
@gpshead

gpshead commented Jun 15, 2022

Copy link
Copy Markdown
Member

This PR has been replaced by #93879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants