Skip to content
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.

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

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

Merged
merged 7 commits into from Jun 21, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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 possibly triggered if the path starts
# with '//' because http clients treat //path as an absolute URI
# without scheme (similar to http://path) rather than a 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
# the 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
33 changes: 31 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,35 @@ 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.

//netloc/ in a Location header is a redirect to a new host.
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.
"""
os.mkdir(os.path.join(self.tempdir, 'existing_directory'))
url = f'/python.org/..%2f..%2f..%2f..%2f..%2f../%0a%0d/../{self.tempdir_name}/existing_directory'
# Canonicalizes to /tmp/tempdir_name/existing_directory which does
# exist and is a dir, triggering the 301 redirect and former bug.
attack_url = f'/{url}' # //python.org... multi-slash prefix, no trailing slash
expected_location = f'{url}/' # /python.org.../ single slash single prefix, trailing slash

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, expected_location,
msg='Expected Location header to start with a single / and '
gpshead marked this conversation as resolved.
Show resolved Hide resolved
'end with a / as this is a directory redirect.')
gpshead marked this conversation as resolved.
Show resolved Hide resolved

attack3_url = f'//{url}' # ///python.org... triple-slash prefix, no trailing slash
response = self.request(attack3_url)
self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
self.assertEqual(response.getheader('Location'), expected_location)

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.