From 04d48527b62a35d912f93bc75613a6cca606df66 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 13 May 2021 05:43:42 +0000 Subject: Reject open redirection in the console proxy NOTE(melwitt): This is the combination of two commits, the bug fix and a followup change to the unit test to enable it also run on Python < 3.6. Our console proxies (novnc, serial, spice) run in a websockify server whose request handler inherits from the python standard SimpleHTTPRequestHandler. There is a known issue [1] in the SimpleHTTPRequestHandler which allows open redirects by way of URLs in the following format: http://vncproxy.my.domain.com//example.com/%2F.. which if visited, will redirect a user to example.com. We can intercept a request and reject requests that pass a redirection URL beginning with "//" by implementing the SimpleHTTPRequestHandler.send_head() method containing the vulnerability to reject such requests with a 400 Bad Request. This code is copied from a patch suggested in one of the issue comments [2]. Closes-Bug: #1927677 [1] https://bugs.python.org/issue32084 [2] https://bugs.python.org/issue32084#msg306545 Conflicts: nova/tests/unit/console/test_websocketproxy.py NOTE(melwitt): The conflict is because change I23ac1cc79482d0fabb359486a4b934463854cae5 (Allow TLS ciphers/protocols to be configurable for console proxies) is not in Train. NOTE(melwitt): The difference from the cherry picked change: HTTPStatus.BAD_REQUEST => 400 is due to the fact that HTTPStatus does not exist in Python 2.7. Reduce mocking in test_reject_open_redirect for compat This is a followup for change Ie36401c782f023d1d5f2623732619105dc2cfa24 to reduce mocking in the unit test coverage for it. While backporting the bug fix, it was found to be incompatible with earlier versions of Python < 3.6 due to a difference in internal implementation [1]. This reduces the mocking in the unit test to be more agnostic to the internals of the StreamRequestHandler (ancestor of SimpleHTTPRequestHandler) and work across Python versions >= 2.7. Related-Bug: #1927677 [1] https://github.com/python/cpython/commit/34eeed42901666fce099947f93dfdfc05411f286 Change-Id: I546d376869a992601b443fb95acf1034da2a8f36 (cherry picked from commit 214cabe6848a1fdb4f5941d994c6cc11107fc4af) (cherry picked from commit 9c2f29783734cb5f9cb05a08d328c10e1d16c4f1) (cherry picked from commit 94e265f3ca615aa18de0081a76975019997b8709) (cherry picked from commit d43b88a33407b1253e7bce70f720a44f7688141f) Change-Id: Ie36401c782f023d1d5f2623732619105dc2cfa24 (cherry picked from commit 781612b33282ed298f742c85dab58a075c8b793e) (cherry picked from commit 470925614223c8dd9b1233f54f5a96c02b2d4f70) (cherry picked from commit 6b70350bdcf59a9712f88b6435ba2c6500133e5b) (cherry picked from commit 719e651e6be277950632e0c2cf5cc9a018344e7b) --- nova/console/websocketproxy.py | 22 +++++++++++++++ nova/tests/unit/console/test_websocketproxy.py | 33 ++++++++++++++++++++++ ...roxy-reject-open-redirect-4ac0a7895acca7eb.yaml | 19 +++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml diff --git a/nova/console/websocketproxy.py b/nova/console/websocketproxy.py index e13b3c0fe1..87d755ec66 100644 --- a/nova/console/websocketproxy.py +++ b/nova/console/websocketproxy.py @@ -19,6 +19,7 @@ Leverages websockify.py by Joel Martin ''' import copy +import os import socket import sys @@ -305,6 +306,27 @@ class NovaProxyRequestHandler(NovaProxyRequestHandlerBase, # Fall back to the websockify <= v0.8.0 'socket' method location. return websockify.WebSocketServer.socket(*args, **kwargs) + def send_head(self): + # This code is copied from this example patch: + # https://bugs.python.org/issue32084#msg306545 + path = self.translate_path(self.path) + 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('//'): + self.send_error(400, + "URI must not start with //") + return None + + return super(NovaProxyRequestHandler, self).send_head() + class NovaWebSocketProxy(websockify.WebSocketProxy): def __init__(self, *args, **kwargs): diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 98e162d59c..45917c2f41 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -15,6 +15,7 @@ """Tests for nova websocketproxy.""" import copy +import io import socket import mock @@ -626,6 +627,38 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): self.wh.server.top_new_client(conn, address) self.assertIsNone(self.wh._compute_rpcapi) + def test_reject_open_redirect(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'' + ] + + 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. + handler = websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data to a 'wfile' + # attribute. + output = io.BytesIO() + handler.wfile = output + # Process the mock_req again to do the capture. + handler.do_GET() + output.seek(0) + result = output.readlines() + + # Verify no redirect happens and instead a 400 Bad Request is returned. + self.assertIn('400 URI must not start with //', result[0].decode()) + class NovaWebsocketSecurityProxyTestCase(test.NoDBTestCase): diff --git a/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml b/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml new file mode 100644 index 0000000000..ce05b9a867 --- /dev/null +++ b/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml @@ -0,0 +1,19 @@ +--- +security: + - | + A vulnerability in the console proxies (novnc, serial, spice) that allowed + open redirection has been `patched`_. The novnc, serial, and spice console + proxies are implemented as websockify servers and the request handler + inherits from the python standard SimpleHTTPRequestHandler. There is a + `known issue`_ in the SimpleHTTPRequestHandler which allows open redirects + by way of URLs in the following format:: + + http://vncproxy.my.domain.com//example.com/%2F.. + + which if visited, will redirect a user to example.com. + + The novnc, serial, and spice console proxies will now reject requests that + pass a redirection URL beginning with "//" with a 400 Bad Request. + + .. _patched: https://bugs.launchpad.net/nova/+bug/1927677 + .. _known issue: https://bugs.python.org/issue32084 -- cgit v1.2.1