diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs index 0236ff306152bd..c70cbb3bb4e8ae 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs +++ b/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? 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 + } } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs index 4e9f4327ee9e22..807b727747b800 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs +++ b/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,17 +633,11 @@ 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. @@ -657,13 +645,18 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredential } 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 EnsureInitialized(ref List? list) => list ??= new List(); @@ -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++) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index 06fd705edb8aca..e5073e79538531 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -416,5 +416,19 @@ private static string MakeUrl(string baseUri, ArraySegment 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(); + } + } } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs index cc1a881d6f2a72..e84497cfe2db11 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs +++ b/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(); + } + } } } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs index b3540d646a1d25..b48b32934cf8bb 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs +++ b/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(); } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index e8ddb07f82df76..d87b536f5f6d09 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/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); } - }; + } + ; } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index aadf36b19d114f..811b6932e896de 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/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)