diff options
author | Sean Mooney <work@seanmooney.info> | 2021-08-23 15:37:48 +0100 |
---|---|---|
committer | Elod Illes <elod.illes@est.tech> | 2021-09-08 21:00:08 +0200 |
commit | 9588cdbfd4649ea53d60303f2d10c5d62a070a07 (patch) | |
tree | 1b90a4928dc8214852dfb52100dad89de71db5c0 | |
parent | 1eceeebfb25ccdacdb8c74c5171f0fa9e591ce82 (diff) | |
download | nova-9588cdbfd4649ea53d60303f2d10c5d62a070a07.tar.gz |
address open redirect with 3 forward slashes
Ie36401c782f023d1d5f2623732619105dc2cfa24 was intended
to address OSSA-2021-002 (CVE-2021-3654) however after its
release it was discovered that the fix only worked
for urls with 2 leading slashes or more then 4.
This change adresses the missing edgecase for 3 leading slashes
and also maintian support for rejecting 2+.
Conflicts:
nova/tests/unit/console/test_websocketproxy.py
NOTE: conflict is due to I58b0382c86d4ef798572edb63d311e0e3e6937bb
is missing in Victoria and Ie36401c782f023d1d5f2623732619105dc2cfa24
backport contained conflicts and methods order was swapped.
Change-Id: I95f68be76330ff09e5eabb5ef8dd9a18f5547866
co-authored-by: Matteo Pozza
Closes-Bug: #1927677
(cherry picked from commit 6fbd0b758dcac71323f3be179b1a9d1c17a4acc5)
(cherry picked from commit 47dad4836a26292e9d34e516e1525ecf00be127c)
-rw-r--r-- | nova/console/websocketproxy.py | 7 | ||||
-rw-r--r-- | nova/tests/unit/console/test_websocketproxy.py | 34 |
2 files changed, 35 insertions, 6 deletions
diff --git a/nova/console/websocketproxy.py b/nova/console/websocketproxy.py index 1410d64260..8512ac62ad 100644 --- a/nova/console/websocketproxy.py +++ b/nova/console/websocketproxy.py @@ -290,14 +290,9 @@ class NovaProxyRequestHandler(websockify.ProxyRequestHandler): if os.path.isdir(path): parts = urlparse.urlsplit(self.path) if not parts.path.endswith('/'): - # redirect browser - doing basically what apache does - new_parts = (parts[0], parts[1], parts[2] + '/', - parts[3], parts[4]) - new_url = urlparse.urlunsplit(new_parts) - # Browsers interpret "Location: //uri" as an absolute URI # like "http://URI" - if new_url.startswith('//'): + if self.path.startswith('//'): self.send_error(HTTPStatus.BAD_REQUEST, "URI must not start with //") return None diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 4ed2d2d4dc..3ec73d817a 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -660,6 +660,40 @@ class NovaProxyRequestHandlerTestCase(test.NoDBTestCase): self.assertIn('Error code: 400', self.data) self.assertIn('Message: URI must not start with //', self.data) + def test_reject_open_redirect_3_slashes(self): + # This will test the behavior when an attempt is made to cause an open + # redirect. It should be rejected. + mock_req = mock.MagicMock() + mock_req.makefile().readline.side_effect = [ + b'GET ///example.com/%2F.. HTTP/1.1\r\n', + b'' + ] + + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data by calling the + # request socket sendall() method. + self.data = b'' + + def fake_sendall(data): + self.data += data + + mock_req.sendall.side_effect = fake_sendall + + client_addr = ('8.8.8.8', 54321) + mock_server = mock.MagicMock() + # This specifies that the server will be able to handle requests other + # than only websockets. + mock_server.only_upgrade = False + + # Constructing a handler will process the mock_req request passed in. + websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + + # Verify no redirect happens and instead a 400 Bad Request is returned. + self.data = self.data.decode() + self.assertIn('Error code: 400', self.data) + self.assertIn('Message: URI must not start with //', self.data) + @mock.patch('websockify.websocketproxy.select_ssl_version') def test_ssl_min_version_is_not_set(self, mock_select_ssl): websocketproxy.NovaWebSocketProxy() |