From 0cf9a729e2bf414c01c87c9ee15d4c21938ade90 Mon Sep 17 00:00:00 2001 From: Frederick Price Date: Mon, 11 Sep 2023 16:49:13 -0400 Subject: [PATCH 1/2] Cherry picked b4bcc06a9cfe13d96d5270809d963f8ba278f89b, with some fixups --- Lib/ssl.py | 31 +- Lib/test/test_ssl.py | 367 ++++++++++++++++++ ...-08-22-17-39-12.gh-issue-108310.fVM3sg.rst | 7 + 3 files changed, 404 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Security/2023-08-22-17-39-12.gh-issue-108310.fVM3sg.rst diff --git a/Lib/ssl.py b/Lib/ssl.py index b89c9ad344822a3..4142b8d98b8ce4f 100644 --- a/Lib/ssl.py +++ b/Lib/ssl.py @@ -832,7 +832,7 @@ def _create(cls, sock, server_side=False, do_handshake_on_connect=True, ) self = cls.__new__(cls, **kwargs) super(SSLSocket, self).__init__(**kwargs) - self.settimeout(sock.gettimeout()) + sock_timeout = sock.gettimeout() sock.detach() self._context = context @@ -851,9 +851,38 @@ def _create(cls, sock, server_side=False, do_handshake_on_connect=True, if e.errno != errno.ENOTCONN: raise connected = False + blocking = self.getblocking() + self.setblocking(False) + try: + # We are not connected so this is not supposed to block, but + # testing revealed otherwise on macOS and Windows so we do + # the non-blocking dance regardless. Our raise when any data + # is found means consuming the data is harmless. + notconn_pre_handshake_data = self.recv(1) + except OSError as e: + # EINVAL occurs for recv(1) on non-connected on unix sockets. + if e.errno not in (errno.ENOTCONN, errno.EINVAL): + raise + notconn_pre_handshake_data = b'' + self.setblocking(blocking) + if notconn_pre_handshake_data: + # This prevents pending data sent to the socket before it was + # closed from escaping to the caller who could otherwise + # presume it came through a successful TLS connection. + reason = "Closed before TLS handshake with data in recv buffer." + notconn_pre_handshake_data_error = SSLError(e.errno, reason) + # Add the SSLError attributes that _ssl.c always adds. + notconn_pre_handshake_data_error.reason = reason + notconn_pre_handshake_data_error.library = None + try: + self.close() + except OSError: + pass + raise notconn_pre_handshake_data_error else: connected = True + self.settimeout(sock_timeout) # Must come after setblocking() calls. self._connected = connected if connected: # create the SSL object diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index 1418a84d1d1c117..f1a31d992175dca 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -3,11 +3,14 @@ import sys import unittest from test import support +import re import socket import select +import struct import time import datetime import gc +import http.client import os import errno import pprint @@ -4612,6 +4615,370 @@ def test_bpo37428_pha_cert_none(self): self.assertEqual(s.getpeercert(), {}) +HAS_KEYLOG = hasattr(ssl.SSLContext, 'keylog_filename') +requires_keylog = unittest.skipUnless( + HAS_KEYLOG, 'test requires OpenSSL 1.1.1 with keylog callback') + +class TestSSLDebug(unittest.TestCase): + + def keylog_lines(self, fname=support.TESTFN): + with open(fname) as f: + return len(list(f)) + + @requires_keylog + def test_keylog_defaults(self): + self.addCleanup(support.unlink, support.TESTFN) + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) + self.assertEqual(ctx.keylog_filename, None) + + self.assertFalse(os.path.isfile(support.TESTFN)) + ctx.keylog_filename = support.TESTFN + self.assertEqual(ctx.keylog_filename, support.TESTFN) + self.assertTrue(os.path.isfile(support.TESTFN)) + self.assertEqual(self.keylog_lines(), 1) + + ctx.keylog_filename = None + self.assertEqual(ctx.keylog_filename, None) + + with self.assertRaises((IsADirectoryError, PermissionError)): + # Windows raises PermissionError + ctx.keylog_filename = os.path.dirname( + os.path.abspath(support.TESTFN)) + + with self.assertRaises(TypeError): + ctx.keylog_filename = 1 + + @requires_keylog + def test_keylog_filename(self): + self.addCleanup(support.unlink, support.TESTFN) + client_context, server_context, hostname = testing_context() + + client_context.keylog_filename = support.TESTFN + server = ThreadedEchoServer(context=server_context, chatty=False) + with server: + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as s: + s.connect((HOST, server.port)) + # header, 5 lines for TLS 1.3 + self.assertEqual(self.keylog_lines(), 6) + + client_context.keylog_filename = None + server_context.keylog_filename = support.TESTFN + server = ThreadedEchoServer(context=server_context, chatty=False) + with server: + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as s: + s.connect((HOST, server.port)) + self.assertGreaterEqual(self.keylog_lines(), 11) + + client_context.keylog_filename = support.TESTFN + server_context.keylog_filename = support.TESTFN + server = ThreadedEchoServer(context=server_context, chatty=False) + with server: + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as s: + s.connect((HOST, server.port)) + self.assertGreaterEqual(self.keylog_lines(), 21) + + client_context.keylog_filename = None + server_context.keylog_filename = None + + @requires_keylog + @unittest.skipIf(sys.flags.ignore_environment, + "test is not compatible with ignore_environment") + def test_keylog_env(self): + self.addCleanup(support.unlink, support.TESTFN) + with unittest.mock.patch.dict(os.environ): + os.environ['SSLKEYLOGFILE'] = support.TESTFN + self.assertEqual(os.environ['SSLKEYLOGFILE'], support.TESTFN) + + ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) + self.assertEqual(ctx.keylog_filename, None) + + ctx = ssl.create_default_context() + self.assertEqual(ctx.keylog_filename, support.TESTFN) + + ctx = ssl._create_stdlib_context() + self.assertEqual(ctx.keylog_filename, support.TESTFN) + + def test_msg_callback(self): + client_context, server_context, hostname = testing_context() + + def msg_cb(conn, direction, version, content_type, msg_type, data): + pass + + self.assertIs(client_context._msg_callback, None) + client_context._msg_callback = msg_cb + self.assertIs(client_context._msg_callback, msg_cb) + with self.assertRaises(TypeError): + client_context._msg_callback = object() + + def test_msg_callback_tls12(self): + client_context, server_context, hostname = testing_context() + client_context.options |= ssl.OP_NO_TLSv1_3 + + msg = [] + + def msg_cb(conn, direction, version, content_type, msg_type, data): + self.assertIsInstance(conn, ssl.SSLSocket) + self.assertIsInstance(data, bytes) + self.assertIn(direction, {'read', 'write'}) + msg.append((direction, version, content_type, msg_type)) + + client_context._msg_callback = msg_cb + + server = ThreadedEchoServer(context=server_context, chatty=False) + with server: + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as s: + s.connect((HOST, server.port)) + + self.assertIn( + ("read", TLSVersion.TLSv1_2, _TLSContentType.HANDSHAKE, + _TLSMessageType.SERVER_KEY_EXCHANGE), + msg + ) + self.assertIn( + ("write", TLSVersion.TLSv1_2, _TLSContentType.CHANGE_CIPHER_SPEC, + _TLSMessageType.CHANGE_CIPHER_SPEC), + msg + ) + + def test_msg_callback_deadlock_bpo43577(self): + client_context, server_context, hostname = testing_context() + server_context2 = testing_context()[1] + + def msg_cb(conn, direction, version, content_type, msg_type, data): + pass + + def sni_cb(sock, servername, ctx): + sock.context = server_context2 + + server_context._msg_callback = msg_cb + server_context.sni_callback = sni_cb + + server = ThreadedEchoServer(context=server_context, chatty=False) + with server: + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as s: + s.connect((HOST, server.port)) + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as s: + s.connect((HOST, server.port)) + + +def set_socket_so_linger_on_with_zero_timeout(sock): + sock.setsockopt(socket.SOL_SOCKET, socket.SO_LINGER, struct.pack('ii', 1, 0)) + + +class TestPreHandshakeClose(unittest.TestCase): + """Verify behavior of close sockets with received data before to the handshake. + """ + + class SingleConnectionTestServerThread(threading.Thread): + + def __init__(self, *, name, call_after_accept): + self.call_after_accept = call_after_accept + self.received_data = b'' # set by .run() + self.wrap_error = None # set by .run() + self.listener = None # set by .start() + self.port = None # set by .start() + super().__init__(name=name) + + def __enter__(self): + self.start() + return self + + def __exit__(self, *args): + try: + if self.listener: + self.listener.close() + except OSError: + pass + self.join() + self.wrap_error = None # avoid dangling references + + def start(self): + self.ssl_ctx = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) + self.ssl_ctx.verify_mode = ssl.CERT_REQUIRED + self.ssl_ctx.load_verify_locations(cafile=ONLYCERT) + self.ssl_ctx.load_cert_chain(certfile=ONLYCERT, keyfile=ONLYKEY) + self.listener = socket.socket() + self.port = support.bind_port(self.listener) + self.listener.settimeout(2.0) + self.listener.listen(1) + super().start() + + def run(self): + conn, address = self.listener.accept() + self.listener.close() + with conn: + if self.call_after_accept(conn): + return + try: + tls_socket = self.ssl_ctx.wrap_socket(conn, server_side=True) + except OSError as err: # ssl.SSLError inherits from OSError + self.wrap_error = err + else: + try: + self.received_data = tls_socket.recv(400) + except OSError: + pass # closed, protocol error, etc. + + def non_linux_skip_if_other_okay_error(self, err): + if sys.platform == "linux": + return # Expect the full test setup to always work on Linux. + if (isinstance(err, ConnectionResetError) or + (isinstance(err, OSError) and err.errno == errno.EINVAL) or + re.search('wrong.version.number', getattr(err, "reason", ""), re.I)): + # On Windows the TCP RST leads to a ConnectionResetError + # (ECONNRESET) which Linux doesn't appear to surface to userspace. + # If wrap_socket() winds up on the "if connected:" path and doing + # the actual wrapping... we get an SSLError from OpenSSL. Typically + # WRONG_VERSION_NUMBER. While appropriate, neither is the scenario + # we're specifically trying to test. The way this test is written + # is known to work on Linux. We'll skip it anywhere else that it + # does not present as doing so. + self.skipTest(f"Could not recreate conditions on {sys.platform}:" + f" {err=}") + # If maintaining this conditional winds up being a problem. + # just turn this into an unconditional skip anything but Linux. + # The important thing is that our CI has the logic covered. + + def test_preauth_data_to_tls_server(self): + server_accept_called = threading.Event() + ready_for_server_wrap_socket = threading.Event() + + def call_after_accept(unused): + server_accept_called.set() + if not ready_for_server_wrap_socket.wait(2.0): + raise RuntimeError("wrap_socket event never set, test may fail.") + return False # Tell the server thread to continue. + + server = self.SingleConnectionTestServerThread( + call_after_accept=call_after_accept, + name="preauth_data_to_tls_server") + server.__enter__() # starts it + self.addCleanup(server.__exit__) # ... & unittest.TestCase stops it. + + with socket.socket() as client: + client.connect(server.listener.getsockname()) + # This forces an immediate connection close via RST on .close(). + set_socket_so_linger_on_with_zero_timeout(client) + client.setblocking(False) + + server_accept_called.wait() + client.send(b"DELETE /data HTTP/1.0\r\n\r\n") + client.close() # RST + + ready_for_server_wrap_socket.set() + server.join() + wrap_error = server.wrap_error + self.assertEqual(b"", server.received_data) + self.assertIsInstance(wrap_error, OSError) # All platforms. + self.non_linux_skip_if_other_okay_error(wrap_error) + self.assertIsInstance(wrap_error, ssl.SSLError) + self.assertIn("before TLS handshake with data", wrap_error.args[1]) + self.assertIn("before TLS handshake with data", wrap_error.reason) + self.assertNotEqual(0, wrap_error.args[0]) + self.assertIsNone(wrap_error.library, msg="attr must exist") + + def test_preauth_data_to_tls_client(self): + client_can_continue_with_wrap_socket = threading.Event() + + def call_after_accept(conn_to_client): + # This forces an immediate connection close via RST on .close(). + set_socket_so_linger_on_with_zero_timeout(conn_to_client) + conn_to_client.send( + b"HTTP/1.0 307 Temporary Redirect\r\n" + b"Location: https://example.com/someone-elses-server\r\n" + b"\r\n") + conn_to_client.close() # RST + client_can_continue_with_wrap_socket.set() + return True # Tell the server to stop. + + server = self.SingleConnectionTestServerThread( + call_after_accept=call_after_accept, + name="preauth_data_to_tls_client") + server.__enter__() # starts it + self.addCleanup(server.__exit__) # ... & unittest.TestCase stops it. + + # Redundant; call_after_accept sets SO_LINGER on the accepted conn. + set_socket_so_linger_on_with_zero_timeout(server.listener) + + with socket.socket() as client: + client.connect(server.listener.getsockname()) + if not client_can_continue_with_wrap_socket.wait(2.0): + self.fail("test server took too long.") + ssl_ctx = ssl.create_default_context() + try: + tls_client = ssl_ctx.wrap_socket( + client, server_hostname="localhost") + except OSError as err: # SSLError inherits from OSError + wrap_error = err + received_data = b"" + else: + wrap_error = None + received_data = tls_client.recv(400) + tls_client.close() + + server.join() + self.assertEqual(b"", received_data) + self.assertIsInstance(wrap_error, OSError) # All platforms. + self.non_linux_skip_if_other_okay_error(wrap_error) + self.assertIsInstance(wrap_error, ssl.SSLError) + self.assertIn("before TLS handshake with data", wrap_error.args[1]) + self.assertIn("before TLS handshake with data", wrap_error.reason) + self.assertNotEqual(0, wrap_error.args[0]) + self.assertIsNone(wrap_error.library, msg="attr must exist") + + def test_https_client_non_tls_response_ignored(self): + + server_responding = threading.Event() + + class SynchronizedHTTPSConnection(http.client.HTTPSConnection): + def connect(self): + http.client.HTTPConnection.connect(self) + # Wait for our fault injection server to have done its thing. + if not server_responding.wait(1.0) and support.verbose: + sys.stdout.write("server_responding event never set.") + self.sock = self._context.wrap_socket( + self.sock, server_hostname=self.host) + + def call_after_accept(conn_to_client): + # This forces an immediate connection close via RST on .close(). + set_socket_so_linger_on_with_zero_timeout(conn_to_client) + conn_to_client.send( + b"HTTP/1.0 402 Payment Required\r\n" + b"\r\n") + conn_to_client.close() # RST + server_responding.set() + return True # Tell the server to stop. + + server = self.SingleConnectionTestServerThread( + call_after_accept=call_after_accept, + name="non_tls_http_RST_responder") + server.__enter__() # starts it + self.addCleanup(server.__exit__) # ... & unittest.TestCase stops it. + # Redundant; call_after_accept sets SO_LINGER on the accepted conn. + set_socket_so_linger_on_with_zero_timeout(server.listener) + + connection = SynchronizedHTTPSConnection( + f"localhost", + port=server.port, + context=ssl.create_default_context(), + timeout=2.0, + ) + # There are lots of reasons this raises as desired, long before this + # test was added. Sending the request requires a successful TLS wrapped + # socket; that fails if the connection is broken. It may seem pointless + # to test this. It serves as an illustration of something that we never + # want to happen... properly not happening. + with self.assertRaises(OSError) as err_ctx: + connection.request("HEAD", "/test", headers={"Host": "localhost"}) + response = connection.getresponse() + + def test_main(verbose=False): if support.verbose: import warnings diff --git a/Misc/NEWS.d/next/Security/2023-08-22-17-39-12.gh-issue-108310.fVM3sg.rst b/Misc/NEWS.d/next/Security/2023-08-22-17-39-12.gh-issue-108310.fVM3sg.rst new file mode 100644 index 000000000000000..403c77a9d480ee7 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2023-08-22-17-39-12.gh-issue-108310.fVM3sg.rst @@ -0,0 +1,7 @@ +Fixed an issue where instances of :class:`ssl.SSLSocket` were vulnerable to +a bypass of the TLS handshake and included protections (like certificate +verification) and treating sent unencrypted data as if it were +post-handshake TLS encrypted data. Security issue reported as +`CVE-2023-40217 +`_ by +Aapo Oksman. Patch by Gregory P. Smith. From 5071ac4b67924880b4ef1bd0a5cdb8500273a4a2 Mon Sep 17 00:00:00 2001 From: Frederick Price Date: Mon, 11 Sep 2023 18:00:56 -0400 Subject: [PATCH 2/2] Add in CVE fix documentation --- Misc/NEWS.d/3.7.17.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Misc/NEWS.d/3.7.17.rst b/Misc/NEWS.d/3.7.17.rst index 201dff382c07bcf..2383769b3122bff 100644 --- a/Misc/NEWS.d/3.7.17.rst +++ b/Misc/NEWS.d/3.7.17.rst @@ -1,3 +1,18 @@ +.. date: 2023-08-22-17-39-12 +.. gh-issue: 108310 +.. nonce: fVM3sg +.. release date: 2023-08-24 +.. section: Security + +Fixed an issue where instances of :class:`ssl.SSLSocket` were vulnerable to +a bypass of the TLS handshake and included protections (like certificate +verification) and treating sent unencrypted data as if it were +post-handshake TLS encrypted data. Security issue reported as +`CVE-2023-40217 +`_ by Aapo +Oksman. Patch by Gregory P. Smith. + +.. .. date: 2023-06-05-04-07-52 .. gh-issue: 103142 .. nonce: GLWDMX