This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/qpid-proton.git
commit d7fbad0487f9f9d9466187026ec9d3f8fb646852 Author: Andrew Stitcher <[email protected]> AuthorDate: Thu Aug 20 12:08:32 2026 -0400 PROTON-2959: [Python] Fix heap use after free in SASL tests Tests were using self.s1 after replacing self.t1 with a new transport, leaving a dangling pointer to the freed original transport. This demonstrated a (longstanding) issue in the bindings: If a Transport object is destroyed but the application has created SASL or SSL objects those python objects now refer to a C object that has also been destroyed as it is owned by the transport object. The fix is for the SASL and SSL object to hold a weakref to the Transport object and to check if it is alive before every C level operation, throwing an exception if it isn't. This is much better than the previous segv! --- python/proton/_transport.py | 38 ++++++++++++++++++ python/tests/proton_tests/sasl.py | 81 ++++++++++++++++++++++++++++++++++++++- python/tests/proton_tests/ssl.py | 66 ++++++++++++++++++++++++++++++- 3 files changed, 182 insertions(+), 3 deletions(-) diff --git a/python/proton/_transport.py b/python/proton/_transport.py index b24b150cb..9c36cd14e 100644 --- a/python/proton/_transport.py +++ b/python/proton/_transport.py @@ -19,6 +19,7 @@ from __future__ import annotations +import weakref from typing import Callable, Optional, Union, TYPE_CHECKING from cproton import PN_EOS, PN_SASL_AUTH, PN_SASL_NONE, PN_SASL_OK, PN_SASL_PERM, PN_SASL_SYS, PN_SASL_TEMP, \ @@ -571,11 +572,20 @@ class SASL: if not transport._sasl: sasl = super().__new__(cls) sasl._sasl = pn_sasl(transport._impl) + sasl._transport = weakref.ref(transport) transport._sasl = sasl return sasl else: return transport._sasl + def _check_alive(self) -> None: + """Check that the parent Transport is still alive. + + :raise: :exc:`SASLException` if the Transport has been destroyed. + """ + if self._transport() is None: + raise SASLException("Cannot access SASL object - parent Transport has been destroyed") + def _check(self, err): if err < 0: exc = EXCEPTIONS.get(err, SASLException) @@ -599,6 +609,7 @@ class SASL: * Otherwise a string containing the user is returned. """ + self._check_alive() return pn_sasl_get_user(self._sasl) @property @@ -624,6 +635,7 @@ class SASL: * Otherwise a string containing the user is returned. """ + self._check_alive() return pn_sasl_get_authorization(self._sasl) @property @@ -636,6 +648,7 @@ class SASL: :rtype: The authentication mechanism selected by the SASL layer. """ + self._check_alive() return pn_sasl_get_mech(self._sasl) @property @@ -646,6 +659,7 @@ class SASL: :rtype: * ``None`` if no negotiation has taken place. * Otherwise the outcome of the negotiation. """ + self._check_alive() outcome = pn_sasl_outcome(self._sasl) if outcome == PN_SASL_NONE: return None @@ -675,6 +689,7 @@ class SASL: ``"mech1 mech2 ..."``, or a Python list of strings ``["mech1", "mech2", ...]``. """ + self._check_alive() if isinstance(mechs, list): mechs = " ".join(mechs) pn_sasl_allowed_mechs(self._sasl, mechs) @@ -682,10 +697,12 @@ class SASL: @property def allow_insecure_mechs(self) -> bool: """Allow unencrypted cleartext passwords (PLAIN mech)""" + self._check_alive() return pn_sasl_get_allow_insecure_mechs(self._sasl) @allow_insecure_mechs.setter def allow_insecure_mechs(self, insecure: bool) -> None: + self._check_alive() pn_sasl_set_allow_insecure_mechs(self._sasl, insecure) def done(self, outcome): @@ -693,6 +710,7 @@ class SASL: Set the outcome of SASL negotiation. Used by the server to set the result of the negotiation process. """ + self._check_alive() pn_sasl_done(self._sasl, outcome) def config_name(self, name: str): @@ -707,6 +725,7 @@ class SASL: :param name: The configuration name. """ + self._check_alive() pn_sasl_config_name(self._sasl, name) def config_path(self, path: str): @@ -724,6 +743,7 @@ class SASL: :param path: The configuration path, may contain colon-separated list if more than one path is specified. """ + self._check_alive() pn_sasl_config_path(self._sasl, path) @@ -879,6 +899,14 @@ class SSL: else: return err + def _check_alive(self) -> None: + """Check that the parent Transport is still alive. + + :raise: :exc:`SSLException` if the Transport has been destroyed. + """ + if self._transport() is None: + raise SSLException("Cannot access SSL object - parent Transport has been destroyed") + def __new__( cls: type['SSL'], transport: Transport, @@ -907,6 +935,7 @@ class SSL: raise SSLUnavailable() if domain: pn_ssl_init(obj._ssl, domain._domain, session_id) + obj._transport = weakref.ref(transport) transport._ssl = obj return transport._ssl @@ -922,6 +951,7 @@ class SSL: :return: The cypher name, or ``None`` if no cipher in use. """ + self._check_alive() return pn_ssl_get_cipher_name(self._ssl, 128) def protocol_name(self) -> Optional[str]: @@ -936,6 +966,7 @@ class SSL: :return: The protocol name if SSL is active, or ``None`` if SSL connection is not ready or active. """ + self._check_alive() return pn_ssl_get_protocol_name(self._ssl, 128) SHA1 = PN_SSL_SHA1 @@ -986,6 +1017,7 @@ class SSL: :return: A string which contains the requested sub field value which is valid until the ssl object is destroyed. """ + self._check_alive() subfield_value = pn_ssl_get_remote_subject_subfield(self._ssl, subfield_name) return subfield_value @@ -995,6 +1027,7 @@ class SSL: :return: A string containing the full subject. """ + self._check_alive() subject = pn_ssl_get_remote_subject(self._ssl) return subject @@ -1073,6 +1106,7 @@ class SSL: :const:`SHA256`, :const:`SHA512`, :const:`MD5`. :return: Hex fingerprint in a string, or ``None`` if an error occurred. """ + self._check_alive() return pn_ssl_get_cert_fingerprint(self._ssl, fingerprint_length, digest_name) # Convenience functions for obtaining fingerprint for specific hashing algorithms @@ -1122,6 +1156,7 @@ class SSL: """ The subject from the peers certificate. """ + self._check_alive() return pn_ssl_get_remote_subject(self._ssl) RESUME_UNKNOWN = PN_SSL_RESUME_UNKNOWN @@ -1151,6 +1186,7 @@ class SSL: * :const:`RESUME_NEW` * :const:`RESUME_REUSED` """ + self._check_alive() return pn_ssl_resume_status(self._ssl) @property @@ -1170,12 +1206,14 @@ class SSL: .. note:: Verification of the hostname is only done if :const:`SSLDomain.VERIFY_PEER_NAME` is set using :meth:`SSLDomain.set_peer_authentication`.""" + self._check_alive() err, name = pn_ssl_get_peer_hostname(self._ssl, 1024) self._check(err) return name @peer_hostname.setter def peer_hostname(self, hostname: Optional[str]) -> None: + self._check_alive() self._check(pn_ssl_set_peer_hostname(self._ssl, hostname)) diff --git a/python/tests/proton_tests/sasl.py b/python/tests/proton_tests/sasl.py index 88899d6aa..d2cde9c49 100644 --- a/python/tests/proton_tests/sasl.py +++ b/python/tests/proton_tests/sasl.py @@ -269,6 +269,85 @@ class SaslTest(Test): assert sasl1.my_attribute == attr assert sasl2.my_attribute == attr + def test_after_transport_destroyed(self): + """Verify that SASL raises exception when Transport is destroyed""" + from proton import TransportException + + # Create a transport and get its SASL object + transport = Transport() + sasl = transport.sasl() + + # Verify SASL works while transport is alive + assert sasl.outcome is None + sasl.allowed_mechs("PLAIN ANONYMOUS") + assert sasl.allow_insecure_mechs is False + sasl.allow_insecure_mechs = True + assert sasl.allow_insecure_mechs is True + + # Destroy the transport + del transport + + # All SASL methods should now raise TransportException + try: + _ = sasl.user + assert False, "Should have raised TransportException for sasl.user" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = sasl.authorization + assert False, "Should have raised TransportException for sasl.authorization" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = sasl.mech + assert False, "Should have raised TransportException for sasl.mech" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = sasl.outcome + assert False, "Should have raised TransportException for sasl.outcome" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + sasl.allowed_mechs("ANONYMOUS") + assert False, "Should have raised TransportException for sasl.allowed_mechs()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = sasl.allow_insecure_mechs + assert False, "Should have raised TransportException for sasl.allow_insecure_mechs getter" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + sasl.allow_insecure_mechs = False + assert False, "Should have raised TransportException for sasl.allow_insecure_mechs setter" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + sasl.done(SASL.OK) + assert False, "Should have raised TransportException for sasl.done()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + sasl.config_name("test") + assert False, "Should have raised TransportException for sasl.config_name()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + sasl.config_path("/tmp") + assert False, "Should have raised TransportException for sasl.config_path()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + def testSaslSkipped(self): """Verify that the server (with SASL) correctly handles a client without SASL""" self.t1 = Transport() @@ -277,7 +356,6 @@ class SaslTest(Test): assert self.s2.outcome is None assert self.t2.condition is None assert self.t2.authenticated is False - assert self.s1.outcome is None assert self.t1.condition is None assert self.t1.authenticated is False @@ -288,7 +366,6 @@ class SaslTest(Test): self.pump() assert self.s2.outcome is None assert self.t2.condition is not None - assert self.s1.outcome is None assert self.t1.condition is not None def testMechNotFound(self): diff --git a/python/tests/proton_tests/ssl.py b/python/tests/proton_tests/ssl.py index 797f29117..289a4da43 100644 --- a/python/tests/proton_tests/ssl.py +++ b/python/tests/proton_tests/ssl.py @@ -149,7 +149,7 @@ class SslTest(common.Test): server.connection.close() self._pump(client, server) - def test_ssl_with_small_buffer(self): + def test_with_small_buffer(self): self.server_domain.set_credentials(self._testpath("server-certificate.pem"), self._testpath("server-private-key.pem"), "server-password") @@ -968,3 +968,67 @@ class SslTest(common.Test): assert False, "Expected error did not occur!" except SSLException: pass + + def test_after_transport_destroyed(self): + """Verify that SSL raises exception when Transport is destroyed""" + from proton import TransportException + + # Create a transport and get its SSL object + transport = Transport() + ssl = SSL(transport, self.client_domain) + + # Verify SSL works while transport is alive + assert ssl.cipher_name() is None # No cipher until handshake + assert ssl.protocol_name() is None # No protocol until handshake + + # Destroy the transport + del transport + + # All SSL methods should now raise TransportException + try: + _ = ssl.cipher_name() + assert False, "Should have raised TransportException for ssl.cipher_name()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = ssl.protocol_name() + assert False, "Should have raised TransportException for ssl.protocol_name()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = ssl.remote_subject + assert False, "Should have raised TransportException for ssl.remote_subject" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = ssl.resume_status() + assert False, "Should have raised TransportException for ssl.resume_status()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = ssl.peer_hostname + assert False, "Should have raised TransportException for ssl.peer_hostname getter" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + ssl.peer_hostname = "example.com" + assert False, "Should have raised TransportException for ssl.peer_hostname setter" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = ssl.get_cert_subject() + assert False, "Should have raised TransportException for ssl.get_cert_subject()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) + + try: + _ = ssl.get_cert_fingerprint(41, SSL.SHA1) + assert False, "Should have raised TransportException for ssl.get_cert_fingerprint()" + except TransportException as e: + assert "parent Transport has been destroyed" in str(e) --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
