Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hamzaavvan wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Lib/http/server.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -101,6 +101,7 @@
import socket # For gethostbyaddr()
import socketserver
import sys
import re
import time
import urllib.parse
import contextlib
Expand DownExpand Up@@ -332,6 +333,12 @@ def parse_request(self):
return False
self.command, self.path = command, path

# bpo-43223: The purpose of replacing '//' with '/' is to protect against
# open redirect attacks reside within http.server module which can be triggered
# if the path contains '//' at the beginning because web clients treat //path as
# an absolute url without scheme (similar to http://path) rather than a relative path
self.path = re.sub(r'^(/)+', '/', self.path)

# Examine the headers and look for a Connection directive.
try:
self.headers = http.client.parse_headers(self.rfile,
Expand Down
11 changes: 11 additions & 0 deletions Lib/test/test_http/test_http.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
import unittest
import re

class TestHTTP(unittest.TestCase):

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')
Copy link
Contributor

@ambvambvJul 12, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

@hamzaavvanhamzaavvanAug 21, 2021•
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ambvambvAug 29, 2021

Choose a reason for hiding this comment

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

@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.


if __name__ == '__main__':
unittest.main()
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Security/2021-03-13-21-25-29.bpo-43223.ieBVWq.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
:mod:`http.server`: Fix an open redirection vulnerability in the HTTP server when an URL contains ``//``.
Vulnerability discovered and fixed by Hamza Avvan.