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: avoid treating path as URI with netloc #93894

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
18 changes: 13 additions & 5 deletions Lib/http/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -678,13 +678,10 @@ def send_head(self):
path = self.translate_path(self.path)
f = None
if os.path.isdir(path):
parts = urllib.parse.urlsplit(self.path)
if not parts.path.endswith('/'):
new_url = _add_trailing_slash(self.path)
if new_url:
# 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])
new_url = urllib.parse.urlunsplit(new_parts)
self.send_header("Location", new_url)
self.send_header("Content-Length", "0")
self.end_headers()
Expand Down Expand Up @@ -881,6 +878,17 @@ def guess_type(self, path):
return 'application/octet-stream'


def _add_trailing_slash(path):
nascheme marked this conversation as resolved.
Show resolved Hide resolved
"""Returns URL with trailing slash on path, if required. If not required,
returns None.
"""
path, _, fragment = path.partition('#')
path, _, query = path.partition('?')
if path.endswith('/'):
return None # already has slash, no redirect needed
return urllib.parse.urlunsplit(('', '', path + '/', query, fragment))


# Utilities for CGIHTTPRequestHandler

def _url_collapse_path(path):
Expand Down
24 changes: 22 additions & 2 deletions Lib/test/test_httpservers.py
Original file line number Diff line number Diff line change
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):
nascheme marked this conversation as resolved.
Show resolved Hide resolved
"""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
10 changes: 8 additions & 2 deletions Lib/urllib/parse.py
Original file line number Diff line number Diff line change
Expand Up @@ -497,9 +497,15 @@ def urlunsplit(components):
This may result in a slightly different, but equivalent URL, if the URL that
was parsed originally had unnecessary delimiters (for example, a ? with an
empty query; the RFC states that these are equivalent)."""
scheme, netloc, url, query, fragment, _coerce_result = (
scheme, netloc, path, query, fragment, _coerce_result = (
_coerce_args(*components))
if netloc or (scheme and scheme in uses_netloc and url[:2] != '//'):
if scheme in uses_netloc and path.startswith('//'):
nascheme marked this conversation as resolved.
Show resolved Hide resolved
nascheme marked this conversation as resolved.
Show resolved Hide resolved
# gh-87389: avoid confusing a path with multiple leading slashes
# as a URI relative reference.
url = '/' + path.lstrip('/')
else:
url = path
if netloc or (scheme and scheme in uses_netloc):
if url and url[:1] != '/': url = '/' + url
url = '//' + (netloc or '') + url
if scheme:
Expand Down