From 2d9b43281599f7a96b37d8576f6ff64e3d7ec8dc Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 8 Mar 2022 12:12:53 +0100 Subject: [PATCH 1/3] Add Expiry timestamp on SafeFreeCredentials handle --- .../Windows/SspiCli/SecuritySafeHandles.cs | 25 +++++++++++++------ .../Net/Security/Unix/SafeFreeCredentials.cs | 6 +++++ .../System/Net/Security/SslSessionsCache.cs | 9 ++++--- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index e0128ab0b994a8..957dc767a534b2 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Diagnostics; using System.Globalization; using System.Runtime.InteropServices; @@ -170,12 +171,14 @@ internal abstract class SafeFreeCredentials : DebugSafeHandle internal abstract class SafeFreeCredentials : SafeHandle { #endif + internal DateTime _expiry; internal Interop.SspiCli.CredHandle _handle; //should be always used as by ref in PInvokes parameters protected SafeFreeCredentials() : base(IntPtr.Zero, true) { _handle = default; + _expiry = DateTime.MaxValue; } public override bool IsInvalid @@ -183,6 +186,8 @@ public override bool IsInvalid get { return IsClosed || _handle.IsZero; } } + public DateTime Expiry => _expiry; + #if DEBUG public new IntPtr DangerousGetHandle() { @@ -197,7 +202,6 @@ public static unsafe int AcquireDefaultCredential( out SafeFreeCredentials outCredential) { int errorCode = -1; - long timeStamp; outCredential = new SafeFreeCredential_SECURITY(); @@ -210,7 +214,9 @@ public static unsafe int AcquireDefaultCredential( null, null, ref outCredential._handle, - out timeStamp); + out long timestamp); + + outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); if (NetEventSource.Log.IsEnabled()) NetEventSource.Verbose(null, $"{nameof(Interop.SspiCli.AcquireCredentialsHandleW)} returns 0x{errorCode:x}, handle = {outCredential}"); @@ -229,6 +235,7 @@ public static unsafe int AcquireCredentialsHandle( out SafeFreeCredentials outCredential) { outCredential = new SafeFreeCredential_SECURITY(); + int errorCode = Interop.SspiCli.AcquireCredentialsHandleW( null, package, @@ -238,7 +245,9 @@ public static unsafe int AcquireCredentialsHandle( null, null, ref outCredential._handle, - out _); + out long timestamp); + + outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); if (errorCode != 0) { @@ -267,7 +276,9 @@ public static unsafe int AcquireCredentialsHandle( null, null, ref outCredential._handle, - out _); + out long timestamp); + + outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); if (NetEventSource.Log.IsEnabled()) NetEventSource.Verbose(null, $"{nameof(Interop.SspiCli.AcquireCredentialsHandleW)} returns 0x{errorCode:x}, handle = {outCredential}"); @@ -285,8 +296,6 @@ public static unsafe int AcquireCredentialsHandle( Interop.SspiCli.SCH_CREDENTIALS* authdata, out SafeFreeCredentials outCredential) { - long timeStamp; - outCredential = new SafeFreeCredential_SECURITY(); int errorCode = Interop.SspiCli.AcquireCredentialsHandleW( @@ -298,7 +307,9 @@ public static unsafe int AcquireCredentialsHandle( null, null, ref outCredential._handle, - out timeStamp); + out long timestamp); + + outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); if (NetEventSource.Log.IsEnabled()) NetEventSource.Verbose(null, $"{nameof(Interop.SspiCli.AcquireCredentialsHandleW)} returns 0x{errorCode:x}, handle = {outCredential}"); diff --git a/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs b/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs index 7d26be95ab96b6..c2e0e0226eace7 100644 --- a/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs +++ b/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Diagnostics; using System.Runtime.ConstrainedExecution; using System.Runtime.InteropServices; @@ -18,8 +19,13 @@ internal abstract class SafeFreeCredentials : DebugSafeHandle internal abstract class SafeFreeCredentials : SafeHandle { #endif + internal DateTime _expiry; + + public DateTime Expiry => _expiry; + protected SafeFreeCredentials(IntPtr handle, bool ownsHandle) : base(handle, ownsHandle) { + _expiry = DateTime.MaxValue; } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs index f7b730c102c933..4e16dd64486595 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs @@ -134,9 +134,9 @@ public bool Equals(SslCredKey other) //SafeCredentialReference? cached; SafeFreeCredentials? credentials = GetCachedCredential(key); - if (credentials == null || credentials.IsClosed || credentials.IsInvalid) + if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < DateTime.UtcNow) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Not found or invalid, Current Cache Coun = {s_cachedCreds.Count}"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Not found or invalid, Current Cache Count = {s_cachedCreds.Count}"); return null; } @@ -169,12 +169,13 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri SafeFreeCredentials? credentials = GetCachedCredential(key); - if (credentials == null || credentials.IsClosed || credentials.IsInvalid) + DateTime now = DateTime.UtcNow; + if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < now) { lock (s_cachedCreds) { credentials = GetCachedCredential(key); - if (credentials == null || credentials.IsClosed || credentials.IsInvalid) + if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < now) { SafeCredentialReference? cached = SafeCredentialReference.CreateReference(creds); From 827d298d7408882d6cfabc042d1a075604d93f4a Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 8 Mar 2022 14:58:57 +0100 Subject: [PATCH 2/3] Recalculate expiration timestamp based on CertificateContext --- .../Windows/SspiCli/SecuritySafeHandles.cs | 23 ++++------ .../src/System/Net/Security/SecureChannel.cs | 42 ++++++++++++++++--- 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 957dc767a534b2..3c5bbe532161ea 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; using System.Diagnostics; using System.Globalization; using System.Runtime.InteropServices; @@ -171,8 +170,8 @@ internal abstract class SafeFreeCredentials : DebugSafeHandle internal abstract class SafeFreeCredentials : SafeHandle { #endif - internal DateTime _expiry; + internal DateTime _expiry; internal Interop.SspiCli.CredHandle _handle; //should be always used as by ref in PInvokes parameters protected SafeFreeCredentials() : base(IntPtr.Zero, true) @@ -202,6 +201,7 @@ public static unsafe int AcquireDefaultCredential( out SafeFreeCredentials outCredential) { int errorCode = -1; + long timeStamp; outCredential = new SafeFreeCredential_SECURITY(); @@ -214,9 +214,7 @@ public static unsafe int AcquireDefaultCredential( null, null, ref outCredential._handle, - out long timestamp); - - outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); + out timeStamp); if (NetEventSource.Log.IsEnabled()) NetEventSource.Verbose(null, $"{nameof(Interop.SspiCli.AcquireCredentialsHandleW)} returns 0x{errorCode:x}, handle = {outCredential}"); @@ -235,7 +233,6 @@ public static unsafe int AcquireCredentialsHandle( out SafeFreeCredentials outCredential) { outCredential = new SafeFreeCredential_SECURITY(); - int errorCode = Interop.SspiCli.AcquireCredentialsHandleW( null, package, @@ -245,9 +242,7 @@ public static unsafe int AcquireCredentialsHandle( null, null, ref outCredential._handle, - out long timestamp); - - outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); + out _); if (errorCode != 0) { @@ -276,9 +271,7 @@ public static unsafe int AcquireCredentialsHandle( null, null, ref outCredential._handle, - out long timestamp); - - outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); + out _); if (NetEventSource.Log.IsEnabled()) NetEventSource.Verbose(null, $"{nameof(Interop.SspiCli.AcquireCredentialsHandleW)} returns 0x{errorCode:x}, handle = {outCredential}"); @@ -296,6 +289,8 @@ public static unsafe int AcquireCredentialsHandle( Interop.SspiCli.SCH_CREDENTIALS* authdata, out SafeFreeCredentials outCredential) { + long timeStamp; + outCredential = new SafeFreeCredential_SECURITY(); int errorCode = Interop.SspiCli.AcquireCredentialsHandleW( @@ -307,9 +302,7 @@ public static unsafe int AcquireCredentialsHandle( null, null, ref outCredential._handle, - out long timestamp); - - outCredential._expiry = DateTime.FromFileTimeUtc(timestamp); + out timeStamp); if (NetEventSource.Log.IsEnabled()) NetEventSource.Verbose(null, $"{nameof(Interop.SspiCli.AcquireCredentialsHandleW)} returns 0x{errorCode:x}, handle = {outCredential}"); diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs index c9b67dda132ca8..03a30276582429 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs @@ -171,8 +171,8 @@ internal void Close() { if (!_remoteCertificateExposed) { - _remoteCertificate?.Dispose(); - _remoteCertificate = null; + _remoteCertificate?.Dispose(); + _remoteCertificate = null; } _securityContext?.Dispose(); @@ -607,8 +607,8 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!); } - _credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext, - _sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer); + + _credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions); thumbPrint = guessedThumbPrint; // Delay until here in case something above threw. } @@ -713,14 +713,44 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint) } else { - _credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext, _sslAuthenticationOptions.EnabledSslProtocols, - _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer); + _credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions); thumbPrint = guessedThumbPrint; } return cachedCred; } + private static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions) + { + SafeFreeCredentials cred = SslStreamPal.AcquireCredentialsHandle(sslAuthenticationOptions.CertificateContext, sslAuthenticationOptions.EnabledSslProtocols, + sslAuthenticationOptions.EncryptionPolicy, sslAuthenticationOptions.IsServer); + + if (sslAuthenticationOptions.CertificateContext != null) + { + // + // Since the SafeFreeCredentials can be cached and reused, it may happen on long running processes that some cert on + // the chain expires and all subsequent connections would send expired intermediate certificates. Find the earliest + // NotAfter timestamp on the chain and use it as expiration timestamp for the credentials. + // This provides an opportunity to recreate the credentials with an alternative (and still valid) + // certificate chain. + // + SslStreamCertificateContext certificateContext = sslAuthenticationOptions.CertificateContext; + DateTime expiry = certificateContext.Certificate.NotAfter; + + foreach (X509Certificate2 cert in certificateContext.IntermediateCertificates) + { + if (cert.NotAfter < expiry) + { + expiry = cert.NotAfter; + } + } + + cred._expiry = expiry.ToUniversalTime(); + } + + return cred; + } + // internal ProtocolToken NextMessage(ReadOnlySpan incomingBuffer) { From 54f919d2ed22fdcca78e3e05227ccf079f899e34 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 8 Mar 2022 16:28:32 +0100 Subject: [PATCH 3/3] Fix case when user provides CertificateContext --- .../src/System/Net/Security/SecureChannel.cs | 35 ++++++++++++++----- .../System/Net/Security/SslSessionsCache.cs | 6 ++-- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs index 03a30276582429..fbf29e0364cb02 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs @@ -607,9 +607,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint) _sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!); } - _credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions); - thumbPrint = guessedThumbPrint; // Delay until here in case something above threw. } } @@ -735,17 +733,38 @@ private static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOpt // certificate chain. // SslStreamCertificateContext certificateContext = sslAuthenticationOptions.CertificateContext; - DateTime expiry = certificateContext.Certificate.NotAfter; + cred._expiry = GetExpiryTimestamp(certificateContext); - foreach (X509Certificate2 cert in certificateContext.IntermediateCertificates) + if (cred._expiry < DateTime.UtcNow) { - if (cert.NotAfter < expiry) + // + // The CertificateContext from auth options is recreated just before creating the SafeFreeCredentials. However, in case when + // it was provided by the user code, it may still contain the (now expired) certificate chain. Such expiration timestamp would + // effectively disable caching as it would lead to creating new credentials for each connection. We attempt to recover by creating + // a temporary certificate context (which builds a new chain with hopefully more recent chain). + // + certificateContext = SslStreamCertificateContext.Create( + certificateContext.Certificate, + new X509Certificate2Collection(certificateContext.IntermediateCertificates), + trust: certificateContext.Trust); + + cred._expiry = GetExpiryTimestamp(certificateContext); + } + + static DateTime GetExpiryTimestamp(SslStreamCertificateContext certificateContext) + { + DateTime expiry = certificateContext.Certificate.NotAfter; + + foreach (X509Certificate2 cert in certificateContext.IntermediateCertificates) { - expiry = cert.NotAfter; + if (cert.NotAfter < expiry) + { + expiry = cert.NotAfter; + } } - } - cred._expiry = expiry.ToUniversalTime(); + return expiry.ToUniversalTime(); + } } return cred; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs index 4e16dd64486595..59009e72b4cfa1 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslSessionsCache.cs @@ -169,13 +169,13 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri SafeFreeCredentials? credentials = GetCachedCredential(key); - DateTime now = DateTime.UtcNow; - if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < now) + DateTime utcNow = DateTime.UtcNow; + if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < utcNow) { lock (s_cachedCreds) { credentials = GetCachedCredential(key); - if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < now) + if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < utcNow) { SafeCredentialReference? cached = SafeCredentialReference.CreateReference(creds);