Skip to content

Commit 47dad48

Browse files
committed
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+. Change-Id: I95f68be76330ff09e5eabb5ef8dd9a18f5547866 co-authored-by: Matteo Pozza Closes-Bug: #1927677 (cherry picked from commit 6fbd0b7)
1 parent 088addf commit 47dad48

File tree

2 files changed

+35
-6
lines changed

2 files changed

+35
-6
lines changed

nova/console/websocketproxy.py

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -289,14 +289,9 @@ def send_head(self):
289289
if os.path.isdir(path):
290290
parts = urlparse.urlsplit(self.path)
291291
if not parts.path.endswith('/'):
292-
# redirect browser - doing basically what apache does
293-
new_parts = (parts[0], parts[1], parts[2] + '/',
294-
parts[3], parts[4])
295-
new_url = urlparse.urlunsplit(new_parts)
296-
297292
# Browsers interpret "Location: //uri" as an absolute URI
298293
# like "http://URI"
299-
if new_url.startswith('//'):
294+
if self.path.startswith('//'):
300295
self.send_error(HTTPStatus.BAD_REQUEST,
301296
"URI must not start with //")
302297
return None

nova/tests/unit/console/test_websocketproxy.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,40 @@ def fake_sendall(data):
650650
self.assertIn('Error code: 400', self.data)
651651
self.assertIn('Message: URI must not start with //', self.data)
652652

653+
def test_reject_open_redirect_3_slashes(self):
654+
# This will test the behavior when an attempt is made to cause an open
655+
# redirect. It should be rejected.
656+
mock_req = mock.MagicMock()
657+
mock_req.makefile().readline.side_effect = [
658+
b'GET ///example.com/%2F.. HTTP/1.1\r\n',
659+
b''
660+
]
661+
662+
# Collect the response data to verify at the end. The
663+
# SimpleHTTPRequestHandler writes the response data by calling the
664+
# request socket sendall() method.
665+
self.data = b''
666+
667+
def fake_sendall(data):
668+
self.data += data
669+
670+
mock_req.sendall.side_effect = fake_sendall
671+
672+
client_addr = ('8.8.8.8', 54321)
673+
mock_server = mock.MagicMock()
674+
# This specifies that the server will be able to handle requests other
675+
# than only websockets.
676+
mock_server.only_upgrade = False
677+
678+
# Constructing a handler will process the mock_req request passed in.
679+
websocketproxy.NovaProxyRequestHandler(
680+
mock_req, client_addr, mock_server)
681+
682+
# Verify no redirect happens and instead a 400 Bad Request is returned.
683+
self.data = self.data.decode()
684+
self.assertIn('Error code: 400', self.data)
685+
self.assertIn('Message: URI must not start with //', self.data)
686+
653687
@mock.patch('nova.objects.ConsoleAuthToken.validate')
654688
def test_no_compute_rpcapi_with_invalid_token(self, mock_validate):
655689
"""Tests that we don't create a ComputeAPI object until we actually

0 commit comments

Comments
 (0)