Patrick Williams | 61a2d43 | 2022-10-18 12:43:19 -0500 | [diff] [blame^] | 1 | From 4dc2cae3abd75f386374d0635d00443b897d0672 Mon Sep 17 00:00:00 2001 |
| 2 | From: "Miss Islington (bot)" |
| 3 | <31488909+miss-islington@users.noreply.github.com> |
| 4 | Date: Wed, 22 Jun 2022 01:42:52 -0700 |
| 5 | Subject: [PATCH] gh-87389: Fix an open redirection vulnerability in |
| 6 | http.server. (GH-93879) (GH-94094) |
| 7 | |
| 8 | Fix an open redirection vulnerability in the `http.server` module when |
| 9 | an URI path starts with `//` that could produce a 301 Location header |
| 10 | with a misleading target. Vulnerability discovered, and logic fix |
| 11 | proposed, by Hamza Avvan (@hamzaavvan). |
| 12 | |
| 13 | Test and comments authored by Gregory P. Smith [Google]. |
| 14 | (cherry picked from commit 4abab6b603dd38bec1168e9a37c40a48ec89508e) |
| 15 | |
| 16 | Co-authored-by: Gregory P. Smith <greg@krypto.org> |
| 17 | |
| 18 | Signed-off-by: Riyaz Khan <Riyaz.Khan@kpit.com> |
| 19 | |
| 20 | CVE: CVE-2021-28861 |
| 21 | |
| 22 | Upstream-Status: Backport [https://github.com/python/cpython/commit/4dc2cae3abd75f386374d0635d00443b897d0672] |
| 23 | |
| 24 | --- |
| 25 | Lib/http/server.py | 7 +++ |
| 26 | Lib/test/test_httpservers.py | 53 ++++++++++++++++++- |
| 27 | ...2-06-15-20-09-23.gh-issue-87389.QVaC3f.rst | 3 ++ |
| 28 | 3 files changed, 61 insertions(+), 2 deletions(-) |
| 29 | create mode 100644 Misc/NEWS.d/next/Security/2022-06-15-20-09-23.gh-issue-87389.QVaC3f.rst |
| 30 | |
| 31 | diff --git a/Lib/http/server.py b/Lib/http/server.py |
| 32 | index 38f7accad7a3..39de35458c38 100644 |
| 33 | --- a/Lib/http/server.py |
| 34 | +++ b/Lib/http/server.py |
| 35 | @@ -332,6 +332,13 @@ def parse_request(self): |
| 36 | return False |
| 37 | self.command, self.path = command, path |
| 38 | |
| 39 | + # gh-87389: The purpose of replacing '//' with '/' is to protect |
| 40 | + # against open redirect attacks possibly triggered if the path starts |
| 41 | + # with '//' because http clients treat //path as an absolute URI |
| 42 | + # without scheme (similar to http://path) rather than a path. |
| 43 | + if self.path.startswith('//'): |
| 44 | + self.path = '/' + self.path.lstrip('/') # Reduce to a single / |
| 45 | + |
| 46 | # Examine the headers and look for a Connection directive. |
| 47 | try: |
| 48 | self.headers = http.client.parse_headers(self.rfile, |
| 49 | diff --git a/Lib/test/test_httpservers.py b/Lib/test/test_httpservers.py |
| 50 | index 87d4924a34b3..fb026188f0b4 100644 |
| 51 | --- a/Lib/test/test_httpservers.py |
| 52 | +++ b/Lib/test/test_httpservers.py |
| 53 | @@ -330,7 +330,7 @@ class request_handler(NoLogRequestHandler, SimpleHTTPRequestHandler): |
| 54 | pass |
| 55 | |
| 56 | def setUp(self): |
| 57 | - BaseTestCase.setUp(self) |
| 58 | + super().setUp() |
| 59 | self.cwd = os.getcwd() |
| 60 | basetempdir = tempfile.gettempdir() |
| 61 | os.chdir(basetempdir) |
| 62 | @@ -358,7 +358,7 @@ def tearDown(self): |
| 63 | except: |
| 64 | pass |
| 65 | finally: |
| 66 | - BaseTestCase.tearDown(self) |
| 67 | + super().tearDown() |
| 68 | |
| 69 | def check_status_and_reason(self, response, status, data=None): |
| 70 | def close_conn(): |
| 71 | @@ -414,6 +414,55 @@ def test_undecodable_filename(self): |
| 72 | self.check_status_and_reason(response, HTTPStatus.OK, |
| 73 | data=support.TESTFN_UNDECODABLE) |
| 74 | |
| 75 | + def test_get_dir_redirect_location_domain_injection_bug(self): |
| 76 | + """Ensure //evil.co/..%2f../../X does not put //evil.co/ in Location. |
| 77 | + |
| 78 | + //netloc/ in a Location header is a redirect to a new host. |
| 79 | + https://github.com/python/cpython/issues/87389 |
| 80 | + |
| 81 | + This checks that a path resolving to a directory on our server cannot |
| 82 | + resolve into a redirect to another server. |
| 83 | + """ |
| 84 | + os.mkdir(os.path.join(self.tempdir, 'existing_directory')) |
| 85 | + url = f'/python.org/..%2f..%2f..%2f..%2f..%2f../%0a%0d/../{self.tempdir_name}/existing_directory' |
| 86 | + expected_location = f'{url}/' # /python.org.../ single slash single prefix, trailing slash |
| 87 | + # Canonicalizes to /tmp/tempdir_name/existing_directory which does |
| 88 | + # exist and is a dir, triggering the 301 redirect logic. |
| 89 | + response = self.request(url) |
| 90 | + self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY) |
| 91 | + location = response.getheader('Location') |
| 92 | + self.assertEqual(location, expected_location, msg='non-attack failed!') |
| 93 | + |
| 94 | + # //python.org... multi-slash prefix, no trailing slash |
| 95 | + attack_url = f'/{url}' |
| 96 | + response = self.request(attack_url) |
| 97 | + self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY) |
| 98 | + location = response.getheader('Location') |
| 99 | + self.assertFalse(location.startswith('//'), msg=location) |
| 100 | + self.assertEqual(location, expected_location, |
| 101 | + msg='Expected Location header to start with a single / and ' |
| 102 | + 'end with a / as this is a directory redirect.') |
| 103 | + |
| 104 | + # ///python.org... triple-slash prefix, no trailing slash |
| 105 | + attack3_url = f'//{url}' |
| 106 | + response = self.request(attack3_url) |
| 107 | + self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY) |
| 108 | + self.assertEqual(response.getheader('Location'), expected_location) |
| 109 | + |
| 110 | + # If the second word in the http request (Request-URI for the http |
| 111 | + # method) is a full URI, we don't worry about it, as that'll be parsed |
| 112 | + # and reassembled as a full URI within BaseHTTPRequestHandler.send_head |
| 113 | + # so no errant scheme-less //netloc//evil.co/ domain mixup can happen. |
| 114 | + attack_scheme_netloc_2slash_url = f'https://pypi.org/{url}' |
| 115 | + expected_scheme_netloc_location = f'{attack_scheme_netloc_2slash_url}/' |
| 116 | + response = self.request(attack_scheme_netloc_2slash_url) |
| 117 | + self.check_status_and_reason(response, HTTPStatus.MOVED_PERMANENTLY) |
| 118 | + location = response.getheader('Location') |
| 119 | + # We're just ensuring that the scheme and domain make it through, if |
| 120 | + # there are or aren't multiple slashes at the start of the path that |
| 121 | + # follows that isn't important in this Location: header. |
| 122 | + self.assertTrue(location.startswith('https://pypi.org/'), msg=location) |
| 123 | + |
| 124 | def test_get(self): |
| 125 | #constructs the path relative to the root directory of the HTTPServer |
| 126 | response = self.request(self.base_url + '/test') |
| 127 | diff --git a/Misc/NEWS.d/next/Security/2022-06-15-20-09-23.gh-issue-87389.QVaC3f.rst b/Misc/NEWS.d/next/Security/2022-06-15-20-09-23.gh-issue-87389.QVaC3f.rst |
| 128 | new file mode 100644 |
| 129 | index 000000000000..029d437190de |
| 130 | --- /dev/null |
| 131 | +++ b/Misc/NEWS.d/next/Security/2022-06-15-20-09-23.gh-issue-87389.QVaC3f.rst |
| 132 | @@ -0,0 +1,3 @@ |
| 133 | +:mod:`http.server`: Fix an open redirection vulnerability in the HTTP server |
| 134 | +when an URI path starts with ``//``. Vulnerability discovered, and initial |
| 135 | +fix proposed, by Hamza Avvan. |