From a6b3a86b4e5417ef661b5afe62715f09ff6f70af Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Mon, 12 Dec 2022 16:09:16 -0800 Subject: [PATCH 1/5] fix SslStream.IsMutuallyAuthenticated with cached credentials (#79128) * fix SslStream.IsMutuallyAuthenticated with cached credentials * nano * protocol * fix test * Apply suggestions from code review Co-authored-by: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Co-authored-by: Simon Rozsival * fix CertificateValidationClientServer_EndToEnd_Ok test Co-authored-by: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Co-authored-by: Simon Rozsival --- .../Interop/Windows/SspiCli/SSPIWrapper.cs | 3 ++ .../Net/CertificateValidationPal.Android.cs | 4 ++ .../Net/CertificateValidationPal.OSX.cs | 4 ++ .../Net/CertificateValidationPal.Unix.cs | 4 ++ .../Net/CertificateValidationPal.Windows.cs | 22 ++++++++ .../System/Net/Security/SslStream.Protocol.cs | 7 ++- .../CertificateValidationClientServer.cs | 16 +++--- .../SslStreamMutualAuthenticationTest.cs | 50 +++++++++++++++++-- 8 files changed, 99 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs index 81fdba8901bb3..b13e217b7374e 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs @@ -298,6 +298,9 @@ private static bool QueryCertContextAttribute(ISSPIInterface secModule, SafeDele public static bool QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(ISSPIInterface secModule, SafeDeleteContext securityContext, out SafeFreeCertContext? certContext) => QueryCertContextAttribute(secModule, securityContext, Interop.SspiCli.ContextAttribute.SECPKG_ATTR_REMOTE_CERT_CONTEXT, out certContext); + public static bool QueryContextAttributes_SECPKG_ATTR_LOCAL_CERT_CONTEXT(ISSPIInterface secModule, SafeDeleteContext securityContext, out SafeFreeCertContext? certContext) + => QueryCertContextAttribute(secModule, securityContext, Interop.SspiCli.ContextAttribute.SECPKG_ATTR_LOCAL_CERT_CONTEXT, out certContext); + public static bool QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CHAIN(ISSPIInterface secModule, SafeDeleteContext securityContext, out SafeFreeCertContext? certContext) => QueryCertContextAttribute(secModule, securityContext, Interop.SspiCli.ContextAttribute.SECPKG_ATTR_REMOTE_CERT_CHAIN, out certContext); diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs index 43716cd04367b..dc0865382cdf2 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs @@ -91,6 +91,10 @@ internal static SslPolicyErrors VerifyCertificateProperties( return cert; } + // This is only called when we selected local client certificate. + // Currently this is only when Java crypto asked for it. + internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true; + // // Used only by client SSL code, never returns null. // diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs index f9d3afe570af6..1f3fce1d7d863 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs @@ -102,6 +102,10 @@ internal static SslPolicyErrors VerifyCertificateProperties( return result; } + // This is only called when we selected local client certificate. + // Currently this is only when Apple crypto asked for it. + internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true; + // // Used only by client SSL code, never returns null. // diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs index 498a095410376..7074726a9177d 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs @@ -101,6 +101,10 @@ internal static SslPolicyErrors VerifyCertificateProperties( return result; } + // This is only called when we selected local client certificate. + // Currently this is only when OpenSSL needs it because peer asked. + internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true; + // // Used only by client SSL code, never returns null. // diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs index 1ead906ec3585..926cb646e7916 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs @@ -89,6 +89,28 @@ internal static SslPolicyErrors VerifyCertificateProperties( return result; } + // Check that local certificate was used by schannel. + internal static bool IsLocalCertificateUsed(SafeDeleteContext securityContext) + { + SafeFreeCertContext? localContext = null; + try + { + if (SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_LOCAL_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out localContext) && + localContext != null) + { + return !localContext.IsInvalid; + } + } + finally + { + localContext?.Dispose(); + } + + // Some older Windows do not support that. This is only called when client certificate was provided + // so assume it was for a reason. + return true; + } + // // Used only by client SSL code, never returns null. // 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 1d4dfa9ac6b0b..6362af4cec0cd 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 @@ -518,7 +518,6 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) bool cachedCred = false; // this is a return result from this method. X509Certificate2? selectedCert = SelectClientCertificate(out sessionRestartAttempt); - try { // Try to locate cached creds first. @@ -974,6 +973,12 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot } _remoteCertificate = certificate; + if (_selectedClientCertificate != null && !CertificateValidationPal.IsLocalCertificateUsed(_securityContext!)) + { + // We may slect client cert but it may not be used. + // This is primarily issue on Windows with credential caching + _selectedClientCertificate = null; + } if (_remoteCertificate == null) { diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs index 494bcf86fc226..a5bc4f1ddd7ff 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationClientServer.cs @@ -165,11 +165,16 @@ public async Task CertificateValidationClientServer_EndToEnd_Ok(bool useClientSe clientCerts.Add(_clientCertificate); } - Task clientAuthentication = sslClientStream.AuthenticateAsClientAsync( - serverName, - clientCerts, - SslProtocolSupport.DefaultSslProtocols, - false); + // Connect to GUID to prevent TLS resume + var options = new SslClientAuthenticationOptions() + { + TargetHost = Guid.NewGuid().ToString("N"), + ClientCertificates = clientCerts, + EnabledSslProtocols = SslProtocolSupport.DefaultSslProtocols, + CertificateChainPolicy = new X509ChainPolicy(), + }; + options.CertificateChainPolicy.VerificationFlags = X509VerificationFlags.IgnoreInvalidName; + Task clientAuthentication = sslClientStream.AuthenticateAsClientAsync(options, default); Task serverAuthentication = sslServerStream.AuthenticateAsServerAsync( _serverCertificate, @@ -258,7 +263,6 @@ private bool ClientSideRemoteServerCertificateValidation(object sender, X509Cert Assert.Equal(expectedSslPolicyErrors, sslPolicyErrors); Assert.Equal(_serverCertificate, certificate); - return true; } diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index fc34a29fdc7ba..e832b6500c4c1 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -8,6 +8,7 @@ using System.Security.Cryptography.X509Certificates; using Xunit; +using System.Runtime.InteropServices; namespace System.Net.Security.Tests { @@ -83,6 +84,47 @@ public async Task SslStream_RequireClientCert_IsMutuallyAuthenticated_ReturnsTru } } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] + [ClassData(typeof(SslProtocolSupport.SupportedSslProtocolsTestData))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/65563", TestPlatforms.Android)] + public async Task SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect( + SslProtocols protocol) + { + var clientOptions = new SslClientAuthenticationOptions + { + ClientCertificates = new X509CertificateCollection() { _clientCertificate }, + EnabledSslProtocols = protocol, + RemoteCertificateValidationCallback = delegate { return true; }, + TargetHost = Guid.NewGuid().ToString("N") + }; + + for (int i = 0; i < 5; i++) + { + (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(); + using (client) + using (server) + { + bool expectMutualAuthentication = (i % 2) == 0; + + var serverOptions = new SslServerAuthenticationOptions + { + ClientCertificateRequired = expectMutualAuthentication, + ServerCertificate = expectMutualAuthentication ? _serverCertificate : _selfSignedCertificate, + RemoteCertificateValidationCallback = delegate { return true; }, + EnabledSslProtocols = protocol + }; + + await TestConfiguration.WhenAllOrAnyFailedWithTimeout( + client.AuthenticateAsClientAsync(clientOptions), + server.AuthenticateAsServerAsync(serverOptions)); + + // mutual authentication should only be set if server required client cert + Assert.Equal(expectMutualAuthentication, server.IsMutuallyAuthenticated); + Assert.Equal(expectMutualAuthentication, client.IsMutuallyAuthenticated); + }; + } + } + [ClassData(typeof(SslProtocolSupport.SupportedSslProtocolsTestData))] [PlatformSpecific(TestPlatforms.Linux)] // https://github.com/dotnet/runtime/issues/65563 [Theory] @@ -128,7 +170,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -183,7 +225,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -221,7 +263,7 @@ public async Task SslStream_ResumedSessionsCallbackMaybeSet_IsMutuallyAuthentica if (expectMutualAuthentication) { - clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; + clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; } else { @@ -242,7 +284,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } From 453a97ac07e9ddde0ae61c8c2f9b960921c8eb80 Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Mon, 10 Jul 2023 10:38:39 -0700 Subject: [PATCH 2/5] fix IsMutuallyAuthenticated with NegotiateClientCertificateAsync (#88488) --- .../System/Net/Security/SslStream.Protocol.cs | 14 ++- .../SslStreamMutualAuthenticationTest.cs | 102 +++++++++++++++++- 2 files changed, 106 insertions(+), 10 deletions(-) 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 6362af4cec0cd..653a8092474c1 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 @@ -56,7 +56,12 @@ internal X509Certificate? LocalClientCertificate { get { - return _selectedClientCertificate; + if (_selectedClientCertificate != null && CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!)) + { + return _selectedClientCertificate; + } + + return null; } } @@ -973,13 +978,6 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot } _remoteCertificate = certificate; - if (_selectedClientCertificate != null && !CertificateValidationPal.IsLocalCertificateUsed(_securityContext!)) - { - // We may slect client cert but it may not be used. - // This is primarily issue on Windows with credential caching - _selectedClientCertificate = null; - } - if (_remoteCertificate == null) { if (NetEventSource.Log.IsEnabled() && RemoteCertRequired) NetEventSource.Error(this, $"Remote certificate required, but no remote certificate received"); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index e832b6500c4c1..4aca0662bc29e 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -33,6 +33,39 @@ public void Dispose() _clientCertificate.Dispose(); } + public enum ClientCertSource + { + ClientCertificate, + SelectionCallback, + CertificateContext + } + + public static TheoryData CertSourceData() + { + TheoryData data = new(); + + foreach (var source in Enum.GetValues()) + { + data.Add(source); + } + + return data; + } + + + public static TheoryData BoolAndCertSourceData() + { + TheoryData data = new(); + + foreach (var source in Enum.GetValues()) + { + data.Add(true, source); + data.Add(false, source); + } + + return data; + } + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] [InlineData(false, false)] [InlineData(false, true)] @@ -125,9 +158,74 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } } + [ConditionalTheory(typeof(TestConfiguration), nameof(TestConfiguration.SupportsRenegotiation))] + [MemberData(nameof(CertSourceData))] + [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] + public async Task SslStream_NegotiateClientCertificate_IsMutuallyAuthenticatedCorrect(ClientCertSource certSource) + { + SslStreamCertificateContext context = SslStreamCertificateContext.Create(_serverCertificate, null); + var clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N") + }; + + for (int round = 0; round < 3; round++) + { + (Stream stream1, Stream stream2) = TestHelper.GetConnectedStreams(); + using (var client = new SslStream(stream1, false, AllowAnyCertificate)) + using (var server = new SslStream(stream2, false, AllowAnyCertificate)) + { + + switch (certSource) + { + case ClientCertSource.ClientCertificate: + clientOptions.ClientCertificates = new X509CertificateCollection() { _clientCertificate }; + break; + case ClientCertSource.SelectionCallback: + clientOptions.LocalCertificateSelectionCallback = ClientCertSelectionCallback; + break; + case ClientCertSource.CertificateContext: + clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(_clientCertificate, new()); + break; + } + + Task t2 = client.AuthenticateAsClientAsync(clientOptions); + Task t1 = server.AuthenticateAsServerAsync(new SslServerAuthenticationOptions + { + ServerCertificateContext = context, + ClientCertificateRequired = false, + EnabledSslProtocols = SslProtocols.Tls12, + + }); + + await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); + + if (round >= 0 && server.RemoteCertificate != null) + { + // TLS resumed + Assert.True(client.IsMutuallyAuthenticated, "client.IsMutuallyAuthenticated"); + Assert.True(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated"); + continue; + } + + Assert.False(client.IsMutuallyAuthenticated, "client.IsMutuallyAuthenticated"); + Assert.False(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated"); + + var t = client.ReadAsync(new byte[1]); + await server.NegotiateClientCertificateAsync(); + Assert.NotNull(server.RemoteCertificate); + await server.WriteAsync(new byte[1]); + await t; + + Assert.NotNull(server.RemoteCertificate); + Assert.True(client.IsMutuallyAuthenticated, "client.IsMutuallyAuthenticated"); + Assert.True(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated"); + } + } + } + + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] [ClassData(typeof(SslProtocolSupport.SupportedSslProtocolsTestData))] - [PlatformSpecific(TestPlatforms.Linux)] // https://github.com/dotnet/runtime/issues/65563 - [Theory] public async Task SslStream_ResumedSessionsClientCollection_IsMutuallyAuthenticatedCorrect( SslProtocols protocol) { From 8db62eff0f299bb0481d5683390d5efb5e582b5e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 7 Dec 2023 13:42:05 +0100 Subject: [PATCH 3/5] Port rest of the changes from #79898 --- .../Interop/Windows/SspiCli/Interop.SSPI.cs | 18 +++++++- .../Windows/SspiCli/SecuritySafeHandles.cs | 6 +++ .../Net/CertificateValidationPal.Android.cs | 4 +- .../Net/CertificateValidationPal.OSX.cs | 2 +- .../Net/CertificateValidationPal.Unix.cs | 2 +- .../Net/CertificateValidationPal.Windows.cs | 18 +++++++- .../System/Net/Security/SslStream.Protocol.cs | 45 ++++++++++++------- .../Net/Security/SslStreamPal.Android.cs | 2 +- .../System/Net/Security/SslStreamPal.OSX.cs | 2 +- .../System/Net/Security/SslStreamPal.Unix.cs | 2 +- .../Net/Security/SslStreamPal.Windows.cs | 12 ++++- .../SslStreamMutualAuthenticationTest.cs | 4 -- 12 files changed, 87 insertions(+), 30 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/Interop.SSPI.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/Interop.SSPI.cs index c8e7dd3649024..2e668f6c84638 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/Interop.SSPI.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/Interop.SSPI.cs @@ -67,6 +67,7 @@ internal enum ContextAttribute SECPKG_ATTR_ISSUER_LIST_EX = 0x59, // returns SecPkgContext_IssuerListInfoEx SECPKG_ATTR_CLIENT_CERT_POLICY = 0x60, // sets SecPkgCred_ClientCertCtlPolicy SECPKG_ATTR_CONNECTION_INFO = 0x5A, // returns SecPkgContext_ConnectionInfo + SECPKG_ATTR_SESSION_INFO = 0x5D, // sets SecPkgContext_SessionInfo SECPKG_ATTR_CIPHER_INFO = 0x64, // returns SecPkgContext_CipherInfo SECPKG_ATTR_REMOTE_CERT_CHAIN = 0x67, // returns PCCERT_CONTEXT SECPKG_ATTR_UI_INFO = 0x68, // sets SEcPkgContext_UiInfo @@ -249,7 +250,7 @@ public enum Flags SCH_CRED_IGNORE_REVOCATION_OFFLINE = 0x1000, SCH_CRED_CACHE_ONLY_URL_RETRIEVAL_ON_CREATE = 0x2000, SCH_SEND_ROOT_CERT = 0x40000, - SCH_SEND_AUX_RECORD = 0x00200000, + SCH_SEND_AUX_RECORD = 0x00200000, SCH_USE_STRONG_CRYPTO = 0x00400000, SCH_USE_PRESHAREDKEY_ONLY = 0x800000, SCH_ALLOW_NULL_ENCRYPTION = 0x02000000, @@ -334,6 +335,21 @@ internal unsafe struct SecPkgCred_ClientCertPolicy public char* pwszSslCtlIdentifier; } + [StructLayout(LayoutKind.Sequential)] + internal unsafe struct SecPkgContext_SessionInfo + { + public uint dwFlags; + public uint cbSessionId; + public fixed byte rgbSessionId[32]; + + [Flags] + public enum Flags + { + Zero = 0, + SSL_SESSION_RECONNECT = 0x01, + }; + } + [LibraryImport(Interop.Libraries.SspiCli, SetLastError = true)] internal static partial int EncryptMessage( ref CredHandle contextHandle, diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 12603f7df6ae9..880189a741625 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Globalization; using System.Runtime.InteropServices; +using System.Security.Cryptography.X509Certificates; using System.Security.Authentication.ExtendedProtection; using Microsoft.Win32.SafeHandles; @@ -310,10 +311,15 @@ public static unsafe int AcquireCredentialsHandle( internal sealed class SafeFreeCredential_SECURITY : SafeFreeCredentials { +#pragma warning disable 0649 + // This is used only by SslStream but it is included elsewhere + public X509Certificate? LocalCertificate; +#pragma warning restore 0649 public SafeFreeCredential_SECURITY() : base() { } protected override bool ReleaseHandle() { + LocalCertificate?.Dispose(); return Interop.SspiCli.FreeCredentialsHandle(ref _handle) == 0; } } diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs index dc0865382cdf2..96962f9240be9 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs @@ -18,7 +18,7 @@ internal static SslPolicyErrors VerifyCertificateProperties( string? hostName) { if (remoteCertificate == null) - return SslPolicyErrors.RemoteCertificateNotAvailable; + return SslPolicyErrors.RemoteCertificateNotAvailable; SslPolicyErrors errors = chain.Build(remoteCertificate) ? SslPolicyErrors.None @@ -93,7 +93,7 @@ internal static SslPolicyErrors VerifyCertificateProperties( // This is only called when we selected local client certificate. // Currently this is only when Java crypto asked for it. - internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true; + internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? _2) => true; // // Used only by client SSL code, never returns null. diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs index 1f3fce1d7d863..3bd0c7142c3fc 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs @@ -104,7 +104,7 @@ internal static SslPolicyErrors VerifyCertificateProperties( // This is only called when we selected local client certificate. // Currently this is only when Apple crypto asked for it. - internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true; + internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? _2) => true; // // Used only by client SSL code, never returns null. diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs index 7074726a9177d..90b9275e9af36 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs @@ -103,7 +103,7 @@ internal static SslPolicyErrors VerifyCertificateProperties( // This is only called when we selected local client certificate. // Currently this is only when OpenSSL needs it because peer asked. - internal static bool IsLocalCertificateUsed(SafeDeleteContext? _) => true; + internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? _2) => true; // // Used only by client SSL code, never returns null. diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs index 926cb646e7916..3d622f6c2dfc7 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs @@ -8,6 +8,7 @@ using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Security.Principal; +using static Interop.SspiCli; namespace System.Net { @@ -90,8 +91,23 @@ internal static SslPolicyErrors VerifyCertificateProperties( } // Check that local certificate was used by schannel. - internal static bool IsLocalCertificateUsed(SafeDeleteContext securityContext) + internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _credentialsHandle, SafeDeleteContext securityContext) { + SecPkgContext_SessionInfo info = default; + // fails on Server 2008 and older. We will fall-back to probing LOCAL_CERT_CONTEXT in that case. + if (SSPIWrapper.QueryBlittableContextAttributes( + GlobalSSPI.SSPISecureChannel, + securityContext, + Interop.SspiCli.ContextAttribute.SECPKG_ATTR_SESSION_INFO, + ref info) && + ((SecPkgContext_SessionInfo.Flags)info.dwFlags).HasFlag(SecPkgContext_SessionInfo.Flags.SSL_SESSION_RECONNECT)) + { + // This is TLS Resumed session. Windows can fail to query the local cert bellow. + // Instead, we will determine the usage form used credentials. + SafeFreeCredential_SECURITY creds = (SafeFreeCredential_SECURITY)_credentialsHandle!; + return creds.LocalCertificate != null; + } + SafeFreeCertContext? localContext = null; try { 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 653a8092474c1..45f0bbe408f24 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 @@ -30,8 +30,6 @@ public partial class SslStream private int _trailerSize = 16; private int _maxDataSize = 16354; - private bool _refreshCredentialNeeded = true; - private static readonly Oid s_serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1", "1.3.6.1.5.5.7.3.1"); private static readonly Oid s_clientAuthOid = new Oid("1.3.6.1.5.5.7.3.2", "1.3.6.1.5.5.7.3.2"); @@ -109,11 +107,6 @@ internal bool RemoteCertRequired } } - internal void SetRefreshCredentialNeeded() - { - _refreshCredentialNeeded = true; - } - internal void CloseContext() { if (!_remoteCertificateExposed) @@ -515,7 +508,7 @@ This will not restart a session but helps minimizing the number of handles we cr --*/ - private bool AcquireClientCredentials(ref byte[]? thumbPrint) + private bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredentialsRequested = false) { // Acquire possible Client Certificate information and set it on the handle. @@ -580,7 +573,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!); } - _credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions); + _credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions, newCredentialsRequested); thumbPrint = guessedThumbPrint; // Delay until here in case something above threw. } } @@ -691,9 +684,9 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint) return cachedCred; } - private static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions) + private static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions, bool newCredentialsRequested = false) { - SafeFreeCredentials? cred = SslStreamPal.AcquireCredentialsHandle(sslAuthenticationOptions); + SafeFreeCredentials? cred = SslStreamPal.AcquireCredentialsHandle(sslAuthenticationOptions, newCredentialsRequested); if (sslAuthenticationOptions.CertificateContext != null && cred != null) { @@ -753,7 +746,6 @@ internal ProtocolToken NextMessage(ReadOnlySpan incomingBuffer) if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "NextMessage() returned SecurityStatusPal.CredentialsNeeded"); - SetRefreshCredentialNeeded(); status = GenerateToken(incomingBuffer, ref nextmsg); } @@ -792,6 +784,11 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan inputBuffer, ref byte bool sendTrustList = false; byte[]? thumbPrint = null; + // We need to try get credentials at the beginning. + // _credentialsHandle may be always null on some platforms but + // _securityContext will be allocated on first call. + bool refreshCredentialNeeded = _securityContext == null; + // // Looping through ASC or ISC with potentially cached credential that could have been // already disposed from a different thread before ISC or ASC dir increment a cred ref count. @@ -801,7 +798,7 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan inputBuffer, ref byte do { thumbPrint = null; - if (_refreshCredentialNeeded) + if (refreshCredentialNeeded) { cachedCreds = _sslAuthenticationOptions.IsServer ? AcquireServerCredentials(ref thumbPrint) @@ -830,15 +827,31 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan inputBuffer, ref byte _sslAuthenticationOptions, SelectClientCertificate ); + + if (status.ErrorCode == SecurityStatusPalErrorCode.CredentialsNeeded) + { + refreshCredentialNeeded = true; + cachedCreds = AcquireClientCredentials(ref thumbPrint, newCredentialsRequested: true); + + if (NetEventSource.Log.IsEnabled()) + NetEventSource.Info(this, "InitializeSecurityContext() returned 'CredentialsNeeded'."); + + status = SslStreamPal.InitializeSecurityContext( + ref _credentialsHandle!, + ref _securityContext, + _sslAuthenticationOptions.TargetHost, + inputBuffer, + ref result, + _sslAuthenticationOptions, + SelectClientCertificate); + } } } while (cachedCreds && _credentialsHandle == null); } finally { - if (_refreshCredentialNeeded) + if (refreshCredentialNeeded) { - _refreshCredentialNeeded = false; - // // Assuming the ISC or ASC has referenced the credential, // we want to call dispose so to decrement the effective ref count. diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs index f01dd68e294b2..184cbd2a81773 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs @@ -55,7 +55,7 @@ public static SecurityStatusPal Renegotiate( throw new PlatformNotSupportedException(); } - public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions) + public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions _1, bool _2) { return null; } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs index 255b30d7f2c2f..d8fc15ca7a546 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs @@ -62,7 +62,7 @@ public static SecurityStatusPal Renegotiate( throw new PlatformNotSupportedException(); } - public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions) + public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions _1, bool _2) { return null; } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index 1e1a0df55889e..e4188015d4167 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs @@ -46,7 +46,7 @@ public static SecurityStatusPal InitializeSecurityContext( return HandshakeInternal(ref context, inputBuffer, ref outputBuffer, sslAuthenticationOptions, clientCertificateSelectionCallback); } - public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions) + public static SafeFreeCredentials? AcquireCredentialsHandle(SslAuthenticationOptions _1, bool _2) { return null; } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs index 1321cc0754ed0..2c69293145800 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs @@ -136,7 +136,7 @@ public static SecurityStatusPal Renegotiate( return status; } - public static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions) + public static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions, bool newCredentialsRequested) { try { @@ -156,6 +156,16 @@ public static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOpti AttachCertificateStore(cred, certificateContext.Trust._store!); } + // Windows can fail to get local credentials in case of TLS Resume. + // We will store associated certificate in credentials and use it in case + // of TLS resume. It will be disposed when the credentials are. + if (newCredentialsRequested && sslAuthenticationOptions.CertificateContext != null) + { + SafeFreeCredential_SECURITY handle = (SafeFreeCredential_SECURITY)cred; + // We need to create copy to avoid Disposal issue. + handle.LocalCertificate = new X509Certificate2(sslAuthenticationOptions.CertificateContext.Certificate); + } + return cred; } catch (Win32Exception e) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index 4aca0662bc29e..90c0a6cb04901 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -37,7 +37,6 @@ public enum ClientCertSource { ClientCertificate, SelectionCallback, - CertificateContext } public static TheoryData CertSourceData() @@ -184,9 +183,6 @@ public async Task SslStream_NegotiateClientCertificate_IsMutuallyAuthenticatedCo case ClientCertSource.SelectionCallback: clientOptions.LocalCertificateSelectionCallback = ClientCertSelectionCallback; break; - case ClientCertSource.CertificateContext: - clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(_clientCertificate, new()); - break; } Task t2 = client.AuthenticateAsClientAsync(clientOptions); From 6f9b43089098ec1b159e06f474a2bf42d1d9a567 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Tue, 2 Jan 2024 17:35:11 +0100 Subject: [PATCH 4/5] Update src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs Co-authored-by: Stephen Toub --- .../src/System/Net/CertificateValidationPal.Windows.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs index 3d622f6c2dfc7..d56fcc7f5ce36 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs @@ -94,6 +94,7 @@ internal static SslPolicyErrors VerifyCertificateProperties( internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _credentialsHandle, SafeDeleteContext securityContext) { SecPkgContext_SessionInfo info = default; + // fails on Server 2008 and older. We will fall-back to probing LOCAL_CERT_CONTEXT in that case. if (SSPIWrapper.QueryBlittableContextAttributes( GlobalSSPI.SSPISecureChannel, From 5f6a75706c176778f62c4afcfd31f759a9df4afc Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 2 Jan 2024 17:43:13 +0100 Subject: [PATCH 5/5] Code review feedback --- .../src/System/Net/CertificateValidationPal.Windows.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs index d56fcc7f5ce36..ecdfef0242939 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs @@ -91,7 +91,7 @@ internal static SslPolicyErrors VerifyCertificateProperties( } // Check that local certificate was used by schannel. - internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _credentialsHandle, SafeDeleteContext securityContext) + internal static bool IsLocalCertificateUsed(SafeFreeCredentials? credentialsHandle, SafeDeleteContext securityContext) { SecPkgContext_SessionInfo info = default; @@ -105,7 +105,7 @@ internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _credentialsHan { // This is TLS Resumed session. Windows can fail to query the local cert bellow. // Instead, we will determine the usage form used credentials. - SafeFreeCredential_SECURITY creds = (SafeFreeCredential_SECURITY)_credentialsHandle!; + SafeFreeCredential_SECURITY creds = (SafeFreeCredential_SECURITY)credentialsHandle!; return creds.LocalCertificate != null; }