From ee93104aa0e7ac77ca7e4c6db9873356a9e3e622 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Thu, 2 May 2024 16:38:14 +0200 Subject: [PATCH] Removed unused sessions from SSL_CTX internal cache (#101684) * Disable OpenSSL internal SSL_SESSION cache for clients * Attempt no. 2 * Revert "Disable OpenSSL internal SSL_SESSION cache for clients" This reverts commit 56a308e88171bb797d13d50953b83262cd8289cd. --- .../Interop.OpenSsl.cs | 2 +- .../Interop.SslCtx.cs | 36 +++++++++++++------ .../entrypoints.c | 1 + .../opensslshim.h | 2 ++ .../pal_ssl.c | 5 +++ .../pal_ssl.h | 5 +++ 6 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 8636bbe488486..171980d0a7654 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -748,7 +748,7 @@ private static unsafe void RemoveSessionCallback(IntPtr ctx, IntPtr session) IntPtr name = Ssl.SessionGetHostname(session); Debug.Assert(name != IntPtr.Zero); - ctxHandle.RemoveSession(name); + ctxHandle.RemoveSession(name, session); } [UnmanagedCallersOnly] diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs index 5baa776d9d5a8..d92e15e940e65 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs @@ -39,6 +39,9 @@ internal static partial class Ssl [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetCaching")] internal static unsafe partial int SslCtxSetCaching(SafeSslContextHandle ctx, int mode, int cacheSize, int contextIdLength, Span contextId, delegate* unmanaged neewSessionCallback, delegate* unmanaged removeSessionCallback); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxRemoveSession")] + internal static unsafe partial void SslCtxRemoveSession(SafeSslContextHandle ctx, IntPtr session); + internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, ReadOnlyCollection chain) { // send pre-computed list of intermediates. @@ -142,27 +145,38 @@ internal bool TryAddSession(IntPtr namePtr, IntPtr session) // This will use strdup() so it is safe to pass in raw pointer. Interop.Ssl.SessionSetHostname(session, namePtr); + IntPtr oldSession = IntPtr.Zero; + lock (_sslSessions) { if (!_sslSessions.TryAdd(targetName, session)) { - if (_sslSessions.Remove(targetName, out IntPtr oldSession)) - { - Interop.Ssl.SessionFree(oldSession); - } - + // session to this target host exists, replace it + _sslSessions.Remove(targetName, out oldSession); bool added = _sslSessions.TryAdd(targetName, session); Debug.Assert(added); } } + if (oldSession != IntPtr.Zero) + { + // remove old session also from the internal OpenSSL cache + // and drop reference count. Since SSL_CTX_remove_session + // will call session_remove_cb, we need to do this outside + // of _sslSessions lock to avoid deadlock with another thread + // which could be holding SSL_CTX lock and trying to acquire + // _sslSessions lock. + Interop.Ssl.SslCtxRemoveSession(this, oldSession); + Interop.Ssl.SessionFree(oldSession); + } + return true; } return false; } - internal void RemoveSession(IntPtr namePtr) + internal void RemoveSession(IntPtr namePtr, IntPtr session) { Debug.Assert(_sslSessions != null); @@ -171,11 +185,14 @@ internal void RemoveSession(IntPtr namePtr) if (_sslSessions != null && targetName != null) { - IntPtr oldSession; - bool removed; + IntPtr oldSession = IntPtr.Zero; + bool removed = false; lock (_sslSessions) { - removed = _sslSessions.Remove(targetName, out oldSession); + if (_sslSessions.TryGetValue(targetName, out IntPtr existingSession) && existingSession == session) + { + removed = _sslSessions.Remove(targetName, out oldSession); + } } if (removed) @@ -209,7 +226,6 @@ internal bool TrySetSession(SafeSslHandle sslHandle, string name) // This will increase reference count on the session as needed. // We need to hold lock here to prevent session being deleted before the call is done. Interop.Ssl.SslSetSession(sslHandle, session); - return true; } } diff --git a/src/native/libs/System.Security.Cryptography.Native/entrypoints.c b/src/native/libs/System.Security.Cryptography.Native/entrypoints.c index a8bcbde96e511..cb4b621e27fa3 100644 --- a/src/native/libs/System.Security.Cryptography.Native/entrypoints.c +++ b/src/native/libs/System.Security.Cryptography.Native/entrypoints.c @@ -312,6 +312,7 @@ static const Entry s_cryptoNative[] = DllImportEntry(CryptoNative_IsSslStateOK) DllImportEntry(CryptoNative_SslCtxAddExtraChainCert) DllImportEntry(CryptoNative_SslCtxSetCaching) + DllImportEntry(CryptoNative_SslCtxRemoveSession) DllImportEntry(CryptoNative_SslCtxSetCiphers) DllImportEntry(CryptoNative_SslCtxSetDefaultOcspCallback) DllImportEntry(CryptoNative_SslCtxSetEncryptionPolicy) diff --git a/src/native/libs/System.Security.Cryptography.Native/opensslshim.h b/src/native/libs/System.Security.Cryptography.Native/opensslshim.h index 9dd878858822b..7837810de715e 100644 --- a/src/native/libs/System.Security.Cryptography.Native/opensslshim.h +++ b/src/native/libs/System.Security.Cryptography.Native/opensslshim.h @@ -555,6 +555,7 @@ int EVP_DigestSqueeze(EVP_MD_CTX *ctx, unsigned char *out, size_t outlen); REQUIRED_FUNCTION(SSL_CTX_new) \ REQUIRED_FUNCTION(SSL_CTX_sess_set_new_cb) \ REQUIRED_FUNCTION(SSL_CTX_sess_set_remove_cb) \ + REQUIRED_FUNCTION(SSL_CTX_remove_session) \ LIGHTUP_FUNCTION(SSL_CTX_set_alpn_protos) \ LIGHTUP_FUNCTION(SSL_CTX_set_alpn_select_cb) \ REQUIRED_FUNCTION(SSL_CTX_set_cipher_list) \ @@ -1083,6 +1084,7 @@ FOR_ALL_OPENSSL_FUNCTIONS #define SSL_CTX_new SSL_CTX_new_ptr #define SSL_CTX_sess_set_new_cb SSL_CTX_sess_set_new_cb_ptr #define SSL_CTX_sess_set_remove_cb SSL_CTX_sess_set_remove_cb_ptr +#define SSL_CTX_remove_session SSL_CTX_remove_session_ptr #define SSL_CTX_set_alpn_protos SSL_CTX_set_alpn_protos_ptr #define SSL_CTX_set_alpn_select_cb SSL_CTX_set_alpn_select_cb_ptr #define SSL_CTX_set_cipher_list SSL_CTX_set_cipher_list_ptr diff --git a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c index e6bd41143c165..e320d1c73d776 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c @@ -701,6 +701,11 @@ int CryptoNative_SslCtxSetCaching(SSL_CTX* ctx, int mode, int cacheSize, int con return retValue; } +int CryptoNative_SslCtxRemoveSession(SSL_CTX* ctx, SSL_SESSION* session) +{ + return SSL_CTX_remove_session(ctx, session); +} + const char* CryptoNative_SslGetServerName(SSL* ssl) { return SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); diff --git a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h index 3c63564cc4e59..9c9d7026119c5 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h @@ -167,6 +167,11 @@ Sets session caching. 0 is disabled. */ PALEXPORT int CryptoNative_SslCtxSetCaching(SSL_CTX* ctx, int mode, int cacheSize, int contextIdLength, uint8_t* contextId, SslCtxNewSessionCallback newSessionCb, SslCtxRemoveSessionCallback removeSessionCb); +/* +Removes a session from internal cache. +*/ +PALEXPORT int CryptoNative_SslCtxRemoveSession(SSL_CTX* ctx, SSL_SESSION* session); + /* Sets callback to log TLS session keys */