From 27e9b8a48632696311bd8c4af93ec52a49ba5e09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Langa?= Date: Tue, 22 Aug 2023 19:53:15 +0200 Subject: [PATCH 1/3] gh-108310: Fix CVE-2023-40217: Check for & avoid the ssl pre-close flaw (#108315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instances of `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. The vulnerability is caused when a socket is connected, data is sent by the malicious peer and stored in a buffer, and then the malicious peer closes the socket within a small timing window before the other peers’ TLS handshake can begin. After this sequence of events the closed socket will not immediately attempt a TLS handshake due to not being connected but will also allow the buffered data to be read as if a successful TLS handshake had occurred. Co-authored-by: Gregory P. Smith [Google LLC] ----- Notable adjustments for Python 2.7: Use alternative for self.getblocking(), which was added in Python 3.7 see: https://docs.python.org/3/library/socket.html#socket.socket.getblocking Set self._sslobj early to avoid AttributeError Use SSLError where necessary (it is not a subclass of OSError) --- Lib/ssl.py | 32 +++ Lib/test/test_ssl.py | 210 ++++++++++++++++++ ...-08-22-17-39-12.gh-issue-108310.fVM3sg.rst | 7 + 3 files changed, 249 insertions(+) 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 0bb43a4a4de..de9ce6bc134 100644 --- a/Lib/ssl.py +++ b/Lib/ssl.py @@ -523,6 +523,7 @@ class SSLSocket(socket): server_hostname=None, _context=None): + self._sslobj = None self._makefile_refs = 0 if _context: self._context = _context @@ -573,6 +574,8 @@ class SSLSocket(socket): self.do_handshake_on_connect = do_handshake_on_connect self.suppress_ragged_eofs = suppress_ragged_eofs + sock_timeout = sock.gettimeout() + # See if we are connected try: self.getpeername() @@ -580,9 +583,38 @@ class SSLSocket(socket): if e.errno != errno.ENOTCONN: raise connected = False + blocking = (sock.gettimeout() != 0) + 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 socket_error 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._closed = False self._sslobj = None self._connected = connected diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index ef2e59c1d15..fd657761a05 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -8,9 +8,11 @@ from test.script_helper import assert_python_ok import asyncore import socket import select +import struct import time import datetime import gc +import httplib import os import errno import pprint @@ -3240,6 +3242,213 @@ else: self.assertRaises(ValueError, s.read, 1024) self.assertRaises(ValueError, s.write, b'hello') +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() + threading.Thread.__init__(self, name=name) + + def __enter__(self): + self.start() + return self + + def __exit__(self, *args): + try: + if self.listener: + self.listener.close() + except ssl.SSLError: + 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) + threading.Thread.start(self) + + def run(self): + conn, address = self.listener.accept() + self.listener.close() + with closing(conn): + if self.call_after_accept(conn): + return + try: + tls_socket = self.ssl_ctx.wrap_socket(conn, server_side=True) + except ssl.SSLError as err: + 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("Could not recreate conditions on {}: \ + err={}".format(sys.platform,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 closing(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, 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 closing(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 ssl.SSLError as err: + 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, 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(httplib.HTTPSConnection): + def connect(self): + httplib.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( + "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(ssl.SSLError) as err_ctx: + connection.request("HEAD", "/test", headers={"Host": "localhost"}) + response = connection.getresponse() + def test_main(verbose=False): if support.verbose: @@ -3274,6 +3483,7 @@ def test_main(verbose=False): raise support.TestFailed("Can't read certificate file %r" % filename) tests = [ContextTests, BasicTests, BasicSocketTests, SSLErrorTests] + tests += [TestPreHandshakeClose] if support.is_resource_enabled('network'): tests.append(NetworkedTests) 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 00000000000..403c77a9d48 --- /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. -- 2.41.0 From 8ea38387426d3d2dd4c38f5ab7c26ee0ab0fb242 Mon Sep 17 00:00:00 2001 From: "Miss Islington (bot)" <31488909+miss-islington@users.noreply.github.com> Date: Wed, 23 Aug 2023 03:10:56 -0700 Subject: [PATCH 2/3] gh-108342: Break ref cycle in SSLSocket._create() exc (GH-108344) (#108352) Explicitly break a reference cycle when SSLSocket._create() raises an exception. Clear the variable storing the exception, since the exception traceback contains the variables and so creates a reference cycle. This test leak was introduced by the test added for the fix of GH-108310. (cherry picked from commit 64f99350351bc46e016b2286f36ba7cd669b79e3) Co-authored-by: Victor Stinner --- Lib/ssl.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Lib/ssl.py b/Lib/ssl.py index de9ce6bc134..ac734b4f2ad 100644 --- a/Lib/ssl.py +++ b/Lib/ssl.py @@ -610,7 +610,11 @@ class SSLSocket(socket): self.close() except OSError: pass - raise notconn_pre_handshake_data_error + try: + raise notconn_pre_handshake_data_error + finally: + # Explicitly break the reference cycle. + notconn_pre_handshake_data_error = None else: connected = True -- 2.41.0 From 845491ee514d6489466664aeef377fe9155f8e83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Langa?= Date: Thu, 24 Aug 2023 12:09:30 +0200 Subject: [PATCH 3/3] [3.8] gh-108342: Make ssl TestPreHandshakeClose more reliable (GH-108370) (#108408) * In preauth tests of test_ssl, explicitly break reference cycles invoving SingleConnectionTestServerThread to make sure that the thread is deleted. Otherwise, the test marks the environment as altered because the threading module sees a "dangling thread" (SingleConnectionTestServerThread). This test leak was introduced by the test added for the fix of issue gh-108310. * Use support.SHORT_TIMEOUT instead of hardcoded 1.0 or 2.0 seconds timeout. * SingleConnectionTestServerThread.run() catchs TimeoutError * Fix a race condition (missing synchronization) in test_preauth_data_to_tls_client(): the server now waits until the client connect() completed in call_after_accept(). * test_https_client_non_tls_response_ignored() calls server.join() explicitly. * Replace "localhost" with server.listener.getsockname()[0]. (cherry picked from commit 592bacb6fc0833336c0453e818e9b95016e9fd47) --- Lib/test/support/__init__.py | 2 + Lib/test/test_ssl.py | 105 ++++++++++++++++++++--------------- 2 files changed, 62 insertions(+), 45 deletions(-) diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index ccc11c1b4b0..d6ee9934f04 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -47,6 +47,8 @@ __all__ = ["Error", "TestFailed", "TestDidNotRun", "ResourceDenied", "import_mod "strip_python_stderr", "IPV6_ENABLED", "run_with_tz", "SuppressCrashReport"] +SHORT_TIMEOUT = 30.0 # Added to make backporting from 3.x easier + class Error(Exception): """Base class for regression test exceptions.""" diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index fd657761a05..0f5aa37a0c1 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -3252,12 +3252,16 @@ class TestPreHandshakeClose(unittest.TestCase): class SingleConnectionTestServerThread(threading.Thread): - def __init__(self, name, call_after_accept): + def __init__(self, name, call_after_accept, timeout=None): 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() + if timeout is None: + self.timeout = support.SHORT_TIMEOUT + else: + self.timeout = timeout threading.Thread.__init__(self, name=name) def __enter__(self): @@ -3280,13 +3284,22 @@ class TestPreHandshakeClose(unittest.TestCase): 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.settimeout(self.timeout) self.listener.listen(1) threading.Thread.start(self) def run(self): - conn, address = self.listener.accept() - self.listener.close() + try: + conn, address = self.listener.accept() + except OSError as e: + if e.errno == errno.ETIMEDOUT: + # on timeout, just close the listener + return + else: + raise + finally: + self.listener.close() + with closing(conn): if self.call_after_accept(conn): return @@ -3300,33 +3313,13 @@ class TestPreHandshakeClose(unittest.TestCase): 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("Could not recreate conditions on {}: \ - err={}".format(sys.platform,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): + if not ready_for_server_wrap_socket.wait(support.SHORT_TIMEOUT): raise RuntimeError("wrap_socket event never set, test may fail.") return False # Tell the server thread to continue. @@ -3348,18 +3341,28 @@ class TestPreHandshakeClose(unittest.TestCase): ready_for_server_wrap_socket.set() server.join() + wrap_error = server.wrap_error - self.assertEqual(b"", server.received_data) - 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") + try: + self.assertEqual(b"", server.received_data) + 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") + finally: + # gh-108342: Explicitly break the reference cycle + wrap_error = None + server = None def test_preauth_data_to_tls_client(self): + server_can_continue_with_wrap_socket = threading.Event() client_can_continue_with_wrap_socket = threading.Event() def call_after_accept(conn_to_client): + if not server_can_continue_with_wrap_socket.wait(support.SHORT_TIMEOUT): + print("ERROR: test client took too long") + # 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( @@ -3381,8 +3384,10 @@ class TestPreHandshakeClose(unittest.TestCase): with closing(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.") + server_can_continue_with_wrap_socket.set() + + if not client_can_continue_with_wrap_socket.wait(support.SHORT_TIMEOUT): + self.fail("test server took too long") ssl_ctx = ssl.create_default_context() try: tls_client = ssl_ctx.wrap_socket( @@ -3396,22 +3401,29 @@ class TestPreHandshakeClose(unittest.TestCase): tls_client.close() server.join() - self.assertEqual(b"", received_data) - 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") + try: + self.assertEqual(b"", received_data) + 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") + finally: + # gh-108342: Explicitly break the reference cycle + wrap_error = None + server = None def test_https_client_non_tls_response_ignored(self): - server_responding = threading.Event() class SynchronizedHTTPSConnection(httplib.HTTPSConnection): def connect(self): + # Call clear text HTTP connect(), not the encrypted HTTPS (TLS) + # connect(): wrap_socket() is called manually below. httplib.HTTPConnection.connect(self) + # Wait for our fault injection server to have done its thing. - if not server_responding.wait(1.0) and support.verbose: + if not server_responding.wait(support.SHORT_TIMEOUT) and support.verbose: sys.stdout.write("server_responding event never set.") self.sock = self._context.wrap_socket( self.sock, server_hostname=self.host) @@ -3426,29 +3438,32 @@ class TestPreHandshakeClose(unittest.TestCase): server_responding.set() return True # Tell the server to stop. + timeout = 2.0 server = self.SingleConnectionTestServerThread( call_after_accept=call_after_accept, - name="non_tls_http_RST_responder") + name="non_tls_http_RST_responder", + timeout=timeout) 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( - "localhost", + server.listener.getsockname()[0], port=server.port, context=ssl.create_default_context(), - timeout=2.0, + timeout=timeout, ) # 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(ssl.SSLError) as err_ctx: + with self.assertRaises(ssl.SSLError): connection.request("HEAD", "/test", headers={"Host": "localhost"}) response = connection.getresponse() + server.join() def test_main(verbose=False): if support.verbose: -- 2.41.0