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 56a308e.
dotnet · May 2, 2024 · ee93104 · ee93104
1 parent fc76b1c
commit ee93104
Show file tree

Hide file tree

Showing 6 changed files with 40 additions and 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
@@ -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
@@ -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<byte> contextId, delegate* unmanaged<IntPtr, IntPtr, int> neewSessionCallback, delegate* unmanaged<IntPtr, IntPtr, void> removeSessionCallback);
 
+        [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxRemoveSession")]
+        internal static unsafe partial void SslCtxRemoveSession(SafeSslContextHandle ctx, IntPtr session);
+
         internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, ReadOnlyCollection<X509Certificate2> 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
@@ -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
@@ -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
@@ -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
@@ -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
 */