Skip to content

Commit

Permalink
pythongh-87389: Fix an open redirection vulnerability in http.server.
Browse files Browse the repository at this point in the history
Fix an open redirection vulnerability in the `http.server` module when
an URI path starts with `//`.  Vulnerability discovered, and initial fix
proposed, by Hamza Avvan.

Test authored and secondary mitigation by Gregory P. Smith [Google].
  • Loading branch information
gpshead committed Jun 15, 2022
1 parent 50e0866 commit 2f09fe2
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 4 deletions.
14 changes: 12 additions & 2 deletions Lib/http/server.py
Expand Up @@ -329,6 +329,13 @@ def parse_request(self):
return False
self.command, self.path = command, path

# gh-87389: The purpose of replacing '//' with '/' is to protect against
# open redirect attacks module which could be triggered if the path
# starts with '//' because web clients treat //path as an absolute url
# without scheme (similar to http://path) rather than a relative path.
if self.path.startswith('//'):
self.path = '/' + self.path.lstrip('/') # Reduce to a single /

# Examine the headers and look for a Connection directive.
try:
self.headers = http.client.parse_headers(self.rfile,
Expand Down Expand Up @@ -682,8 +689,11 @@ def send_head(self):
if not parts.path.endswith('/'):
# redirect browser - doing basically what apache does
self.send_response(HTTPStatus.MOVED_PERMANENTLY)
new_parts = (parts[0], parts[1], parts[2] + '/',
parts[3], parts[4])
# scheme[0] and netloc[1] are intentionally blanked out as we
# are only processing a path. They could allow injection into
# Location header if self.path wound up containing
# more than it was supposed to. See gh-87389.
new_parts = ('', '', parts[2] + '/', parts[3], parts[4])
new_url = urllib.parse.urlunsplit(new_parts)
self.send_header("Location", new_url)
self.send_header("Content-Length", "0")
Expand Down
24 changes: 22 additions & 2 deletions Lib/test/test_httpservers.py
Expand Up @@ -334,7 +334,7 @@ class request_handler(NoLogRequestHandler, SimpleHTTPRequestHandler):
pass

def setUp(self):
BaseTestCase.setUp(self)
super().setUp()
self.cwd = os.getcwd()
basetempdir = tempfile.gettempdir()
os.chdir(basetempdir)
Expand Down Expand Up @@ -362,7 +362,7 @@ def tearDown(self):
except:
pass
finally:
BaseTestCase.tearDown(self)
super().tearDown()

def check_status_and_reason(self, response, status, data=None):
def close_conn():
Expand Down Expand Up @@ -418,6 +418,26 @@ def test_undecodable_filename(self):
self.check_status_and_reason(response, HTTPStatus.OK,
data=os_helper.TESTFN_UNDECODABLE)

def test_get_dir_redirect_location_domain_injection_bug(self):
"""Ensure //evil.co/..%2f../../X does not put //evil.co/ in Location.
//domain/ in a Location header is a redirect to a new domain name.
https://github.com/python/cpython/issues/87389
This checks that a path resolving to a directory on our server cannot
resolve into a redirect to another server telling it that the
directory in question exists on the Referrer server.
"""
os.mkdir(os.path.join(self.tempdir, 'existing_directory'))
attack_url = f'//python.org/..%2f..%2f..%2f..%2f..%2f../%0a%0d/../{self.tempdir_name}/existing_directory'
response = self.request(attack_url)
self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
location = response.getheader('Location')
self.assertFalse(location.startswith('//'), msg=location)
self.assertEqual(location, attack_url[1:] + '/',
msg='Expected Location: to start with a single / and '
'end with a / as this is a directory redirect.')

def test_get(self):
#constructs the path relative to the root directory of the HTTPServer
response = self.request(self.base_url + '/test')
Expand Down
@@ -0,0 +1,3 @@
:mod:`http.server`: Fix an open redirection vulnerability in the HTTP server
when an URI path starts with ``//``. Vulnerability discovered, and initial
fix proposed, by Hamza Avvan.

0 comments on commit 2f09fe2

Please sign in to comment.