From d079ce3d362bd36ef3c95b50215e249afdd4a200 Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Mon, 22 Jul 2024 13:11:54 -0500 Subject: [PATCH 1/6] Authenticate socket connection for `socket.socketpair()` fallback Co-authored-by: Gregory P. Smith --- Lib/socket.py | 95 +++++++++++++++---- Lib/test/test_socket.py | 92 +++++++++++++++++- ...-07-22-13-11-28.gh-issue-122133.0mPeta.rst | 5 + 3 files changed, 168 insertions(+), 24 deletions(-) create mode 100644 Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst diff --git a/Lib/socket.py b/Lib/socket.py index 524ce1361b9091..0e738926124c92 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -52,7 +52,7 @@ import _socket from _socket import * -import os, sys, io, selectors +import os, sys, io, selectors, time from enum import IntEnum, IntFlag try: @@ -628,28 +628,81 @@ def socketpair(family=AF_INET, type=SOCK_STREAM, proto=0): if proto != 0: raise ValueError("Only protocol zero is supported") - # We create a connected TCP socket. Note the trick with - # setblocking(False) that prevents us from having to create a thread. - lsock = socket(family, type, proto) - try: - lsock.bind((host, 0)) - lsock.listen() - # On IPv6, ignore flow_info and scope_id - addr, port = lsock.getsockname()[:2] - csock = socket(family, type, proto) + import secrets # Delay import until actually needed. + auth_len = secrets.DEFAULT_ENTROPY + reason = "unknown" + for _ in range(5): + # We do not try forever, that'd just provide another process + # the ability to make us waste CPU retrying. In all normal + # circumstances the first connection auth attempt succeeds. + s_auth = secrets.token_bytes() + c_auth = secrets.token_bytes() + + # We create a connected TCP socket. Note the trick with + # setblocking(False) prevents us from having to create a thread. + csock = None + ssock = None + reason = "unknown" + lsock = socket(family, type, proto) try: - csock.setblocking(False) + lsock.bind((host, 0)) + lsock.listen() + # On IPv6, ignore flow_info and scope_id + addr, port = lsock.getsockname()[:2] + csock = socket(family, type, proto) try: - csock.connect((addr, port)) - except (BlockingIOError, InterruptedError): - pass - csock.setblocking(True) - ssock, _ = lsock.accept() - except: - csock.close() - raise - finally: - lsock.close() + csock.setblocking(False) + try: + csock.connect((addr, port)) + except (BlockingIOError, InterruptedError): + pass + csock.setblocking(True) + ssock, _ = lsock.accept() + except Exception: + csock.close() + raise + + def authenticate_socket_conn(send_sock, recv_sock, auth_bytes): + nonlocal auth_len + data_buf = bytearray(auth_len) + data_mem = memoryview(data_buf) + data_len = 0 + + # Send the authentication bytes. + if send_sock.send(auth_bytes) != auth_len: + raise ConnectionError("send() sent too few auth bytes.") + + # Attempt to read the authentication bytes from the socket. + max_auth_time = time.monotonic() + 3.0 + while time.monotonic() < max_auth_time and data_len < auth_len: + bytes_received = recv_sock.recv_into(data_mem, auth_len - data_len) + if bytes_received == 0: + break # Connection closed. + data_len += bytes_received + data_mem = data_mem[bytes_received:] + + # Check that the authentication bytes match. + if len(data_buf) != auth_len: + raise ConnectionError("recv() got too few auth bytes.") + if bytes(data_buf) != auth_bytes: + raise ConnectionError(f"Mismatched auth token.") + + # Authenticating avoids using a connection from something else + # able to connect to {host}:{port} instead of us. + try: + authenticate_socket_conn(ssock, csock, s_auth) + authenticate_socket_conn(csock, ssock, c_auth) + except OSError as exc: + csock.close() + ssock.close() + reason = str(exc) + continue + # authentication successful, both sockets are our process. + break + finally: + lsock.close() + else: + raise ConnectionError(f"socketpair authentication failed: {reason}") return (ssock, csock) __all__.append("socketpair") diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index ce0f64b43ed49f..b70a28a1c790b3 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -592,19 +592,27 @@ class SocketPairTest(unittest.TestCase, ThreadableTest): def __init__(self, methodName='runTest'): unittest.TestCase.__init__(self, methodName=methodName) ThreadableTest.__init__(self) + self.cli = None + self.serv = None + + def call_socketpair(self): + # To be overridden by some child classes. + return socket.socketpair() def setUp(self): - self.serv, self.cli = socket.socketpair() + self.serv, self.cli = self.call_socketpair() def tearDown(self): - self.serv.close() + if self.serv: + self.serv.close() self.serv = None def clientSetUp(self): pass def clientTearDown(self): - self.cli.close() + if self.cli: + self.cli.close() self.cli = None ThreadableTest.clientTearDown(self) @@ -4852,6 +4860,84 @@ def _testSend(self): self.assertEqual(msg, MSG) +class PurePythonSocketPairTest(SocketPairTest): + + # Explicitly use socketpair AF_INET or AF_INET6 to to ensure that is the + # code path we're using regardless platform is the pure python one where + # `_socket.socketpair` does not exist. (AF_INET does not work with + # _socket.socketpair on many platforms). + def call_socketpair(self): + # called by super().setUp(). + try: + return socket.socketpair(socket.AF_INET6) + except OSError: + return socket.socketpair(socket.AF_INET) + + # Local imports in this class make for easy security fix backporting. + + def setUp(self): + import _socket + self._orig_sp = getattr(_socket, 'socketpair', None) + if self._orig_sp is not None: + # This forces the version using the non-OS provided socketpair + # emulation via an AF_INET socket in Lib/socket.py. + del _socket.socketpair + import importlib + global socket + socket = importlib.reload(socket) + else: + pass # This platform already uses the non-OS provided version. + super().setUp() + + def tearDown(self): + super().tearDown() + import _socket + if self._orig_sp is not None: + # Restore the default socket.socketpair definition. + _socket.socketpair = self._orig_sp + import importlib + global socket + socket = importlib.reload(socket) + + def test_recv(self): + msg = self.serv.recv(1024) + self.assertEqual(msg, MSG) + + def _test_recv(self): + self.cli.send(MSG) + + def test_send(self): + self.serv.send(MSG) + + def _test_send(self): + msg = self.cli.recv(1024) + self.assertEqual(msg, MSG) + + def _test_injected_authentication_failure(self): + # No-op. Exists for base class threading infrastructure to call. + # We could refactor this test into its own lesser class along with the + # setUp and tearDown code to construct an ideal; it is simpler to keep + # it here and live with extra overhead one this _one_ failure test. + pass + + def test_injected_authentication_failure(self): + import secrets + orig_token_bytes = secrets.token_bytes + fake_n = secrets.DEFAULT_ENTROPY - 1 + from unittest import mock + with mock.patch.object( + secrets, 'token_bytes', + new=lambda nbytes=None: orig_token_bytes(fake_n)): + s1, s2 = None, None + try: + with self.assertRaisesRegex(ConnectionError, + "authentication fail"): + s1, s2 = socket.socketpair() + finally: + if s1: s1.close() + if s2: s2.close() + + class NonBlockingTCPTests(ThreadedTCPSocketTest): def __init__(self, methodName='runTest'): diff --git a/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst b/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst new file mode 100644 index 00000000000000..b1500d0daad51f --- /dev/null +++ b/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst @@ -0,0 +1,5 @@ +Authenticate the socket connection for the ``socket.socketpair()`` fallback +on platforms where ``AF_UNIX`` is not available like Windows. + +Patch by Gregory P. Smith . Reported by Ellie + From e08f3fb4ed781cd728b925b9e17ae07e75405fea Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Tue, 23 Jul 2024 14:45:01 -0500 Subject: [PATCH 2/6] Authenticate with getsocketname and getpeername --- Lib/socket.py | 101 +++++++++++----------------------------- Lib/test/test_socket.py | 45 ++++++++++++------ 2 files changed, 58 insertions(+), 88 deletions(-) diff --git a/Lib/socket.py b/Lib/socket.py index 0e738926124c92..3f7f302ff79e2a 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -628,81 +628,36 @@ def socketpair(family=AF_INET, type=SOCK_STREAM, proto=0): if proto != 0: raise ValueError("Only protocol zero is supported") - import secrets # Delay import until actually needed. - auth_len = secrets.DEFAULT_ENTROPY - reason = "unknown" - for _ in range(5): - # We do not try forever, that'd just provide another process - # the ability to make us waste CPU retrying. In all normal - # circumstances the first connection auth attempt succeeds. - s_auth = secrets.token_bytes() - c_auth = secrets.token_bytes() - - # We create a connected TCP socket. Note the trick with - # setblocking(False) prevents us from having to create a thread. - csock = None - ssock = None - reason = "unknown" - lsock = socket(family, type, proto) + # We create a connected TCP socket. Note the trick with + # setblocking(False) that prevents us from having to create a thread. + lsock = socket(family, type, proto) + try: + lsock.bind((host, 0)) + lsock.listen() + # On IPv6, ignore flow_info and scope_id + addr, port = lsock.getsockname()[:2] + csock = socket(family, type, proto) try: - lsock.bind((host, 0)) - lsock.listen() - # On IPv6, ignore flow_info and scope_id - addr, port = lsock.getsockname()[:2] - csock = socket(family, type, proto) - try: - csock.setblocking(False) - try: - csock.connect((addr, port)) - except (BlockingIOError, InterruptedError): - pass - csock.setblocking(True) - ssock, _ = lsock.accept() - except Exception: - csock.close() - raise - - def authenticate_socket_conn(send_sock, recv_sock, auth_bytes): - nonlocal auth_len - data_buf = bytearray(auth_len) - data_mem = memoryview(data_buf) - data_len = 0 - - # Send the authentication bytes. - if send_sock.send(auth_bytes) != auth_len: - raise ConnectionError("send() sent too few auth bytes.") - - # Attempt to read the authentication bytes from the socket. - max_auth_time = time.monotonic() + 3.0 - while time.monotonic() < max_auth_time and data_len < auth_len: - bytes_received = recv_sock.recv_into(data_mem, auth_len - data_len) - if bytes_received == 0: - break # Connection closed. - data_len += bytes_received - data_mem = data_mem[bytes_received:] - - # Check that the authentication bytes match. - if len(data_buf) != auth_len: - raise ConnectionError("recv() got too few auth bytes.") - if bytes(data_buf) != auth_bytes: - raise ConnectionError(f"Mismatched auth token.") - - # Authenticating avoids using a connection from something else - # able to connect to {host}:{port} instead of us. + csock.setblocking(False) try: - authenticate_socket_conn(ssock, csock, s_auth) - authenticate_socket_conn(csock, ssock, c_auth) - except OSError as exc: - csock.close() - ssock.close() - reason = str(exc) - continue - # authentication successful, both sockets are our process. - break - finally: - lsock.close() - else: - raise ConnectionError(f"socketpair authentication failed: {reason}") + csock.connect((addr, port)) + except (BlockingIOError, InterruptedError): + pass + csock.setblocking(True) + ssock, _ = lsock.accept() + except: + csock.close() + raise + finally: + lsock.close() + + # Authenticating avoids using a connection from something else + # able to connect to {host}:{port} instead of us. + if ( + ssock.getsockname()[:2] != csock.getpeername()[:2] + or csock.getsockname()[:2] != ssock.getpeername()[:2] + ): + raise ConnectionError("Unexpected peer connection") return (ssock, csock) __all__.append("socketpair") diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index b70a28a1c790b3..80704e7eb23a4e 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -4921,21 +4921,36 @@ def _test_injected_authentication_failure(self): pass def test_injected_authentication_failure(self): - import secrets - orig_token_bytes = secrets.token_bytes - fake_n = secrets.DEFAULT_ENTROPY - 1 - from unittest import mock - with mock.patch.object( - secrets, 'token_bytes', - new=lambda nbytes=None: orig_token_bytes(fake_n)): - s1, s2 = None, None - try: - with self.assertRaisesRegex(ConnectionError, - "authentication fail"): - s1, s2 = socket.socketpair() - finally: - if s1: s1.close() - if s2: s2.close() + orig_getsockname = socket.socket.getsockname + inject_sock = None + + def inject_getsocketname(self): + nonlocal inject_sock + sockname = orig_getsockname(self) + # Connect to the listening socket ahead of the + # client socket. + if inject_sock is None: + inject_sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + inject_sock.setblocking(False) + try: + inject_sock.connect(sockname[:2]) + except (BlockingIOError, InterruptedError): + pass + inject_sock.setblocking(True) + return sockname + + sock1 = sock2 = None + try: + socket.socket.getsockname = inject_getsocketname + with self.assertRaises(OSError): + sock1, sock2 = socket.socketpair() + finally: + socket.socket.getsockname = orig_getsockname + if inject_sock: + inject_sock.close() + if sock1: # This cleanup isn't needed on a successful test. + sock1.close() + sock2.close() class NonBlockingTCPTests(ThreadedTCPSocketTest): From fcb43e91cce9fe9b6dc027f6b8955a128e0e310a Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Tue, 23 Jul 2024 14:50:26 -0500 Subject: [PATCH 3/6] Remove time import --- Lib/socket.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/socket.py b/Lib/socket.py index 3f7f302ff79e2a..c620a02229e206 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -52,7 +52,7 @@ import _socket from _socket import * -import os, sys, io, selectors, time +import os, sys, io, selectors from enum import IntEnum, IntFlag try: From fe5da2d40a054b58c58275a77201557d1f8b13ff Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Thu, 25 Jul 2024 10:16:02 -0500 Subject: [PATCH 4/6] Address review comments --- Lib/socket.py | 19 ++++++--- Lib/test/test_socket.py | 41 ++++++++++++++----- ...-07-22-13-11-28.gh-issue-122133.0mPeta.rst | 2 +- 3 files changed, 46 insertions(+), 16 deletions(-) diff --git a/Lib/socket.py b/Lib/socket.py index c620a02229e206..2e6043cbdb8005 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -653,11 +653,20 @@ def socketpair(family=AF_INET, type=SOCK_STREAM, proto=0): # Authenticating avoids using a connection from something else # able to connect to {host}:{port} instead of us. - if ( - ssock.getsockname()[:2] != csock.getpeername()[:2] - or csock.getsockname()[:2] != ssock.getpeername()[:2] - ): - raise ConnectionError("Unexpected peer connection") + # We expect only AF_INET and AF_INET6 families. + try: + if ( + ssock.getsockname() != csock.getpeername() + or csock.getsockname() != ssock.getpeername() + ): + raise ConnectionError("Unexpected peer connection") + except: + # getsockname() and getpeername() can fail + # if either socket isn't connected. + ssock.close() + csock.close() + raise + return (ssock, csock) __all__.append("socketpair") diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 80704e7eb23a4e..bd917ba9f3a43e 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -595,12 +595,12 @@ def __init__(self, methodName='runTest'): self.cli = None self.serv = None - def call_socketpair(self): + def socketpair(self): # To be overridden by some child classes. return socket.socketpair() def setUp(self): - self.serv, self.cli = self.call_socketpair() + self.serv, self.cli = self.socketpair() def tearDown(self): if self.serv: @@ -4907,17 +4907,30 @@ def _test_recv(self): self.cli.send(MSG) def test_send(self): - self.serv.send(MSG) - - def _test_send(self): msg = self.cli.recv(1024) self.assertEqual(msg, MSG) - def _test_injected_authentication_failure(self): - # No-op. Exists for base class threading infrastructure to call. - # We could refactor this test into its own lesser class along with the - # setUp and tearDown code to construct an ideal; it is simpler to keep - # it here and live with extra overhead one this _one_ failure test. + def _test_send(self): + self.serv.send(MSG) + + def test_ipv4(self): + cli, srv = socket.socketpair(socket.AF_INET) + cli.close() + srv.close() + + def _test_ipv4(self): + pass + + @unittest.skipIf(not hasattr(_socket, 'IPPROTO_IPV6') or + not hasattr(_socket, 'IPV6_V6ONLY'), + "IPV6_V6ONLY option not supported") + @unittest.skipUnless(socket_helper.IPV6_ENABLED, 'IPv6 required for this test') + def test_ipv6(self): + cli, srv = socket.socketpair(socket.AF_INET6) + cli.close() + srv.close() + + def _test_ipv6(self): pass def test_injected_authentication_failure(self): @@ -4950,8 +4963,16 @@ def inject_getsocketname(self): inject_sock.close() if sock1: # This cleanup isn't needed on a successful test. sock1.close() + if sock2: sock2.close() + def _test_injected_authentication_failure(self): + # No-op. Exists for base class threading infrastructure to call. + # We could refactor this test into its own lesser class along with the + # setUp and tearDown code to construct an ideal; it is simpler to keep + # it here and live with extra overhead one this _one_ failure test. + pass + class NonBlockingTCPTests(ThreadedTCPSocketTest): diff --git a/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst b/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst index b1500d0daad51f..3544eb3824d0da 100644 --- a/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst +++ b/Misc/NEWS.d/next/Security/2024-07-22-13-11-28.gh-issue-122133.0mPeta.rst @@ -1,5 +1,5 @@ Authenticate the socket connection for the ``socket.socketpair()`` fallback on platforms where ``AF_UNIX`` is not available like Windows. -Patch by Gregory P. Smith . Reported by Ellie +Patch by Gregory P. Smith and Seth Larson . Reported by Ellie From 76522138935283586868c8d189e231d2e4c24286 Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Thu, 25 Jul 2024 10:41:31 -0500 Subject: [PATCH 5/6] Complete refactor from call_socketpair() to socketpair() --- Lib/test/test_socket.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index bd917ba9f3a43e..c5d9385af398f4 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -4866,7 +4866,7 @@ class PurePythonSocketPairTest(SocketPairTest): # code path we're using regardless platform is the pure python one where # `_socket.socketpair` does not exist. (AF_INET does not work with # _socket.socketpair on many platforms). - def call_socketpair(self): + def socketpair(self): # called by super().setUp(). try: return socket.socketpair(socket.AF_INET6) From d1bc40886160e5f1f0e3b177f1f7562fe133031c Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Thu, 25 Jul 2024 15:58:50 -0500 Subject: [PATCH 6/6] Try reversing order back to original --- Lib/test/test_socket.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index c5d9385af398f4..bb65c3c5993b88 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -4862,7 +4862,7 @@ def _testSend(self): class PurePythonSocketPairTest(SocketPairTest): - # Explicitly use socketpair AF_INET or AF_INET6 to to ensure that is the + # Explicitly use socketpair AF_INET or AF_INET6 to ensure that is the # code path we're using regardless platform is the pure python one where # `_socket.socketpair` does not exist. (AF_INET does not work with # _socket.socketpair on many platforms). @@ -4907,11 +4907,11 @@ def _test_recv(self): self.cli.send(MSG) def test_send(self): - msg = self.cli.recv(1024) - self.assertEqual(msg, MSG) + self.serv.send(MSG) def _test_send(self): - self.serv.send(MSG) + msg = self.cli.recv(1024) + self.assertEqual(msg, MSG) def test_ipv4(self): cli, srv = socket.socketpair(socket.AF_INET)