Properly dispose intermediate certificates in SslStream

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs

@@ -9,7 +9,7 @@
 
 namespace System.Net.Security
 {
-    internal sealed class SslAuthenticationOptions
+    internal sealed class SslAuthenticationOptions : IDisposable
     {
         private const string EnableOcspStaplingContextSwitchName = "System.Net.Security.EnableServerOcspStaplingFromOnlyCertificateOnLinux";
 
@@ -153,7 +153,7 @@ internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticati
                     bool ocspFetch = false;
                     _ = AppContext.TryGetSwitch(EnableOcspStaplingContextSwitchName, out ocspFetch);
                     // given cert is X509Certificate2 with key. We can use it directly.
-                    CertificateContext = SslStreamCertificateContext.Create(certificateWithKey, additionalCertificates: null, offline: false, trust: null, noOcspFetch: !ocspFetch);
+                    SetCertificateContextFromCert(certificateWithKey, !ocspFetch);
                 }
                 else
                 {
@@ -165,7 +165,7 @@ internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticati
                         throw new AuthenticationException(SR.net_ssl_io_no_server_cert);
                     }
 
-                    CertificateContext = SslStreamCertificateContext.Create(certificateWithKey);
+                    SetCertificateContextFromCert(certificateWithKey);
                 }
             }
 
@@ -195,13 +195,22 @@ private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols proto
             return protocols;
         }
 
+        internal void SetCertificateContextFromCert(X509Certificate2 certificate, bool? noOcspFetch = null)
+        {
+            CertificateContext = SslStreamCertificateContext.Create(certificate, null, offline: false, null, noOcspFetch ?? true);
+            OwnsCertificateContext = true;
+        }
+
         internal bool AllowRenegotiation { get; set; }
         internal string TargetHost { get; set; }
         internal X509CertificateCollection? ClientCertificates { get; set; }
         internal List<SslApplicationProtocol>? ApplicationProtocols { get; set; }
         internal bool IsServer { get; set; }
         internal bool IsClient => !IsServer;
-        internal SslStreamCertificateContext? CertificateContext { get; set; }
+        internal SslStreamCertificateContext? CertificateContext { get; private set; }
+        // If true, the certificate context was created by the SslStream and
+        // certificates inside should be disposed when no longer needed.
+        internal bool OwnsCertificateContext { get; private set; }
         internal SslProtocols EnabledSslProtocols { get; set; }
         internal X509RevocationMode CertificateRevocationCheckMode { get; set; }
         internal EncryptionPolicy EncryptionPolicy { get; set; }
@@ -221,5 +230,17 @@ private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols proto
 #if TARGET_ANDROID
         internal SslStream.JavaProxy? SslStreamProxy { get; set; }
 #endif
+
+        public void Dispose()
+        {
+            if (OwnsCertificateContext && CertificateContext != null)
+            {
+                CertificateContext.ReleaseResources();
+            }
+
+#if TARGET_ANDROID
+            SslStreamProxy?.Dispose();
+#endif
+        }
     }
 }

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs

@@ -170,9 +170,7 @@ internal void CloseContext()
             _securityContext?.Dispose();
             _credentialsHandle?.Dispose();
 
-#if TARGET_ANDROID
-            _sslAuthenticationOptions.SslStreamProxy?.Dispose();
-#endif
+            _sslAuthenticationOptions.Dispose();
         }
 
         //
@@ -577,11 +575,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredential
 
             if (newCredentialsRequested)
             {
-                if (selectedCert != null)
-                {
-                    // build the cert context only if it was not provided by the user
-                    _sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert);
-                }
+                UpdateCertificateContext(selectedCert);
 
                 if (SslStreamPal.TryUpdateClintCertificate(_credentialsHandle, _securityContext, _sslAuthenticationOptions))
                 {
@@ -639,31 +633,30 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredential
                         NetEventSource.Log.UsingCachedCredential(this);
                     _credentialsHandle = cachedCredentialHandle;
                     cachedCred = true;
-                    if (selectedCert != null)
-                    {
-                        _sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert!);
-                    }
+                    UpdateCertificateContext(selectedCert);
                 }
                 else
                 {
-                    if (selectedCert != null)
-                    {
-                        _sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert!);
-                    }
+                    UpdateCertificateContext(selectedCert);
 
                     _credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions, newCredentialsRequested);
                     thumbPrint = guessedThumbPrint; // Delay until here in case something above threw.
                 }
             }
             finally
             {
-                if (selectedCert != null)
-                {
-                    _sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert);
-                }
+                UpdateCertificateContext(selectedCert);
             }
 
             return cachedCred;
+
+            void UpdateCertificateContext(X509Certificate2? cert)
+            {
+                if (cert != null && _sslAuthenticationOptions.CertificateContext == null)
+                {
+                    _sslAuthenticationOptions.SetCertificateContextFromCert(cert);
+                }
+            }
         }
 
         private static List<T> EnsureInitialized<T>(ref List<T>? list) => list ??= new List<T>();
@@ -737,7 +730,7 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
                 }
 
                 Debug.Assert(localCertificate.Equals(selectedCert), "'selectedCert' does not match 'localCertificate'.");
-                _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert);
+                _sslAuthenticationOptions.SetCertificateContextFromCert(selectedCert);
             }
 
             Debug.Assert(_sslAuthenticationOptions.CertificateContext != null);
@@ -1048,6 +1041,13 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
             bool success = false;
             X509Chain? chain = null;
 
+            // We need to note the number of certs in ExtraStore that were
+            // provided (by the user), we will add more from the received peer
+            // chain and we want to dispose only these after we perform the
+            // validation.
+            // TODO: this forces allocation of X509Certificate2Collection
+            int preexistingExtraCertsCount = _sslAuthenticationOptions.CertificateChainPolicy?.ExtraStore?.Count ?? 0;
+
             try
             {
                 X509Certificate2? certificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, ref chain, _sslAuthenticationOptions.CertificateChainPolicy);
@@ -1152,6 +1152,12 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
 
                 if (chain != null)
                 {
+                    // Dispose only the certificates that were added by GetRemoteCertificate
+                    for (int i = preexistingExtraCertsCount; i < chain.ChainPolicy.ExtraStore.Count; i++)
+                    {
+                        chain.ChainPolicy.ExtraStore[i].Dispose();
+                    }
+
                     int elementsCount = chain.ChainElements.Count;
                     for (int i = 0; i < elementsCount; i++)
                     {

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

@@ -416,5 +416,19 @@ private static string MakeUrl(string baseUri, ArraySegment<char> encodedRequest)
 
             return uriString;
         }
+
+        partial void ReleasePlatformSpecificResources()
+        {
+            Debug.Assert(_staplingForbidden, "Shouldn't release resources while OCSP stapling may be happening on the background.");
+
+            CertificateHandle.Dispose();
+            KeyHandle.Dispose();
+            _rootCertificate?.Dispose();
+
+            foreach (X509Certificate2 cert in _privateIntermediateCertificates)
+            {
+                cert.Dispose();
+            }
+        }
     }
 }

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs

@@ -42,6 +42,8 @@ private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection<
                         count++;
                     }
 
+                    DisposeChainElements(chain);
+
                     // OS failed to build the chain but we have at least some intermediates.
                     // We will try to add them to "Intermediate Certification Authorities" store.
                     if (!osCanBuildChain)
@@ -92,6 +94,8 @@ private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection<
                                     }
                                 }
 
+                                DisposeChainElements(chain);
+
                                 if (!osCanBuildChain)
                                 {
                                     // Add also root to Intermediate CA store so OS can complete building chain.
@@ -123,6 +127,15 @@ static void TryAddToStore(X509Store store, X509Certificate2 certificate)
                     }
                 }
             }
+
+            static void DisposeChainElements(X509Chain chain)
+            {
+                int elementsCount = chain.ChainElements.Count;
+                for (int i = 0; i < elementsCount; i++)
+                {
+                    chain.ChainElements[i].Certificate.Dispose();
+                }
+            }
         }
     }
 }

src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs

@@ -169,7 +169,29 @@ internal static SslStreamCertificateContext Create(
 
         internal SslStreamCertificateContext Duplicate()
         {
-            return new SslStreamCertificateContext(new X509Certificate2(TargetCertificate), IntermediateCertificates, Trust);
+            // Create will internally clone any certificates that it will
+            // retain, so we don't have to duplicate any of the instances here
+            X509Certificate2Collection intermediates = new X509Certificate2Collection();
+            foreach (X509Certificate2 cert in IntermediateCertificates)
+            {
+                intermediates.Add(cert);
+            }
+
+            return Create(new X509Certificate2(TargetCertificate), intermediates, trust: Trust);
+        }
+
+        internal void ReleaseResources()
+        {
+            // TargetCertificate is owned by the user, but we have created the cert context
+            // which looked up intermediate certificates and only we have reference to them.
+            foreach (X509Certificate2 cert in IntermediateCertificates)
+            {
+                cert.Dispose();
+            }
+
+            ReleasePlatformSpecificResources();
         }
+
+        partial void ReleasePlatformSpecificResources();
     }
 }

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs

@@ -114,6 +114,10 @@ public async Task SslStream_RequireClientCert_IsMutuallyAuthenticated_ReturnsTru
                     Assert.False(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated");
                 }
             }
+
+            // Assert that the certificates are not being disposed
+            Assert.NotEqual(_clientCertificate.Handle, IntPtr.Zero);
+            Assert.NotEqual(_serverCertificate.Handle, IntPtr.Zero);
         }
 
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))]
@@ -152,7 +156,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
                     // mutual authentication should only be set if server required client cert
                     Assert.Equal(expectMutualAuthentication, server.IsMutuallyAuthenticated);
                     Assert.Equal(expectMutualAuthentication, client.IsMutuallyAuthenticated);
-                };
+                }
+                ;
             }
         }
 
@@ -268,7 +273,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
                     {
                         Assert.Null(server.RemoteCertificate);
                     }
-                };
+                }
+                ;
             }
         }
 
@@ -322,7 +328,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
                     {
                         Assert.Null(server.RemoteCertificate);
                     }
-                };
+                }
+                ;
             }
         }
 
@@ -380,7 +387,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
                     {
                         Assert.Null(server.RemoteCertificate);
                     }
-                };
+                }
+                ;
             }
         }
 

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs

@@ -807,6 +807,18 @@ public async Task SslStream_ServerUntrustedCaWithCustomTrust_OK(bool usePartialC
 
                 await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2);
             }
+
+            // the ownership of the ExtraStore cert is not transferred to the
+            // SslStream, so we need to verify that the certs are still valid.
+            foreach (X509Certificate2 cert in clientOptions.CertificateChainPolicy.ExtraStore)
+            {
+                Assert.NotEqual(cert.Handle, IntPtr.Zero);
+            }
+
+            foreach (X509Certificate2 cert in clientOptions.CertificateChainPolicy.CustomTrustStore)
+            {
+                Assert.NotEqual(cert.Handle, IntPtr.Zero);
+            }
         }
 
         private async Task SslStream_ClientSendsChain_Core(SslClientAuthenticationOptions clientOptions, X509Certificate2Collection clientChain)