From 32f8a931f9454bceae4bb3700d2cc1fa354f12c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 15:56:56 +0000 Subject: [PATCH 1/3] Track callback exceptions per Connection instead of per helper Exceptions raised inside verify, ALPN selection, OCSP and DTLS cookie callbacks cannot be raised from inside the callback, so they were deferred on a _CallbackExceptionHelper and re-raised by Connection._raise_ssl_error on the next send/recv/do_handshake. For callbacks registered on a Context, that helper (and its FIFO of pending exceptions) was shared by every Connection created from the Context. Because cffi releases the GIL around the SSL_* calls, a server that drives connections from several threads could have one connection pop and raise an exception that another connection's callback produced, aborting an otherwise healthy connection with a misattributed error, while the connection that actually failed reported a generic SSL error (or an IndexError from the unsynchronised check-then-pop). Record the pending exceptions on the Connection whose callback raised them instead, and have _raise_ssl_error and DTLSv1_listen only raise that connection's own. With no shared state left, the _CallbackExceptionHelper base class has nothing to do and is removed. This also fixes exceptions raised from a verify callback installed via Connection.set_verify being silently dropped: _raise_ssl_error only consulted the Context's verify helper, but the callback OpenSSL actually invokes for a connection is the one copied at SSL_new time or the one installed by Connection.set_verify. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm --- CHANGELOG.rst | 16 ++++++ src/OpenSSL/SSL.py | 120 +++++++++++++++++++-------------------------- tests/test_ssl.py | 66 +++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 52cdd756..cfdff5bb 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,6 +4,22 @@ Changelog Versions are year-based with a strict backward-compatibility policy. The third digit is only for regressions. +26.4.1 (UNRELEASED) +------------------- + +Backward-incompatible changes: +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Deprecations: +^^^^^^^^^^^^^ + +Changes: +^^^^^^^^ + +- Fixed a race in which an exception raised by a verify, ALPN selection, OCSP, or DTLS cookie callback for one ``Connection`` could be raised on an unrelated ``Connection`` created from the same ``Context`` and used concurrently from another thread. Exceptions from these callbacks are now tracked per ``Connection``. +- Fixed exceptions raised by a verify callback registered with ``Connection.set_verify`` being swallowed instead of being propagated to the caller. + + 26.4.0 (2026-08-01) ------------------- diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py index b7d58876..388a3386 100644 --- a/src/OpenSSL/SSL.py +++ b/src/OpenSSL/SSL.py @@ -466,43 +466,13 @@ class SysCallError(Error): pass -class _CallbackExceptionHelper: - """ - A base class for wrapper classes that allow for intelligent exception - handling in OpenSSL callbacks. - - :ivar list _problems: Any exceptions that occurred while executing in a - context where they could not be raised in the normal way. Typically - this is because OpenSSL has called into some Python code and requires a - return value. The exceptions are saved to be raised later when it is - possible to do so. - """ - - def __init__(self) -> None: - self._problems: list[Exception] = [] - - def raise_if_problem(self) -> None: - """ - Raise an exception from the OpenSSL error queue or that was previously - captured whe running a callback. - """ - if self._problems: - try: - _raise_current_error() - except Error: - pass - raise self._problems.pop(0) - - -class _VerifyHelper(_CallbackExceptionHelper): +class _VerifyHelper: """ Wrap a callback such that it can be used as a certificate verification callback. """ def __init__(self, callback: _VerifyCallback) -> None: - _CallbackExceptionHelper.__init__(self) - @wraps(callback) def wrapper(ok, store_ctx): # type: ignore[no-untyped-def] x509 = _lib.X509_STORE_CTX_get_current_cert(store_ctx) @@ -520,7 +490,7 @@ def wrapper(ok, store_ctx): # type: ignore[no-untyped-def] connection, cert, error_number, error_depth, ok ) except Exception as e: - self._problems.append(e) + connection._callback_problems.append(e) return 0 else: if result: @@ -534,19 +504,16 @@ def wrapper(ok, store_ctx): # type: ignore[no-untyped-def] ) -class _ALPNSelectHelper(_CallbackExceptionHelper): +class _ALPNSelectHelper: """ Wrap a callback such that it can be used as an ALPN selection callback. """ def __init__(self, callback: _ALPNSelectCallback) -> None: - _CallbackExceptionHelper.__init__(self) - @wraps(callback) def wrapper(ssl, out, outlen, in_, inlen, arg): # type: ignore[no-untyped-def] + conn = Connection._reverse_mapping[ssl] try: - conn = Connection._reverse_mapping[ssl] - # The string passed to us is made up of multiple # length-prefixed bytestrings. We need to split that into a # list. @@ -583,19 +550,26 @@ def wrapper(ssl, out, outlen, in_, inlen, arg): # type: ignore[no-untyped-def] return _lib.SSL_TLSEXT_ERR_NOACK return _lib.SSL_TLSEXT_ERR_OK except Exception as e: - self._problems.append(e) + conn._callback_problems.append(e) return _lib.SSL_TLSEXT_ERR_ALERT_FATAL + # If an exception ever escapes ``wrapper`` (it should not: the only + # statement outside its ``try`` is the connection lookup, which cannot + # fail for an SSL object owned by a Connection), cffi prints it and + # returns this value instead of the default 0, which would be + # SSL_TLSEXT_ERR_OK and make OpenSSL read the never-assigned ``out`` + # and ``outlen`` parameters. self.callback = _ffi.callback( ( "int (*)(SSL *, unsigned char **, unsigned char *, " "const unsigned char *, unsigned int, void *)" ), wrapper, + error=_lib.SSL_TLSEXT_ERR_ALERT_FATAL, ) -class _OCSPServerCallbackHelper(_CallbackExceptionHelper): +class _OCSPServerCallbackHelper: """ Wrap a callback such that it can be used as an OCSP callback for the server side. @@ -618,13 +592,10 @@ class _OCSPServerCallbackHelper(_CallbackExceptionHelper): """ def __init__(self, callback: _OCSPServerCallback[Any]) -> None: - _CallbackExceptionHelper.__init__(self) - @wraps(callback) def wrapper(ssl, cdata): # type: ignore[no-untyped-def] + conn = Connection._reverse_mapping[ssl] try: - conn = Connection._reverse_mapping[ssl] - # Extract the data if any was provided. if cdata != _ffi.NULL: data = _ffi.from_handle(cdata) @@ -656,13 +627,13 @@ def wrapper(ssl, cdata): # type: ignore[no-untyped-def] return 0 except Exception as e: - self._problems.append(e) + conn._callback_problems.append(e) return 2 # SSL_TLSEXT_ERR_ALERT_FATAL self.callback = _ffi.callback("int (*)(SSL *, void *)", wrapper) -class _OCSPClientCallbackHelper(_CallbackExceptionHelper): +class _OCSPClientCallbackHelper: """ Wrap a callback such that it can be used as an OCSP callback for the client side. @@ -685,13 +656,10 @@ class _OCSPClientCallbackHelper(_CallbackExceptionHelper): """ def __init__(self, callback: _OCSPClientCallback[Any]) -> None: - _CallbackExceptionHelper.__init__(self) - @wraps(callback) def wrapper(ssl, cdata): # type: ignore[no-untyped-def] + conn = Connection._reverse_mapping[ssl] try: - conn = Connection._reverse_mapping[ssl] - # Extract the data if any was provided. if cdata != _ffi.NULL: data = _ffi.from_handle(cdata) @@ -714,23 +682,21 @@ def wrapper(ssl, cdata): # type: ignore[no-untyped-def] return int(bool(valid)) except Exception as e: - self._problems.append(e) + conn._callback_problems.append(e) # Return negative value if an exception is hit. return -1 self.callback = _ffi.callback("int (*)(SSL *, void *)", wrapper) -class _CookieGenerateCallbackHelper(_CallbackExceptionHelper): +class _CookieGenerateCallbackHelper: def __init__(self, callback: _CookieGenerateCallback) -> None: - _CallbackExceptionHelper.__init__(self) - max_cookie_len = getattr(_lib, "DTLS1_COOKIE_LENGTH", 255) @wraps(callback) def wrapper(ssl, out, outlen): # type: ignore[no-untyped-def] + conn = Connection._reverse_mapping[ssl] try: - conn = Connection._reverse_mapping[ssl] cookie = callback(conn) if len(cookie) > max_cookie_len: raise ValueError( @@ -741,7 +707,7 @@ def wrapper(ssl, out, outlen): # type: ignore[no-untyped-def] outlen[0] = len(cookie) return 1 except Exception as e: - self._problems.append(e) + conn._callback_problems.append(e) # "a zero return value can be used to abort the handshake" return 0 @@ -751,17 +717,15 @@ def wrapper(ssl, out, outlen): # type: ignore[no-untyped-def] ) -class _CookieVerifyCallbackHelper(_CallbackExceptionHelper): +class _CookieVerifyCallbackHelper: def __init__(self, callback: _CookieVerifyCallback) -> None: - _CallbackExceptionHelper.__init__(self) - @wraps(callback) def wrapper(ssl, c_cookie, cookie_len): # type: ignore[no-untyped-def] + conn = Connection._reverse_mapping[ssl] try: - conn = Connection._reverse_mapping[ssl] return callback(conn, bytes(c_cookie[0:cookie_len])) except Exception as e: - self._problems.append(e) + conn._callback_problems.append(e) return 0 self.callback = _ffi.callback( @@ -2003,6 +1967,16 @@ def __init__( # avoid them getting freed. self._alpn_select_callback_args: Any = None + # Exceptions raised by callbacks (verify, ALPN selection, OCSP, DTLS + # cookies) while OpenSSL was calling into Python on behalf of this + # connection. They cannot be raised from inside the callback, so they + # are recorded here and raised by the next method that OpenSSL + # returns control to. They are kept on the connection rather than on + # the (context-wide, shared) callback wrapper so that a connection + # driven concurrently from another thread can never be handed an + # exception that belongs to a different connection. + self._callback_problems: list[Exception] = [] + # Reference the verify_callback of the Context. This ensures that if # set_verify is called again after the SSL object has been created we # do not point to a dangling reference @@ -2046,13 +2020,22 @@ def __getattr__(self, name: str) -> Any: else: return getattr(self._socket, name) + def _raise_callback_problem(self) -> None: + """ + Raise an exception that was captured while running a callback on + behalf of this connection, if there is one, after discarding whatever + OpenSSL left on its error queue as a consequence of the callback + failing. + """ + if self._callback_problems: + try: + _raise_current_error() + except Error: + pass + raise self._callback_problems.pop(0) + def _raise_ssl_error(self, ssl: Any, result: int) -> None: - if self._context._verify_helper is not None: - self._context._verify_helper.raise_if_problem() - if self._context._alpn_select_helper is not None: - self._context._alpn_select_helper.raise_if_problem() - if self._context._ocsp_helper is not None: - self._context._ocsp_helper.raise_if_problem() + self._raise_callback_problem() error = _lib.SSL_get_error(ssl, result) if error == _lib.SSL_ERROR_WANT_READ: @@ -2561,10 +2544,7 @@ def DTLSv1_listen(self) -> None: # ClientHello with valid cookie, but keep trying'. So basically # WantReadError. But it doesn't work correctly with _raise_ssl_error. # So we raise it manually instead. - if self._cookie_generate_helper is not None: - self._cookie_generate_helper.raise_if_problem() - if self._cookie_verify_helper is not None: - self._cookie_verify_helper.raise_if_problem() + self._raise_callback_problem() if result == 0: raise WantReadError() if result < 0: diff --git a/tests/test_ssl.py b/tests/test_ssl.py index a551fc7b..1f4c4663 100644 --- a/tests/test_ssl.py +++ b/tests/test_ssl.py @@ -2399,6 +2399,46 @@ def select(conn: Connection, options: list[bytes]) -> bytes: interact_in_memory(server, client) assert select_args == [(server, [b"http/1.1", b"spdy/2"])] + def test_alpn_callback_exception_stays_with_its_connection(self) -> None: + """ + An exception raised by the ALPN select callback while serving one + connection is only ever raised on that connection, even though every + connection created from the same `Context` shares the callback + wrapper. Otherwise, with connections driven from several threads, an + unrelated connection could be handed another connection's exception. + """ + + def select(conn: Connection, options: list[bytes]) -> bytes: + return options[0] + + server_context = Context(SSLv23_METHOD) + server_context.set_alpn_select_callback(select) + server_context.use_privatekey( + load_privatekey(FILETYPE_PEM, server_key_pem) + ) + server_context.use_certificate( + load_certificate(FILETYPE_PEM, server_cert_pem) + ) + + client_context = Context(SSLv23_METHOD) + client_context.set_alpn_protos([b"http/1.1"]) + + failing = Connection(server_context, None) + other = Connection(server_context, None) + client = Connection(client_context, None) + + # Simulate the callback having raised on ``failing`` in another + # thread that has not yet had the chance to surface the exception. + failing._callback_problems.append(ValueError("belongs to failing")) + + # The unrelated connection is unaffected... + handshake_in_memory(client, other) + assert other.get_alpn_proto_negotiated() == b"http/1.1" + + # ...and the exception is still delivered to the right connection. + with pytest.raises(ValueError, match="belongs to failing"): + failing.do_handshake() + class TestSession: """ @@ -3012,6 +3052,32 @@ def test_set_verify_overrides_context(self) -> None: with pytest.raises(TypeError): conn.set_verify(VERIFY_PEER, "not a callable") # type: ignore[arg-type] + def test_set_verify_callback_exception(self) -> None: + """ + If the verify callback passed to `Connection.set_verify` raises an + exception, verification fails and the exception is propagated to the + caller of `Connection.do_handshake`. + """ + server_context = Context(SSLv23_METHOD) + server_context.use_privatekey( + load_privatekey(FILETYPE_PEM, root_key_pem) + ) + server_context.use_certificate( + load_certificate(FILETYPE_PEM, root_cert_pem) + ) + server = Connection(server_context, None) + + def verify_callback( + conn: Connection, cert: X509, err: int, depth: int, ok: int + ) -> bool: + raise KeyError("silly verify failure") + + client = Connection(Context(SSLv23_METHOD), None) + client.set_verify(VERIFY_PEER, verify_callback) + + with pytest.raises(KeyError, match="silly verify failure"): + handshake_in_memory(client, server) + def test_set_verify_callback_reference(self) -> None: """ The callback for certificate verification should only be forgotten if From 26ab7eb6eb96e4e20a8ef45303f6fed1ac05fa81 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 17:09:53 +0000 Subject: [PATCH 2/3] Clear the OpenSSL error queue directly before raising a callback exception _raise_callback_problem drained the thread's OpenSSL error queue by calling _raise_current_error() and swallowing the resulting Error. That relies on _raise_current_error() raising even when the queue is empty, and obscures the intent. _raise_ssl_error already uses ERR_clear_error() for the same purpose a few lines further down; do the same here. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm --- src/OpenSSL/SSL.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/OpenSSL/SSL.py b/src/OpenSSL/SSL.py index 388a3386..d91915bc 100644 --- a/src/OpenSSL/SSL.py +++ b/src/OpenSSL/SSL.py @@ -2028,10 +2028,7 @@ def _raise_callback_problem(self) -> None: failing. """ if self._callback_problems: - try: - _raise_current_error() - except Error: - pass + _lib.ERR_clear_error() raise self._callback_problems.pop(0) def _raise_ssl_error(self, ssl: Any, result: int) -> None: From b89f8b3caa09e8a6178a68d1ebfa8fcb37bc7fb7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 17:12:48 +0000 Subject: [PATCH 3/3] Credit the reporter in the changelog Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01A46SM1jtRoG4Yh5P2k9cUm --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cfdff5bb..6175dcd3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,7 +16,7 @@ Deprecations: Changes: ^^^^^^^^ -- Fixed a race in which an exception raised by a verify, ALPN selection, OCSP, or DTLS cookie callback for one ``Connection`` could be raised on an unrelated ``Connection`` created from the same ``Context`` and used concurrently from another thread. Exceptions from these callbacks are now tracked per ``Connection``. +- Fixed a race in which an exception raised by a verify, ALPN selection, OCSP, or DTLS cookie callback for one ``Connection`` could be raised on an unrelated ``Connection`` created from the same ``Context`` and used concurrently from another thread. Exceptions from these callbacks are now tracked per ``Connection``. Discovered and reported by SecDim Security Research. - Fixed exceptions raised by a verify callback registered with ``Connection.set_verify`` being swallowed instead of being propagated to the caller.