Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ironic/console/securityproxy/rfb.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,14 @@ def recv(sock, num):
# Decode the reason why the request failed
reason_len_raw = recv(host_sock, 4)
reason_len = struct.unpack('!I', reason_len_raw)[0]
reason = recv(host_sock, reason_len)
if reason_len <= 256:
reason = recv(host_sock, reason_len)
else:
# NOTE(danms): If the reason is too long, we just assume a
# generic failure instead of reading up to 2^32 bytes to avoid
# a potential exhaustion attack.
reason = b'Unable to negotiate security with server'
reason_len_raw = struct.pack('!I', len(reason))

tenant_sock.sendall(auth.AUTH_STATUS_FAIL
+ reason_len_raw + reason)
Expand Down
24 changes: 24 additions & 0 deletions ironic/tests/unit/console/securityproxy/test_rfb.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,30 @@ def test_fails_on_sec_type_cnt_zero(self):

self._assert_expected_calls()

def test_fails_on_sec_type_cnt_zero_oversized_reason(self):
"""Validate behavior if a server returns an oversized reason string.

If the server sends a reason length greater than 256 bytes, we should
not attempt to read the full payload (which could be up to 2^32 bytes)
and instead substitute a generic failure message.
"""
import struct
self.proxy._fail = mock.Mock()
self._version_handshake()
self._expect_host_recv(1, "\x00")
# Send a reason length of 1000, which exceeds the 256-byte limit
self._expect_host_recv(4, struct.pack('!I', 1000))
generic_reason = b'Unable to negotiate security with server'
generic_reason_len = struct.pack('!I', len(generic_reason))
self._expect_tenant_send(
auth.AUTH_STATUS_FAIL + generic_reason_len + generic_reason)
ex = self.assertRaises(exception.SecurityProxyNegotiationFailed,
self.proxy.connect,
self.tenant_sock,
self.host_sock)
self.assertIn('Unable to negotiate security', str(ex))
self._assert_expected_calls()

@mock.patch.object(authnone.RFBAuthSchemeNone, "security_handshake",
autospec=True)
def test_full_run(self, mock_handshake):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
security:
- |
Fixes a vulnerability where security proxy could be crashed if attacker
controls host VNC server. The exploit requires that VNC server first
informs the client that there is 0 security types available and then sends
a reason with reason length for this. There is no checks for reason length,
so the attacker can make the security proxy to reserve up to 4 GB of memory
and causing it to crash. The security proxy is shared between all tenants,
so crashing the proxy denies console access for all tenants. More
information in the bug description
`bug 2155052 <https://bugs.launchpad.net/ironic/+bug/2155052>`_.
fixes:
- |
Fixes a vulnerability where security proxy could be crashed if the VNC
server sends unreasonable large reason length. The security proxy is shared
between all tenants, so crashing the proxy denies console access for all
tenants. More information in the bug description
`bug 2155052 <https://bugs.launchpad.net/ironic/+bug/2155052>`_.