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 all 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
7 changes: 7 additions & 0 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
53 changes: 51 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,55 @@ 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'
expected_location = f'{url}/' # /python.org.../ single slash single prefix, trailing slash
# Canonicalizes to /tmp/tempdir_name/existing_directory which does
# exist and is a dir, triggering the 301 redirect logic.
response = self.request(url)
self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
location = response.getheader('Location')
self.assertEqual(location, expected_location, msg='non-attack failed!')

# //python.org... multi-slash prefix, no trailing slash
attack_url = f'/{url}'
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

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

# If the second word in the http request (Request-URI for the http
# method) is a full URI, we don't worry about it, as that'll be parsed
# and reassembled as a full URI within BaseHTTPRequestHandler.send_head
# so no errant scheme-less //netloc//evil.co/ domain mixup can happen.
attack_scheme_netloc_2slash_url = f'https://pypi.org/{url}'
expected_scheme_netloc_location = f'{attack_scheme_netloc_2slash_url}/'
response = self.request(attack_scheme_netloc_2slash_url)
self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY)
location = response.getheader('Location')
# We're just ensuring that the scheme and domain make it through, if
# there are or aren't multiple slashes at the start of the path that
# follows that isn't important in this Location: header.
self.assertTrue(location.startswith('https://pypi.org/'), msg=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.