diff --git a/ironic/console/securityproxy/rfb.py b/ironic/console/securityproxy/rfb.py index 566e1e682b..d6ea908691 100644 --- a/ironic/console/securityproxy/rfb.py +++ b/ironic/console/securityproxy/rfb.py @@ -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) diff --git a/ironic/tests/unit/console/securityproxy/test_rfb.py b/ironic/tests/unit/console/securityproxy/test_rfb.py index 5b6bd39754..828cf6fa4b 100644 --- a/ironic/tests/unit/console/securityproxy/test_rfb.py +++ b/ironic/tests/unit/console/securityproxy/test_rfb.py @@ -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): diff --git a/releasenotes/notes/rfb-reason-text-resource-exhaustion-5fc9485d9b0d6698.yaml b/releasenotes/notes/rfb-reason-text-resource-exhaustion-5fc9485d9b0d6698.yaml new file mode 100644 index 0000000000..3b9189ca6d --- /dev/null +++ b/releasenotes/notes/rfb-reason-text-resource-exhaustion-5fc9485d9b0d6698.yaml @@ -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 `_. +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 `_.