[release/7.0] Fix SslStream.IsMutuallyAuthenticated (#95733)

* 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 <[email protected]> Co-authored-by: Simon Rozsival <[email protected]> * fix CertificateValidationClientServer_EndToEnd_Ok test Co-authored-by: Radek Zikmund <[email protected]> Co-authored-by: Simon Rozsival <[email protected]> * fix IsMutuallyAuthenticated with NegotiateClientCertificateAsync (#88488) * Port rest of the changes from #79898 * Update src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs Co-authored-by: Stephen Toub <[email protected]> * Code review feedback --------- Co-authored-by: Tomas Weinfurt <[email protected]> Co-authored-by: Simon Rozsival <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
dotnet · Jan 10, 2024 · b7b55af · b7b55af
1 parent 06e9a55
commit b7b55af
Show file tree

Hide file tree

Showing 14 changed files with 279 additions and 37 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
@@ -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/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/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
@@ -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
@@ -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(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
@@ -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(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
@@ -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(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
@@ -8,6 +8,7 @@
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
 using System.Security.Principal;
+using static Interop.SspiCli;
 
 namespace System.Net
 {
@@ -89,6 +90,44 @@ internal static SslPolicyErrors VerifyCertificateProperties(
             return result;
         }
 
+        // Check that local certificate was used by schannel.
+        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
+            {
+                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
@@ -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");
 
@@ -56,7 +54,12 @@ internal X509Certificate? LocalClientCertificate
         {
             get
             {
-                return _selectedClientCertificate;
+                if (_selectedClientCertificate != null && CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!))
+                {
+                    return _selectedClientCertificate;
+                }
+
+                return null;
             }
         }
 
@@ -104,11 +107,6 @@ internal bool RemoteCertRequired
             }
         }
 
-        internal void SetRefreshCredentialNeeded()
-        {
-            _refreshCredentialNeeded = true;
-        }
-
         internal void CloseContext()
         {
             if (!_remoteCertificateExposed)
@@ -510,15 +508,14 @@ 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.
 
             bool sessionRestartAttempt; // If true and no cached creds we will use anonymous creds.
             bool cachedCred = false;                   // this is a return result from this method.
 
             X509Certificate2? selectedCert = SelectClientCertificate(out sessionRestartAttempt);
-
             try
             {
                 // Try to locate cached creds first.
@@ -576,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.
                 }
             }
@@ -687,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)
             {
@@ -749,7 +746,6 @@ internal ProtocolToken NextMessage(ReadOnlySpan<byte> incomingBuffer)
                 if (NetEventSource.Log.IsEnabled())
                     NetEventSource.Info(this, "NextMessage() returned SecurityStatusPal.CredentialsNeeded");
 
-                SetRefreshCredentialNeeded();
                 status = GenerateToken(incomingBuffer, ref nextmsg);
             }
 
@@ -788,6 +784,11 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> 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.
@@ -797,7 +798,7 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> inputBuffer, ref byte
                 do
                 {
                     thumbPrint = null;
-                    if (_refreshCredentialNeeded)
+                    if (refreshCredentialNeeded)
                     {
                         cachedCreds = _sslAuthenticationOptions.IsServer
                                         ? AcquireServerCredentials(ref thumbPrint)
@@ -826,15 +827,31 @@ private SecurityStatusPal GenerateToken(ReadOnlySpan<byte> 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.
@@ -974,7 +991,6 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
                 }
 
                 _remoteCertificate = certificate;
-
                 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/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
@@ -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
@@ -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
@@ -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/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;
         }