Browse Source

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] 34eeed4290

Change-Id: I546d376869a992601b443fb95acf1034da2a8f36
(cherry picked from commit 214cabe684)
(cherry picked from commit 9c2f297837)
(cherry picked from commit 94e265f3ca)
(cherry picked from commit d43b88a334)

Change-Id: Ie36401c782f023d1d5f2623732619105dc2cfa24
(cherry picked from commit 781612b332)
(cherry picked from commit 4709256142)
(cherry picked from commit 6b70350bdc)
(cherry picked from commit 719e651e6b)
changes/07/791807/6
melanie witt 1 year ago committed by Elod Illes
parent
commit
04d48527b6
  1. 22
      nova/console/websocketproxy.py
  2. 33
      nova/tests/unit/console/test_websocketproxy.py
  3. 19
      releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml

22
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):

33
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):

19
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
Loading…
Cancel
Save