From 32f5873a1d2d72722f29618fd9f1d5556dae8f65 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 15 Jul 2022 15:20:55 -0400 Subject: [PATCH] Fix some SafeHandle finalization in System.Net.Security (#72189) There's still a lot more, but most of it appears to be inevitable given the current design and public APIs in the library, e.g. SslStreamCertificateContext creating resources it internally stores and not providing any way to explicitly clean them up. --- .../src/Interop/Windows/SspiCli/SSPIWrapper.cs | 1 + .../Interop/Windows/SspiCli/SecuritySafeHandles.cs | 12 +++- .../Common/tests/System/Net/Capability.Security.cs | 25 +++++++-- .../Common/tests/System/Net/HttpsTestClient.cs | 11 +++- .../X509Certificates/CertificateAuthority.cs | 3 + .../src/System/Net/Security/SslStream.Protocol.cs | 65 ++++++++++++++++------ .../EnterpriseTests/NegotiateStreamLoopbackTest.cs | 9 ++- .../CertificateValidationClientServer.cs | 32 ++++++----- .../FunctionalTests/NegotiateStreamKerberosTest.cs | 2 + .../NegotiateStreamStreamToStreamTest.cs | 56 ++++++++++++------- .../SslStreamAllowRenegotiationTests.cs | 6 +- .../FunctionalTests/SslStreamConformanceTests.cs | 2 +- .../tests/FunctionalTests/SslStreamEKUTest.cs | 25 +++++---- .../SslStreamMutualAuthenticationTest.cs | 8 ++- .../SslStreamNegotiatedCipherSuiteTest.cs | 3 +- .../FunctionalTests/SslStreamNetworkStreamTest.cs | 11 +++- .../tests/FunctionalTests/SslStreamSniTest.cs | 6 +- .../FunctionalTests/SslStreamStreamToStreamTest.cs | 16 +++++- .../tests/FunctionalTests/TestConfiguration.cs | 6 +- .../tests/FunctionalTests/TestHelper.cs | 2 + .../tests/FunctionalTests/TransportContextTest.cs | 6 +- .../UnitTests/NegotiateAuthenticationTests.cs | 15 +++-- .../X509Certificates/StorePal.Windows.cs | 2 + .../X509Certificates/WindowsInterop.crypt32.cs | 8 ++- 24 files changed, 231 insertions(+), 101 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs index fe83809..81fdba8 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs @@ -192,6 +192,7 @@ namespace System.Net int errorCode = secModule.QueryContextChannelBinding(securityContext, contextAttribute, out result); if (errorCode != 0) { + result.Dispose(); if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(null, $"ERROR = {ErrorDescription(errorCode)}"); return null; } diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 8cd613b..a57d85a 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -441,7 +441,10 @@ namespace System.Net.Security // incorrect arguments to InitializeSecurityContextW in cases where an "contextHandle" was // already present and non-zero. if (isContextAbsent) + { + refContext?.Dispose(); refContext = new SafeDeleteSslContext(); + } } if (targetName == null || targetName.Length == 0) @@ -1114,15 +1117,18 @@ namespace System.Net.Security return status; } + bool refAdded = false; try { - bool ignore = false; - phContext.DangerousAddRef(ref ignore); + phContext.DangerousAddRef(ref refAdded); status = Interop.SspiCli.QueryContextAttributesW(ref phContext._handle, contextAttribute, buffer); } finally { - phContext.DangerousRelease(); + if (refAdded) + { + phContext.DangerousRelease(); + } } if (status == 0 && refHandle != null) diff --git a/src/libraries/Common/tests/System/Net/Capability.Security.cs b/src/libraries/Common/tests/System/Net/Capability.Security.cs index a11fe6e..2455109 100644 --- a/src/libraries/Common/tests/System/Net/Capability.Security.cs +++ b/src/libraries/Common/tests/System/Net/Capability.Security.cs @@ -70,12 +70,29 @@ namespace System.Net.Test.Common { store.Open(OpenFlags.ReadOnly); - X509Certificate2Collection certs = - store.Certificates.Find(X509FindType.FindByThumbprint, CARootThumbprint, false); + X509Certificate2Collection? certs = null; + X509Certificate2Collection? found = null; + try + { + certs = store.Certificates; + found = certs.Find(X509FindType.FindByThumbprint, CARootThumbprint, false); - if (certs.Count == 1) + if (found.Count == 1) + { + return true; + } + } + finally { - return true; + if (found != null) + { + foreach (X509Certificate2 c in found) c.Dispose(); + } + + if (certs != null) + { + foreach (X509Certificate2 c in certs) c.Dispose(); + } } } diff --git a/src/libraries/Common/tests/System/Net/HttpsTestClient.cs b/src/libraries/Common/tests/System/Net/HttpsTestClient.cs index 7cc475b..fe6d5ce 100644 --- a/src/libraries/Common/tests/System/Net/HttpsTestClient.cs +++ b/src/libraries/Common/tests/System/Net/HttpsTestClient.cs @@ -11,7 +11,7 @@ using System.Threading.Tasks; namespace System.Net.Test.Common { - public class HttpsTestClient + public class HttpsTestClient : IDisposable { public class Options { @@ -39,9 +39,9 @@ User-Agent: Testing application public SslPolicyErrors IgnoreSslPolicyErrors { get; set; } = SslPolicyErrors.None; } - private Options _options; + private readonly Options _options; private int _requestCount = 0; - private VerboseTestLogging _log = VerboseTestLogging.GetInstance(); + private readonly VerboseTestLogging _log = VerboseTestLogging.GetInstance(); public SslStream Stream { get; private set; } @@ -50,6 +50,11 @@ User-Agent: Testing application _options = options ?? throw new ArgumentNullException(nameof(options)); } + public void Dispose() + { + _options.ClientCertificate?.Dispose(); + } + public async Task HttpsRequestAsync(Func> httpConversation = null) { _log.WriteLine("[Client] Disabling SslPolicyErrors: {0}", _options.IgnoreSslPolicyErrors.ToString()); diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs index 27cbec0..c674bf6 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs @@ -118,6 +118,7 @@ namespace System.Security.Cryptography.X509Certificates.Tests.Common public void Dispose() { _cert.Dispose(); + _ocspResponder?.Dispose(); } internal string SubjectName => _cert.Subject; @@ -899,7 +900,9 @@ SingleResponse ::= SEQUENCE { eeKey, extensions); + X509Certificate2 tmp = endEntityCert; endEntityCert = endEntityCert.CopyWithPrivateKey(eeKey); + tmp.Dispose(); } if (registerAuthorities) 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 d9db330..4f9a093 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 @@ -156,33 +156,60 @@ namespace System.Net.Security } } - X509Certificate2Collection collectionEx; string certHash = certEx!.Thumbprint; // ELSE Try the MY user and machine stores for private key check. // For server side mode MY machine store takes priority. - X509Store? store = CertificateValidationPal.EnsureStoreOpened(isServer); - if (store != null) + X509Certificate2? found = + FindCertWithPrivateKey(isServer) ?? + FindCertWithPrivateKey(!isServer); + if (found is not null) { - collectionEx = store.Certificates.Find(X509FindType.FindByThumbprint, certHash, false); - if (collectionEx.Count > 0 && collectionEx[0].HasPrivateKey) - { - if (NetEventSource.Log.IsEnabled()) - NetEventSource.Log.FoundCertInStore(isServer, instance); - return collectionEx[0]; - } + return found; } - store = CertificateValidationPal.EnsureStoreOpened(!isServer); - if (store != null) + X509Certificate2? FindCertWithPrivateKey(bool isServer) { - collectionEx = store.Certificates.Find(X509FindType.FindByThumbprint, certHash, false); - if (collectionEx.Count > 0 && collectionEx[0].HasPrivateKey) + if (CertificateValidationPal.EnsureStoreOpened(isServer) is X509Store store) { - if (NetEventSource.Log.IsEnabled()) - NetEventSource.Log.FoundCertInStore(!isServer, instance); - return collectionEx[0]; + X509Certificate2Collection certs = store.Certificates; + X509Certificate2Collection found = certs.Find(X509FindType.FindByThumbprint, certHash, false); + X509Certificate2? cert = null; + try + { + if (found.Count > 0) + { + cert = found[0]; + if (cert.HasPrivateKey) + { + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Log.FoundCertInStore(isServer, instance); + } + + return cert; + } + } + } + finally + { + for (int i = 0; i < found.Count; i++) + { + X509Certificate2 toDispose = found[i]; + if (!ReferenceEquals(toDispose, cert)) + { + toDispose.Dispose(); + } + } + + for (int i = 0; i < certs.Count; i++) + { + certs[i].Dispose(); + } + } } + + return null; } } catch (CryptographicException) @@ -937,11 +964,13 @@ namespace System.Net.Security try { X509Certificate2? certificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, ref chain, _sslAuthenticationOptions.CertificateChainPolicy); - if (_remoteCertificate != null && certificate != null && + if (_remoteCertificate != null && + certificate != null && certificate.RawDataMemory.Span.SequenceEqual(_remoteCertificate.RawDataMemory.Span)) { // This is renegotiation or TLS 1.3 and the certificate did not change. // There is no reason to process callback again as we already established trust. + certificate.Dispose(); return true; } diff --git a/src/libraries/System.Net.Security/tests/EnterpriseTests/NegotiateStreamLoopbackTest.cs b/src/libraries/System.Net.Security/tests/EnterpriseTests/NegotiateStreamLoopbackTest.cs index d6815f5..8dac58a 100644 --- a/src/libraries/System.Net.Security/tests/EnterpriseTests/NegotiateStreamLoopbackTest.cs +++ b/src/libraries/System.Net.Security/tests/EnterpriseTests/NegotiateStreamLoopbackTest.cs @@ -223,9 +223,12 @@ namespace System.Net.Security.Enterprise.Tests Assert.False(stream.LeaveInnerStreamOpen); IIdentity remoteIdentity = stream.RemoteIdentity; - Assert.Equal("Kerberos", remoteIdentity.AuthenticationType); - Assert.True(remoteIdentity.IsAuthenticated); - Assert.Equal(remoteName, remoteIdentity.Name); + using (remoteIdentity as IDisposable) + { + Assert.Equal("Kerberos", remoteIdentity.AuthenticationType); + Assert.True(remoteIdentity.IsAuthenticated); + Assert.Equal(remoteName, remoteIdentity.Name); + } } } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs index 3032267..06fc46b 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs @@ -157,7 +157,6 @@ namespace System.Net.Security.Tests serverConnection.GetStream(), false, ServerSideRemoteClientCertificateValidation)) - { string serverName = _serverCertificate.GetNameInfo(X509NameType.SimpleName, false); var clientCerts = new X509CertificateCollection(); @@ -181,22 +180,25 @@ namespace System.Net.Security.Tests await TestConfiguration.WhenAllOrAnyFailedWithTimeout(clientAuthentication, serverAuthentication); - if (!_clientCertificateRemovedByFilter) + using (sslServerStream.RemoteCertificate) { - Assert.True(sslClientStream.IsMutuallyAuthenticated, "sslClientStream.IsMutuallyAuthenticated"); - Assert.True(sslServerStream.IsMutuallyAuthenticated, "sslServerStream.IsMutuallyAuthenticated"); - - Assert.Equal(sslServerStream.RemoteCertificate.Subject, _clientCertificate.Subject); + if (!_clientCertificateRemovedByFilter) + { + Assert.True(sslClientStream.IsMutuallyAuthenticated, "sslClientStream.IsMutuallyAuthenticated"); + Assert.True(sslServerStream.IsMutuallyAuthenticated, "sslServerStream.IsMutuallyAuthenticated"); + + Assert.Equal(sslServerStream.RemoteCertificate.Subject, _clientCertificate.Subject); + } + else + { + Assert.False(sslClientStream.IsMutuallyAuthenticated, "sslClientStream.IsMutuallyAuthenticated"); + Assert.False(sslServerStream.IsMutuallyAuthenticated, "sslServerStream.IsMutuallyAuthenticated"); + + Assert.Null(sslServerStream.RemoteCertificate); + } + + Assert.Equal(sslClientStream.RemoteCertificate.Subject, _serverCertificate.Subject); } - else - { - Assert.False(sslClientStream.IsMutuallyAuthenticated, "sslClientStream.IsMutuallyAuthenticated"); - Assert.False(sslServerStream.IsMutuallyAuthenticated, "sslServerStream.IsMutuallyAuthenticated"); - - Assert.Null(sslServerStream.RemoteCertificate); - } - - Assert.Equal(sslClientStream.RemoteCertificate.Subject, _serverCertificate.Subject); } } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs index df74c839..8ddb36f 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs @@ -150,6 +150,7 @@ namespace System.Net.Security.Tests Assert.Equal(expectedAuthenticationType, auth.RemoteIdentity.AuthenticationType); Assert.Equal(serverSPN, auth.RemoteIdentity.Name); + (auth.RemoteIdentity as IDisposable)?.Dispose(); Assert.True(auth.IsAuthenticated); Assert.True(auth.IsEncrypted); @@ -195,6 +196,7 @@ namespace System.Net.Security.Tests Assert.Equal(expectedAuthenticationType, serverAuth.RemoteIdentity.AuthenticationType); Assert.Equal(expectedUser, serverAuth.RemoteIdentity.Name); + (serverAuth.RemoteIdentity as IDisposable)?.Dispose(); // Receive a message from the client. var message = "Hello from the client."; diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs index 76a1c31..7b8cc18 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs @@ -63,9 +63,12 @@ namespace System.Net.Security.Tests Assert.False(client.LeaveInnerStreamOpen); IIdentity serverIdentity = client.RemoteIdentity; - Assert.Equal("NTLM", serverIdentity.AuthenticationType); - Assert.False(serverIdentity.IsAuthenticated); - Assert.Equal("", serverIdentity.Name); + using (serverIdentity as IDisposable) + { + Assert.Equal("NTLM", serverIdentity.AuthenticationType); + Assert.False(serverIdentity.IsAuthenticated); + Assert.Equal("", serverIdentity.Name); + } // Expected Server property values: Assert.True(server.IsAuthenticated); @@ -77,11 +80,14 @@ namespace System.Net.Security.Tests Assert.False(server.LeaveInnerStreamOpen); IIdentity clientIdentity = server.RemoteIdentity; - Assert.Equal("NTLM", clientIdentity.AuthenticationType); + using (clientIdentity as IDisposable) + { + Assert.Equal("NTLM", clientIdentity.AuthenticationType); - Assert.True(clientIdentity.IsAuthenticated); + Assert.True(clientIdentity.IsAuthenticated); - IdentityValidator.AssertIsCurrentIdentity(clientIdentity); + IdentityValidator.AssertIsCurrentIdentity(clientIdentity); + } } } @@ -153,9 +159,12 @@ namespace System.Net.Security.Tests Assert.False(client.LeaveInnerStreamOpen); IIdentity serverIdentity = client.RemoteIdentity; - Assert.Equal("NTLM", serverIdentity.AuthenticationType); - Assert.True(serverIdentity.IsAuthenticated); - Assert.Equal(targetName, serverIdentity.Name); + using (serverIdentity as IDisposable) + { + Assert.Equal("NTLM", serverIdentity.AuthenticationType); + Assert.True(serverIdentity.IsAuthenticated); + Assert.Equal(targetName, serverIdentity.Name); + } // Expected Server property values: Assert.True(server.IsAuthenticated); @@ -167,11 +176,14 @@ namespace System.Net.Security.Tests Assert.False(server.LeaveInnerStreamOpen); IIdentity clientIdentity = server.RemoteIdentity; - Assert.Equal("NTLM", clientIdentity.AuthenticationType); + using (clientIdentity as IDisposable) + { + Assert.Equal("NTLM", clientIdentity.AuthenticationType); - Assert.True(clientIdentity.IsAuthenticated); + Assert.True(clientIdentity.IsAuthenticated); - IdentityValidator.AssertIsCurrentIdentity(clientIdentity); + IdentityValidator.AssertIsCurrentIdentity(clientIdentity); + } } } @@ -210,9 +222,12 @@ namespace System.Net.Security.Tests Assert.False(client.LeaveInnerStreamOpen); IIdentity serverIdentity = client.RemoteIdentity; - Assert.Equal("NTLM", serverIdentity.AuthenticationType); - Assert.True(serverIdentity.IsAuthenticated); - Assert.Equal(targetName, serverIdentity.Name); + using (serverIdentity as IDisposable) + { + Assert.Equal("NTLM", serverIdentity.AuthenticationType); + Assert.True(serverIdentity.IsAuthenticated); + Assert.Equal(targetName, serverIdentity.Name); + } // Expected Server property values: Assert.True(server.IsAuthenticated); @@ -224,12 +239,15 @@ namespace System.Net.Security.Tests Assert.False(server.LeaveInnerStreamOpen); IIdentity clientIdentity = server.RemoteIdentity; - Assert.Equal("NTLM", clientIdentity.AuthenticationType); + using (clientIdentity as IDisposable) + { + Assert.Equal("NTLM", clientIdentity.AuthenticationType); - Assert.False(clientIdentity.IsAuthenticated); - // On .NET Desktop: Assert.True(clientIdentity.IsAuthenticated); + Assert.False(clientIdentity.IsAuthenticated); + // On .NET Desktop: Assert.True(clientIdentity.IsAuthenticated); - IdentityValidator.AssertHasName(clientIdentity, new SecurityIdentifier(WellKnownSidType.AnonymousSid, null).Translate(typeof(NTAccount)).Value); + IdentityValidator.AssertHasName(clientIdentity, new SecurityIdentifier(WellKnownSidType.AnonymousSid, null).Translate(typeof(NTAccount)).Value); + } } } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowRenegotiationTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowRenegotiationTests.cs index 469c0a1..233f182 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowRenegotiationTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowRenegotiationTests.cs @@ -35,10 +35,11 @@ namespace System.Net.Security.Tests Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); await s.ConnectAsync(Configuration.Security.TlsRenegotiationServer, 443); using (NetworkStream ns = new NetworkStream(s)) + using (X509Certificate2 clientCert = Configuration.Certificates.GetClientCertificate()) using (SslStream ssl = new SslStream(ns, true, validationCallback)) { X509CertificateCollection certBundle = new X509CertificateCollection(); - certBundle.Add(Configuration.Certificates.GetClientCertificate()); + certBundle.Add(clientCert); SslClientAuthenticationOptions options = new SslClientAuthenticationOptions { @@ -74,10 +75,11 @@ namespace System.Net.Security.Tests Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); await s.ConnectAsync(Configuration.Security.TlsRenegotiationServer, 443); using (NetworkStream ns = new NetworkStream(s)) + using (X509Certificate2 clientCert = Configuration.Certificates.GetClientCertificate()) using (SslStream ssl = new SslStream(ns, true)) { X509CertificateCollection certBundle = new X509CertificateCollection(); - certBundle.Add(Configuration.Certificates.GetClientCertificate()); + certBundle.Add(clientCert); SslClientAuthenticationOptions options = new SslClientAuthenticationOptions { diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs index 0fe9b00..1e12db5a 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs @@ -21,7 +21,7 @@ namespace System.Net.Security.Tests protected override async Task CreateWrappedConnectedStreamsAsync(StreamPair wrapped, bool leaveOpen = false) { - X509Certificate2? cert = Test.Common.Configuration.Certificates.GetServerCertificate(); + using X509Certificate2 cert = Test.Common.Configuration.Certificates.GetServerCertificate(); var ssl1 = new SslStream(wrapped.Stream1, leaveOpen, delegate { return true; }); var ssl2 = new SslStream(wrapped.Stream2, leaveOpen, delegate { return true; }); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamEKUTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamEKUTest.cs index 21880cd..be751ac 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamEKUTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamEKUTest.cs @@ -20,13 +20,13 @@ namespace System.Net.Security.Tests public const int TestTimeoutMilliseconds = 15 * 1000; - public static X509Certificate2 serverCertificateServerEku = Configuration.Certificates.GetServerCertificate(); - public static X509Certificate2 serverCertificateNoEku = Configuration.Certificates.GetNoEKUCertificate(); - public static X509Certificate2 serverCertificateWrongEku = Configuration.Certificates.GetClientCertificate(); + public static readonly X509Certificate2 serverCertificateServerEku = Configuration.Certificates.GetServerCertificate(); + public static readonly X509Certificate2 serverCertificateNoEku = Configuration.Certificates.GetNoEKUCertificate(); + public static readonly X509Certificate2 serverCertificateWrongEku = Configuration.Certificates.GetClientCertificate(); - public static X509Certificate2 clientCertificateWrongEku = Configuration.Certificates.GetServerCertificate(); - public static X509Certificate2 clientCertificateNoEku = Configuration.Certificates.GetNoEKUCertificate(); - public static X509Certificate2 clientCertificateClientEku = Configuration.Certificates.GetClientCertificate(); + public static readonly X509Certificate2 clientCertificateWrongEku = Configuration.Certificates.GetServerCertificate(); + public static readonly X509Certificate2 clientCertificateNoEku = Configuration.Certificates.GetNoEKUCertificate(); + public static readonly X509Certificate2 clientCertificateClientEku = Configuration.Certificates.GetClientCertificate(); [ConditionalFact(nameof(IsRootCertificateInstalled))] public async Task SslStream_NoEKUServerAuth_Ok() @@ -41,7 +41,7 @@ namespace System.Net.Security.Tests var clientOptions = new HttpsTestClient.Options(new IPEndPoint(IPAddress.Loopback, server.Port)); clientOptions.ServerName = serverOptions.ServerCertificate.GetNameInfo(X509NameType.SimpleName, false); - var client = new HttpsTestClient(clientOptions); + using var client = new HttpsTestClient(clientOptions); var tasks = new Task[2]; tasks[0] = server.AcceptHttpsClientAsync(); @@ -64,7 +64,7 @@ namespace System.Net.Security.Tests var clientOptions = new HttpsTestClient.Options(new IPEndPoint(IPAddress.Loopback, server.Port)); clientOptions.ServerName = serverOptions.ServerCertificate.GetNameInfo(X509NameType.SimpleName, false); - var client = new HttpsTestClient(clientOptions); + using var client = new HttpsTestClient(clientOptions); var tasks = new Task[2]; tasks[0] = server.AcceptHttpsClientAsync(); @@ -89,7 +89,7 @@ namespace System.Net.Security.Tests clientOptions.ServerName = serverOptions.ServerCertificate.GetNameInfo(X509NameType.SimpleName, false); clientOptions.ClientCertificate = clientCertificateNoEku; - var client = new HttpsTestClient(clientOptions); + using var client = new HttpsTestClient(clientOptions); var tasks = new Task[2]; tasks[0] = server.AcceptHttpsClientAsync(); @@ -114,7 +114,7 @@ namespace System.Net.Security.Tests clientOptions.ServerName = serverOptions.ServerCertificate.GetNameInfo(X509NameType.SimpleName, false); clientOptions.ClientCertificate = clientCertificateWrongEku; - var client = new HttpsTestClient(clientOptions); + using var client = new HttpsTestClient(clientOptions); var tasks = new Task[2]; tasks[0] = server.AcceptHttpsClientAsync(); @@ -143,15 +143,16 @@ namespace System.Net.Security.Tests serverOptions.RequireClientAuthentication = true; serverOptions.IgnoreSslPolicyErrors = SslPolicyErrors.RemoteCertificateChainErrors; + using (X509Certificate2 clientCert = Configuration.Certificates.GetSelfSignedClientCertificate()) using (var server = new HttpsTestServer(serverOptions)) { server.Start(); var clientOptions = new HttpsTestClient.Options(new IPEndPoint(IPAddress.Loopback, server.Port)); clientOptions.ServerName = serverOptions.ServerCertificate.GetNameInfo(X509NameType.SimpleName, false); - clientOptions.ClientCertificate = Configuration.Certificates.GetSelfSignedClientCertificate(); + clientOptions.ClientCertificate = clientCert; - var client = new HttpsTestClient(clientOptions); + using var client = new HttpsTestClient(clientOptions); var tasks = new Task[2]; tasks[0] = server.AcceptHttpsClientAsync(); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index 3ad072f..5097742 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -12,7 +12,7 @@ namespace System.Net.Security.Tests { using Configuration = System.Net.Test.Common.Configuration; - public class SslStreamMutualAuthenticationTest + public class SslStreamMutualAuthenticationTest : IDisposable { private readonly X509Certificate2 _clientCertificate; private readonly X509Certificate2 _serverCertificate; @@ -23,6 +23,12 @@ namespace System.Net.Security.Tests _clientCertificate = Configuration.Certificates.GetClientCertificate(); } + public void Dispose() + { + _serverCertificate.Dispose(); + _clientCertificate.Dispose(); + } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] [InlineData(false, false)] [InlineData(false, true)] diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs index a110bc8..a1d9eae 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs @@ -696,11 +696,12 @@ namespace System.Net.Security.Tests using (clientStream) using (serverStream) + using (X509Certificate2 serverCert = Configuration.Certificates.GetSelfSignedServerCertificate()) using (SslStream server = new SslStream(serverStream, leaveInnerStreamOpen: false), client = new SslStream(clientStream, leaveInnerStreamOpen: false)) { var serverOptions = new SslServerAuthenticationOptions(); - serverOptions.ServerCertificate = Configuration.Certificates.GetSelfSignedServerCertificate(); + serverOptions.ServerCertificate = serverCert; serverOptions.EncryptionPolicy = serverParams.EncryptionPolicy; serverOptions.EnabledSslProtocols = serverParams.SslProtocols; serverOptions.CipherSuitesPolicy = serverParams.CipherSuitesPolicy; diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 1d1ebad..fde7825 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -153,10 +153,11 @@ namespace System.Net.Security.Tests Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); await s.ConnectAsync(Configuration.Security.TlsRenegotiationServer, 443); using (NetworkStream ns = new NetworkStream(s)) + using (X509Certificate2 clientCert = Configuration.Certificates.GetClientCertificate()) using (SslStream ssl = new SslStream(ns, true, validationCallback)) { X509CertificateCollection certBundle = new X509CertificateCollection(); - certBundle.Add(Configuration.Certificates.GetClientCertificate()); + certBundle.Add(clientCert); // Perform handshake to establish secure connection. await ssl.AuthenticateAsClientAsync(Configuration.Security.TlsRenegotiationServer, certBundle, SslProtocols.Tls12, false); @@ -256,6 +257,8 @@ namespace System.Net.Security.Tests // verify that the session is usable with or without client's certificate await TestHelper.PingPong(client, server, cts.Token); await TestHelper.PingPong(server, client, cts.Token); + + server.RemoteCertificate?.Dispose(); } } @@ -333,6 +336,8 @@ namespace System.Net.Security.Tests // verify that the session is usable with or without client's certificate await TestHelper.PingPong(client, server, cts.Token); await TestHelper.PingPong(server, client, cts.Token); + + server.RemoteCertificate?.Dispose(); } } @@ -560,6 +565,8 @@ namespace System.Net.Security.Tests // verify that the session is usable with or without client's certificate await TestHelper.PingPong(client, server, cts.Token); await TestHelper.PingPong(server, client, cts.Token); + + server.RemoteCertificate?.Dispose(); } } @@ -619,6 +626,8 @@ namespace System.Net.Security.Tests await t; await Assert.ThrowsAsync(() => server.NegotiateClientCertificateAsync()); + + server.RemoteCertificate?.Dispose(); } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSniTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSniTest.cs index e544b63..944ab95 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSniTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSniTest.cs @@ -21,7 +21,7 @@ namespace System.Net.Security.Tests [ActiveIssue("https://github.com/dotnet/runtime/issues/68206", TestPlatforms.Android)] public async Task SslStream_ClientSendsSNIServerReceives_Ok(string hostName) { - X509Certificate serverCert = Configuration.Certificates.GetSelfSignedServerCertificate(); + using X509Certificate serverCert = Configuration.Certificates.GetSelfSignedServerCertificate(); await WithVirtualConnection(async (server, client) => { @@ -55,7 +55,7 @@ namespace System.Net.Security.Tests [MemberData(nameof(HostNameData))] public async Task SslStream_ServerCallbackAndLocalCertificateSelectionSet_Throws(string hostName) { - X509Certificate serverCert = Configuration.Certificates.GetSelfSignedServerCertificate(); + using X509Certificate serverCert = Configuration.Certificates.GetSelfSignedServerCertificate(); int timesCallbackCalled = 0; @@ -99,7 +99,7 @@ namespace System.Net.Security.Tests [ActiveIssue("https://github.com/dotnet/runtime/issues/68206", TestPlatforms.Android)] public async Task SslStream_ServerCallbackNotSet_UsesLocalCertificateSelection(string hostName) { - X509Certificate serverCert = Configuration.Certificates.GetSelfSignedServerCertificate(); + using X509Certificate serverCert = Configuration.Certificates.GetSelfSignedServerCertificate(); int timesCallbackCalled = 0; diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs index 873f754..c195103 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs @@ -485,9 +485,19 @@ namespace System.Net.Security.Tests X509CertificateCollection clientCerts = clientCertificate != null ? new X509CertificateCollection() { clientCertificate } : null; await WithServerCertificate(serverCertificate, async (certificate, name) => { - Task t1 = Task.Factory.FromAsync(clientSslStream.BeginAuthenticateAsClient(name, clientCerts, SslProtocols.None, checkCertificateRevocation: false, null, null), clientSslStream.EndAuthenticateAsClient); - Task t2 = Task.Factory.FromAsync(serverSslStream.BeginAuthenticateAsServer(certificate, clientCertificateRequired: clientCertificate != null, checkCertificateRevocation: false, null, null), serverSslStream.EndAuthenticateAsServer); - await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); + IAsyncResult clientBeginAuth = clientSslStream.BeginAuthenticateAsClient(name, clientCerts, SslProtocols.None, checkCertificateRevocation: false, null, null); + IAsyncResult serverBeginAuth = serverSslStream.BeginAuthenticateAsServer(certificate, clientCertificateRequired: clientCertificate != null, checkCertificateRevocation: false, null, null); + try + { + Task t1 = Task.Factory.FromAsync(clientBeginAuth, clientSslStream.EndAuthenticateAsClient); + Task t2 = Task.Factory.FromAsync(serverBeginAuth, serverSslStream.EndAuthenticateAsServer); + await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); + } + finally + { + clientBeginAuth.AsyncWaitHandle.Dispose(); + serverBeginAuth.AsyncWaitHandle.Dispose(); + } }); } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestConfiguration.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestConfiguration.cs index 17db97d..769f905 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/TestConfiguration.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/TestConfiguration.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.IO; using System.Security.Cryptography.X509Certificates; using System.Threading.Tasks; @@ -28,7 +29,7 @@ namespace System.Net.Security.Tests public static bool SupportsHandshakeAlerts { get { return OperatingSystem.IsLinux() || OperatingSystem.IsWindows() || OperatingSystem.IsFreeBSD(); } } public static bool SupportsRenegotiation { get { return (OperatingSystem.IsWindows() && !PlatformDetection.IsWindows7) || ((OperatingSystem.IsLinux() || OperatingSystem.IsFreeBSD()) && PlatformDetection.OpenSslVersion >= new Version(1, 1, 1)); } } - public static X509Certificate2 ServerCertificate = System.Net.Test.Common.Configuration.Certificates.GetServerCertificate(); + public static readonly X509Certificate2 ServerCertificate = System.Net.Test.Common.Configuration.Certificates.GetServerCertificate(); public static Task WhenAllOrAnyFailedWithTimeout(params Task[] tasks) => tasks.WhenAllOrAnyFailed(PassingTestTimeoutMilliseconds); @@ -49,7 +50,8 @@ namespace System.Net.Security.Tests // New Windows can support null but it may be disabled in Azure images using (Process p = Process.Start(new ProcessStartInfo("powershell", "-Command Get-TlsCipherSuite") { RedirectStandardOutput = true, RedirectStandardError = true })) { - return p.StandardOutput.ReadToEnd().Contains("WITH_NULL"); + using StreamReader reader = p.StandardOutput; + return reader.ReadToEnd().Contains("WITH_NULL"); } } catch { return true; } // assume availability diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs index 62383aa..47e6b40 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs @@ -119,6 +119,7 @@ namespace System.Net.Security.Tests { store.Remove(cert); } + cert.Dispose(); } } } @@ -135,6 +136,7 @@ namespace System.Net.Security.Tests { store.Remove(cert); } + cert.Dispose(); } } } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TransportContextTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TransportContextTest.cs index 2de9d1a..4535fca 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/TransportContextTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/TransportContextTest.cs @@ -36,9 +36,9 @@ namespace System.Net.Security.Tests private static void CheckTransportContext(TransportContext context) { - var cbt1 = context.GetChannelBinding(ChannelBindingKind.Endpoint); - var cbt2 = context.GetChannelBinding(ChannelBindingKind.Unique); - var cbt3 = context.GetChannelBinding(ChannelBindingKind.Unknown); + using ChannelBinding cbt1 = context.GetChannelBinding(ChannelBindingKind.Endpoint); + using ChannelBinding cbt2 = context.GetChannelBinding(ChannelBindingKind.Unique); + using ChannelBinding cbt3 = context.GetChannelBinding(ChannelBindingKind.Unknown); CheckChannelBinding(ChannelBindingKind.Endpoint, cbt1); CheckChannelBinding(ChannelBindingKind.Unique, cbt2); diff --git a/src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs b/src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs index 0ccc266..eb2b3c4 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs +++ b/src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs @@ -6,9 +6,10 @@ using System.Buffers; using System.Buffers.Binary; using System.IO; using System.Net.Security; +using System.Net.Test.Common; +using System.Security.Principal; using System.Text; using System.Threading.Tasks; -using System.Net.Test.Common; using Xunit; namespace System.Net.Security.Tests @@ -34,7 +35,7 @@ namespace System.Net.Security.Tests { NegotiateAuthenticationClientOptions clientOptions = new NegotiateAuthenticationClientOptions { Credential = s_testCredentialRight, TargetName = "HTTP/foo" }; NegotiateAuthentication negotiateAuthentication = new NegotiateAuthentication(clientOptions); - Assert.Throws(() => { _ = negotiateAuthentication.RemoteIdentity; }); + Assert.Throws(() => negotiateAuthentication.RemoteIdentity); } [ConditionalFact(nameof(IsNtlmAvailable))] @@ -54,10 +55,12 @@ namespace System.Net.Security.Tests Assert.True(fakeNtlmServer.IsAuthenticated); Assert.True(negotiateAuthentication.IsAuthenticated); - _ = negotiateAuthentication.RemoteIdentity; - - negotiateAuthentication.Dispose(); - Assert.Throws(() => { _ = negotiateAuthentication.RemoteIdentity; }); + IIdentity remoteIdentity = negotiateAuthentication.RemoteIdentity; + using (remoteIdentity as IDisposable) + { + negotiateAuthentication.Dispose(); + Assert.Throws(() => negotiateAuthentication.RemoteIdentity); + } } [Fact] diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Windows.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Windows.cs index 8175045..bdc213f 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Windows.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Windows.cs @@ -66,6 +66,8 @@ namespace System.Security.Cryptography.X509Certificates return; // The certificate is not present in the store, simply return. Interop.Crypt32.CERT_CONTEXT* pCertContextToDelete = enumCertContext.Disconnect(); // CertDeleteCertificateFromContext always frees the context (even on error) + enumCertContext.Dispose(); + if (!Interop.Crypt32.CertDeleteCertificateFromStore(pCertContextToDelete)) throw Marshal.GetLastWin32Error().ToCryptographicException(); } diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/WindowsInterop.crypt32.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/WindowsInterop.crypt32.cs index 16e5225..ea9b0cf 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/WindowsInterop.crypt32.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/WindowsInterop.crypt32.cs @@ -150,7 +150,13 @@ internal static partial class Interop /// public static unsafe bool CertFindCertificateInStore(SafeCertStoreHandle hCertStore, Interop.Crypt32.CertFindType dwFindType, void* pvFindPara, [NotNull] ref SafeCertContextHandle? pCertContext) { - Interop.Crypt32.CERT_CONTEXT* pPrevCertContext = pCertContext == null ? null : pCertContext.Disconnect(); + Interop.Crypt32.CERT_CONTEXT* pPrevCertContext = null; + if (pCertContext != null) + { + pPrevCertContext = pCertContext.Disconnect(); + pCertContext.Dispose(); + } + pCertContext = Interop.Crypt32.CertFindCertificateInStore(hCertStore, Interop.Crypt32.CertEncodingType.All, Interop.Crypt32.CertFindFlags.None, dwFindType, pvFindPara, pPrevCertContext); return !pCertContext.IsInvalid; } -- 2.7.4