From 4f2285de7d4d59bf7b602909da497dc67a35f900 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Fri, 22 May 2020 12:36:14 +0200 Subject: [PATCH 01/15] Fix invalid test of SafeCredentialReference --- .../Windows/SspiCli/SecuritySafeHandles.cs | 47 +++++++++++-------- .../System/Net/Security/SSPIHandleCache.cs | 2 +- .../Net/Security/Unix/SafeFreeCredentials.cs | 42 +++++++++-------- .../src/System.Net.Security.csproj | 24 ++++++---- .../System/Net/Security/SslSessionsCache.cs | 16 +++---- 5 files changed, 76 insertions(+), 55 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 382d1f6a2979ed..c1cf2c534b798f 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -5,9 +5,9 @@ #nullable enable using System.Diagnostics; using System.Globalization; +using System.Runtime.ConstrainedExecution; using System.Runtime.InteropServices; using System.Security.Authentication.ExtendedProtection; -using Microsoft.Win32.SafeHandles; namespace System.Net.Security { @@ -323,14 +323,8 @@ public static unsafe int AcquireCredentialsHandle( // // This is a class holding a Credential handle reference, used for static handles cache // -#if DEBUG - internal sealed class SafeCredentialReference : DebugCriticalHandleMinusOneIsInvalid - { -#else - internal sealed class SafeCredentialReference : CriticalHandleMinusOneIsInvalid + internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDisposable { -#endif - // // Static cache will return the target handle if found the reference in the table. // @@ -338,30 +332,45 @@ internal sealed class SafeCredentialReference : CriticalHandleMinusOneIsInvalid internal static SafeCredentialReference? CreateReference(SafeFreeCredentials target) { - SafeCredentialReference result = new SafeCredentialReference(target); - if (result.IsInvalid) + if (target.IsInvalid || target.IsClosed) { return null; } + SafeCredentialReference result = new SafeCredentialReference(target); + return result; } - private SafeCredentialReference(SafeFreeCredentials target) : base() + private SafeCredentialReference(SafeFreeCredentials target) //: base() { // Bumps up the refcount on Target to signify that target handle is statically cached so // its dispose should be postponed bool ignore = false; target.DangerousAddRef(ref ignore); Target = target; - SetHandle(new IntPtr(0)); // make this handle valid } - protected override bool ReleaseHandle() + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + public void Dispose(bool disposing) { SafeFreeCredentials target = Target; target?.DangerousRelease(); Target = null!; - return true; + } + + ~SafeCredentialReference() + { + Dispose(false); + +#if DEBUG + Debug.Fail("Finalizer should never be called. The SafeCredentialReference should be always disposed manually."); + throw NotImplemented.ByDesign; +#endif } } @@ -762,11 +771,11 @@ internal static unsafe int AcceptSecurityContext( } else { - outSecBuffer.size = outUnmanagedBuffer[0].cbBuffer; - outSecBuffer.type = outUnmanagedBuffer[0].BufferType; - outSecBuffer.token = outUnmanagedBuffer[0].cbBuffer > 0 ? - new Span((byte*)outUnmanagedBuffer[0].pvBuffer, outUnmanagedBuffer[0].cbBuffer).ToArray() : - null; + outSecBuffer.size = outUnmanagedBuffer[0].cbBuffer; + outSecBuffer.type = outUnmanagedBuffer[0].BufferType; + outSecBuffer.token = outUnmanagedBuffer[0].cbBuffer > 0 ? + new Span((byte*)outUnmanagedBuffer[0].pvBuffer, outUnmanagedBuffer[0].cbBuffer).ToArray() : + null; } } } diff --git a/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs b/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs index 90e1dc85f95cd9..dcff49f222d543 100644 --- a/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs +++ b/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs @@ -34,7 +34,7 @@ internal static void CacheCredential(SafeFreeCredentials newHandle) newRef = Interlocked.Exchange(ref s_cacheSlots[index], newRef); } - newRef?.Dispose(); + newRef.Dispose(); } catch (Exception e) { 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 236e0e61484d0e..20bfc87cb55e20 100644 --- a/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs +++ b/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs @@ -4,8 +4,8 @@ #nullable enable using System.Diagnostics; +using System.Runtime.ConstrainedExecution; using System.Runtime.InteropServices; -using Microsoft.Win32.SafeHandles; namespace System.Net.Security { @@ -27,27 +27,21 @@ protected SafeFreeCredentials(IntPtr handle, bool ownsHandle) : base(handle, own // // This is a class holding a Credential handle reference, used for static handles cache // -#if DEBUG - internal sealed class SafeCredentialReference : DebugCriticalHandleMinusOneIsInvalid - { -#else - internal sealed class SafeCredentialReference : CriticalHandleMinusOneIsInvalid + internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDisposable { -#endif - // // Static cache will return the target handle if found the reference in the table. // - internal SafeFreeCredentials Target; + internal SafeFreeCredentials Target { get; private set; } internal static SafeCredentialReference? CreateReference(SafeFreeCredentials target) { - SafeCredentialReference result = new SafeCredentialReference(target); - if (result.IsInvalid) + if (target.IsInvalid || target.IsClosed) { return null; } + SafeCredentialReference result = new SafeCredentialReference(target); return result; } private SafeCredentialReference(SafeFreeCredentials target) : base() @@ -57,19 +51,29 @@ private SafeCredentialReference(SafeFreeCredentials target) : base() bool ignore = false; target.DangerousAddRef(ref ignore); Target = target; - SetHandle(new IntPtr(0)); // make this handle valid } - protected override bool ReleaseHandle() + public void Dispose() { - SafeFreeCredentials target = Target; - if (target != null) - { - target.DangerousRelease(); - } + Dispose(true); + GC.SuppressFinalize(this); + } + public void Dispose(bool disposing) + { + SafeFreeCredentials target = Target; + target?.DangerousRelease(); Target = null!; - return true; } + + ~SafeCredentialReference() + { + Dispose(false); + +#if DEBUG + Debug.Fail("Finalizer should never be called. The SafeCredentialReference should be always disposed manually."); +#endif + } + } } diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj index 73d613dfa729d5..868faddb5727a8 100644 --- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj +++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj @@ -12,37 +12,43 @@ + + - - - - - - - + + + + + + - + + + + + + Date: Fri, 22 May 2020 22:55:01 +0200 Subject: [PATCH 02/15] Remove commented code --- .../Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index c1cf2c534b798f..4d5042341fdacd 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -341,7 +341,7 @@ internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDispos return result; } - private SafeCredentialReference(SafeFreeCredentials target) //: base() + private SafeCredentialReference(SafeFreeCredentials target) { // Bumps up the refcount on Target to signify that target handle is statically cached so // its dispose should be postponed From 3f88e77929565ea5eaf515f94c36989fbd4cf9ad Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Fri, 22 May 2020 23:32:28 +0200 Subject: [PATCH 03/15] fix csproj --- .../src/System.Net.Security.csproj | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj index 868faddb5727a8..73d613dfa729d5 100644 --- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj +++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj @@ -12,43 +12,37 @@ - - + + + + + + + - - - - - - - + - - - - - Date: Sat, 23 May 2020 00:18:31 +0200 Subject: [PATCH 04/15] add forgot namespace --- .../Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 4d5042341fdacd..68074b5cbbdc66 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -8,6 +8,7 @@ using System.Runtime.ConstrainedExecution; using System.Runtime.InteropServices; using System.Security.Authentication.ExtendedProtection; +using Microsoft.Win32.SafeHandles; namespace System.Net.Security { From 85a8627c72fe11c6e98064df3a3fd20982aab718 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Tue, 2 Jun 2020 14:55:17 +0200 Subject: [PATCH 05/15] Extract SafeCredentialReference to new file --- .../Windows/SspiCli/SecuritySafeHandles.cs | 54 ------------------ .../Net/Security/SafeCredentialReference.cs | 57 +++++++++++++++++++ .../Net/Security/Unix/SafeFreeCredentials.cs | 52 ----------------- .../src/System.Net.Http.csproj | 2 + .../src/System.Net.HttpListener.csproj | 2 + .../src/System.Net.Mail.csproj | 2 + .../Unit/System.Net.Mail.Unit.Tests.csproj | 2 + .../src/System.Net.Security.csproj | 2 + 8 files changed, 67 insertions(+), 106 deletions(-) create mode 100644 src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 68074b5cbbdc66..02743f4416af9f 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -321,60 +321,6 @@ public static unsafe int AcquireCredentialsHandle( } } - // - // This is a class holding a Credential handle reference, used for static handles cache - // - internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDisposable - { - // - // Static cache will return the target handle if found the reference in the table. - // - internal SafeFreeCredentials Target; - - internal static SafeCredentialReference? CreateReference(SafeFreeCredentials target) - { - if (target.IsInvalid || target.IsClosed) - { - return null; - } - - SafeCredentialReference result = new SafeCredentialReference(target); - - return result; - } - private SafeCredentialReference(SafeFreeCredentials target) - { - // Bumps up the refcount on Target to signify that target handle is statically cached so - // its dispose should be postponed - bool ignore = false; - target.DangerousAddRef(ref ignore); - Target = target; - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - public void Dispose(bool disposing) - { - SafeFreeCredentials target = Target; - target?.DangerousRelease(); - Target = null!; - } - - ~SafeCredentialReference() - { - Dispose(false); - -#if DEBUG - Debug.Fail("Finalizer should never be called. The SafeCredentialReference should be always disposed manually."); - throw NotImplemented.ByDesign; -#endif - } - } - internal sealed class SafeFreeCredential_SECURITY : SafeFreeCredentials { public SafeFreeCredential_SECURITY() : base() { } diff --git a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs new file mode 100644 index 00000000000000..7f66fbc78e8205 --- /dev/null +++ b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs @@ -0,0 +1,57 @@ +#nullable enable + +using System.Runtime.ConstrainedExecution; + +namespace System.Net.Security +{ + + // + // This is a class holding a Credential handle reference, used for static handles cache + // + internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDisposable + { + // + // Static cache will return the target handle if found the reference in the table. + // + internal SafeFreeCredentials Target { get; private set; } + + internal static SafeCredentialReference? CreateReference(SafeFreeCredentials target) + { + if (target.IsInvalid || target.IsClosed) + { + return null; + } + + SafeCredentialReference result = new SafeCredentialReference(target); + + return result; + } + private SafeCredentialReference(SafeFreeCredentials target) + { + // Bumps up the refcount on Target to signify that target handle is statically cached so + // its dispose should be postponed + bool ignore = false; + target.DangerousAddRef(ref ignore); + Target = target; + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + private void Dispose(bool disposing) + { + SafeFreeCredentials? target = Target; + target?.DangerousRelease(); + Target = null!; + } + + ~SafeCredentialReference() + { + Dispose(false); + } + } + +} 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 20bfc87cb55e20..76d0dc99011eb0 100644 --- a/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs +++ b/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs @@ -24,56 +24,4 @@ protected SafeFreeCredentials(IntPtr handle, bool ownsHandle) : base(handle, own } } - // - // This is a class holding a Credential handle reference, used for static handles cache - // - internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDisposable - { - // - // Static cache will return the target handle if found the reference in the table. - // - internal SafeFreeCredentials Target { get; private set; } - - internal static SafeCredentialReference? CreateReference(SafeFreeCredentials target) - { - if (target.IsInvalid || target.IsClosed) - { - return null; - } - - SafeCredentialReference result = new SafeCredentialReference(target); - return result; - } - private SafeCredentialReference(SafeFreeCredentials target) : base() - { - // Bumps up the refcount on Target to signify that target handle is statically cached so - // its dispose should be postponed - bool ignore = false; - target.DangerousAddRef(ref ignore); - Target = target; - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - public void Dispose(bool disposing) - { - SafeFreeCredentials target = Target; - target?.DangerousRelease(); - Target = null!; - } - - ~SafeCredentialReference() - { - Dispose(false); - -#if DEBUG - Debug.Fail("Finalizer should never be called. The SafeCredentialReference should be always disposed manually."); -#endif - } - - } } diff --git a/src/libraries/System.Net.Http/src/System.Net.Http.csproj b/src/libraries/System.Net.Http/src/System.Net.Http.csproj index 2f7eb9dc48f43e..3e2cc30580de2f 100644 --- a/src/libraries/System.Net.Http/src/System.Net.Http.csproj +++ b/src/libraries/System.Net.Http/src/System.Net.Http.csproj @@ -286,6 +286,8 @@ Link="Common\System\Net\Http\aspnetcore\Quic\Interop\MsQuicNativeMethods.cs" /> + + + + + Date: Wed, 3 Jun 2020 12:31:39 +0200 Subject: [PATCH 06/15] Clean up --- .../src/System/Net/Security/SafeCredentialReference.cs | 4 +--- .../src/System/Net/Security/SslSessionsCache.cs | 8 ++++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs index 7f66fbc78e8205..374283088e8df5 100644 --- a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs +++ b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs @@ -22,9 +22,7 @@ internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDispos return null; } - SafeCredentialReference result = new SafeCredentialReference(target); - - return result; + return new SafeCredentialReference(target); } private SafeCredentialReference(SafeFreeCredentials target) { 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 75c0c526abff07..f7b425128847d3 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 @@ -131,9 +131,9 @@ public bool Equals(SslCredKey other) return null; } - if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"Found a cached Handle = {cached?.Target}"); + if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"Found a cached Handle = {cached.Target}"); - return cached?.Target; + return cached.Target; } // @@ -213,13 +213,13 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri } else if (NetEventSource.IsEnabled) { - if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() (locked retry) Found already cached Handle = {cached?.Target}"); + if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() (locked retry) Found already cached Handle = {cached.Target}"); } } } else if (NetEventSource.IsEnabled) { - if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() Ignoring incoming handle = {creds} since found already cached Handle = {cached?.Target}"); + if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() Ignoring incoming handle = {creds} since found already cached Handle = {cached.Target}"); } } } From 40dab5af8811afbc3bacaa5f77594a1727a3f67b Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Fri, 5 Jun 2020 18:08:59 +0200 Subject: [PATCH 07/15] Apply suggestions from code review Co-authored-by: Stephen Toub --- .../Common/src/System/Net/Security/SafeCredentialReference.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs index 374283088e8df5..52bd700a26135b 100644 --- a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs +++ b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs @@ -4,7 +4,6 @@ namespace System.Net.Security { - // // This is a class holding a Credential handle reference, used for static handles cache // @@ -24,6 +23,7 @@ internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDispos return new SafeCredentialReference(target); } + private SafeCredentialReference(SafeFreeCredentials target) { // Bumps up the refcount on Target to signify that target handle is statically cached so From 0ea9a2f25a4a7c8c5305f4c790e810b80ffa2be4 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Fri, 5 Jun 2020 18:27:33 +0200 Subject: [PATCH 08/15] Fix nullable check --- .../Common/src/System/Net/Security/SSPIHandleCache.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs b/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs index dcff49f222d543..bb26cd7d703b58 100644 --- a/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs +++ b/src/libraries/Common/src/System/Net/Security/SSPIHandleCache.cs @@ -14,7 +14,7 @@ namespace System.Net.Security internal static class SSPIHandleCache { private const int c_MaxCacheSize = 0x1F; // must a (power of 2) - 1 - private static readonly SafeCredentialReference[] s_cacheSlots = new SafeCredentialReference[c_MaxCacheSize + 1]; + private static readonly SafeCredentialReference?[] s_cacheSlots = new SafeCredentialReference[c_MaxCacheSize + 1]; private static int s_current = -1; internal static void CacheCredential(SafeFreeCredentials newHandle) @@ -31,10 +31,10 @@ internal static void CacheCredential(SafeFreeCredentials newHandle) unchecked { int index = Interlocked.Increment(ref s_current) & c_MaxCacheSize; - newRef = Interlocked.Exchange(ref s_cacheSlots[index], newRef); + newRef = Interlocked.Exchange(ref s_cacheSlots[index], newRef); } - newRef.Dispose(); + newRef?.Dispose(); } catch (Exception e) { From 6621fd176dd92848824e7951ed28c08b45dd1d54 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Tue, 26 May 2020 14:08:56 +0200 Subject: [PATCH 09/15] Fix null ref exception when accessing SecuritySafeHandler in SslSessionSache --- .../Net/Security/SafeCredentialReference.cs | 4 +- .../Net/Security/Unix/SafeFreeCredentials.cs | 1 + .../System/Net/Security/SslSessionsCache.cs | 42 +++++++++---------- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs index 52bd700a26135b..f8269eaf59d040 100644 --- a/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs +++ b/src/libraries/Common/src/System/Net/Security/SafeCredentialReference.cs @@ -12,7 +12,7 @@ internal sealed class SafeCredentialReference : CriticalFinalizerObject, IDispos // // Static cache will return the target handle if found the reference in the table. // - internal SafeFreeCredentials Target { get; private set; } + internal SafeFreeCredentials? Target { get; private set; } internal static SafeCredentialReference? CreateReference(SafeFreeCredentials target) { @@ -43,7 +43,7 @@ private void Dispose(bool disposing) { SafeFreeCredentials? target = Target; target?.DangerousRelease(); - Target = null!; + Target = null; } ~SafeCredentialReference() 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 76d0dc99011eb0..cb87e70db8be8e 100644 --- a/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs +++ b/src/libraries/Common/src/System/Net/Security/Unix/SafeFreeCredentials.cs @@ -6,6 +6,7 @@ using System.Diagnostics; using System.Runtime.ConstrainedExecution; using System.Runtime.InteropServices; +using Microsoft.Win32.SafeHandles; namespace System.Net.Security { 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 f7b425128847d3..bca00428ec8bc3 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 @@ -124,16 +124,22 @@ public bool Equals(SslCredKey other) var key = new SslCredKey(thumbPrint, (int)sslProtocols, isServer, encryptionPolicy); - SafeCredentialReference? cached; - if (!s_cachedCreds.TryGetValue(key, out cached) || cached.Target.IsClosed || cached.Target.IsInvalid) + //SafeCredentialReference? cached; + SafeFreeCredentials? credentials = GetCachedCredential(key); + if (credentials == null || credentials.IsClosed || credentials.IsInvalid) { if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"Not found or invalid, Current Cache Count = {s_cachedCreds.Count}"); return null; } - if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"Found a cached Handle = {cached.Target}"); + if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"Found a cached Handle = {credentials}"); - return cached.Target; + return credentials; + } + + private static SafeFreeCredentials? GetCachedCredential(SslCredKey key) + { + return s_cachedCreds.TryGetValue(key, out SafeCredentialReference? cached) ? cached.Target : null; } // @@ -156,15 +162,16 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri var key = new SslCredKey(thumbPrint, (int)sslProtocols, isServer, encryptionPolicy); - SafeCredentialReference? cached; + SafeFreeCredentials? credentials = GetCachedCredential(key); - if (!s_cachedCreds.TryGetValue(key, out cached) || cached.Target.IsClosed || cached.Target.IsInvalid) + if (credentials == null || credentials.IsClosed || credentials.IsInvalid) { lock (s_cachedCreds) { - if (!s_cachedCreds.TryGetValue(key, out cached) || cached.Target.IsClosed) + credentials = GetCachedCredential(key); + if (credentials == null || credentials.IsClosed) { - cached = SafeCredentialReference.CreateReference(creds); + var cached = SafeCredentialReference.CreateReference(creds); if (cached == null) { @@ -191,20 +198,13 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri for (int i = 0; i < toRemoveAttempt.Length; ++i) { - cached = toRemoveAttempt[i].Value; + var freeCreds = toRemoveAttempt[i].Value.Target; - if (cached != null) + if (freeCreds == null || freeCreds.IsClosed || freeCreds.IsInvalid) { - creds = cached.Target; - cached.Dispose(); - - if (!creds.IsClosed && !creds.IsInvalid && (cached = SafeCredentialReference.CreateReference(creds)) != null) - { - s_cachedCreds[toRemoveAttempt[i].Key] = cached; - } - else + if (s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out var removedCached)) { - s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out cached); + removedCached.Dispose(); } } } @@ -213,13 +213,13 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri } else if (NetEventSource.IsEnabled) { - if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() (locked retry) Found already cached Handle = {cached.Target}"); + if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() (locked retry) Found already cached Handle = {credentials}"); } } } else if (NetEventSource.IsEnabled) { - if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() Ignoring incoming handle = {creds} since found already cached Handle = {cached.Target}"); + if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"CacheCredential() Ignoring incoming handle = {creds} since found already cached Handle = {credentials}"); } } } From 615dc5e2070c5b2d5dda22488a417965e0399917 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Fri, 12 Jun 2020 17:05:19 +0200 Subject: [PATCH 10/15] Remove var --- .../src/System/Net/Security/SslSessionsCache.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 bca00428ec8bc3..08f4a1562c25d2 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 @@ -160,7 +160,7 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri return; } - var key = new SslCredKey(thumbPrint, (int)sslProtocols, isServer, encryptionPolicy); + SslCredKey key = new SslCredKey(thumbPrint, (int)sslProtocols, isServer, encryptionPolicy); SafeFreeCredentials? credentials = GetCachedCredential(key); @@ -171,7 +171,7 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri credentials = GetCachedCredential(key); if (credentials == null || credentials.IsClosed) { - var cached = SafeCredentialReference.CreateReference(creds); + SafeCredentialReference? cached = SafeCredentialReference.CreateReference(creds); if (cached == null) { @@ -198,11 +198,11 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri for (int i = 0; i < toRemoveAttempt.Length; ++i) { - var freeCreds = toRemoveAttempt[i].Value.Target; + SafeFreeCredentials? freeCreds = toRemoveAttempt[i].Value.Target; if (freeCreds == null || freeCreds.IsClosed || freeCreds.IsInvalid) { - if (s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out var removedCached)) + if (s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out SafeCredentialReference? removedCached)) { removedCached.Dispose(); } From 4c3e10fbb56bd540e1d9005057c0b8229731d4e3 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Tue, 16 Jun 2020 15:17:23 +0200 Subject: [PATCH 11/15] Revert to back cache crearing mechanizm --- .../System/Net/Security/SslSessionsCache.cs | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) 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 08f4a1562c25d2..766837b2a18c65 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 @@ -198,15 +198,26 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri for (int i = 0; i < toRemoveAttempt.Length; ++i) { - SafeFreeCredentials? freeCreds = toRemoveAttempt[i].Value.Target; + cached = toRemoveAttempt[i].Value; - if (freeCreds == null || freeCreds.IsClosed || freeCreds.IsInvalid) + if (cached.Target == null) { - if (s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out SafeCredentialReference? removedCached)) - { - removedCached.Dispose(); - } + s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out _); + continue; } + + creds = cached.Target; + cached.Dispose(); + + if (creds != null && !creds.IsClosed && !creds.IsInvalid && (cached = SafeCredentialReference.CreateReference(creds)) != null) + { + s_cachedCreds[toRemoveAttempt[i].Key] = cached; + } + else + { + s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out _); + } + } if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"Scavenged cache, New Cache Count = {s_cachedCreds.Count}"); } From 038440b858cb20e6a930147faba55285d9c06cfe Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Tue, 15 Sep 2020 13:27:50 +0200 Subject: [PATCH 12/15] Revert "Removed nightly to workaround failing Kestrel init. (#41934)" This reverts commit a84720cb68b940374e1955783ad55abb119e2fd3. --- eng/docker/libraries-sdk-aspnetcore.linux.Dockerfile | 2 +- eng/docker/libraries-sdk-aspnetcore.windows.Dockerfile | 2 +- eng/docker/libraries-sdk.linux.Dockerfile | 2 +- eng/docker/libraries-sdk.windows.Dockerfile | 2 +- .../System.Net.Http/tests/StressTests/HttpStress/Dockerfile | 2 +- .../tests/StressTests/HttpStress/windows.Dockerfile | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/eng/docker/libraries-sdk-aspnetcore.linux.Dockerfile b/eng/docker/libraries-sdk-aspnetcore.linux.Dockerfile index 064ca05a232f8c..b0bf080d8ff3da 100644 --- a/eng/docker/libraries-sdk-aspnetcore.linux.Dockerfile +++ b/eng/docker/libraries-sdk-aspnetcore.linux.Dockerfile @@ -1,6 +1,6 @@ # Builds and copies library artifacts into target dotnet sdk image ARG BUILD_BASE_IMAGE=mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-f39df28-20191023143754 -ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/sdk:5.0-buster-slim +ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-buster-slim FROM $BUILD_BASE_IMAGE as corefxbuild diff --git a/eng/docker/libraries-sdk-aspnetcore.windows.Dockerfile b/eng/docker/libraries-sdk-aspnetcore.windows.Dockerfile index b5355f5381e155..6e860466a1b4d1 100644 --- a/eng/docker/libraries-sdk-aspnetcore.windows.Dockerfile +++ b/eng/docker/libraries-sdk-aspnetcore.windows.Dockerfile @@ -1,6 +1,6 @@ # escape=` # Simple Dockerfile which copies library build artifacts into target dotnet sdk image -ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/sdk:5.0-nanoserver-1809 +ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-nanoserver-1809 FROM $SDK_BASE_IMAGE as target ARG TESTHOST_LOCATION=".\\artifacts\\bin\\testhost" diff --git a/eng/docker/libraries-sdk.linux.Dockerfile b/eng/docker/libraries-sdk.linux.Dockerfile index 6da12f9fae2bd2..4f1cb5185a35b1 100644 --- a/eng/docker/libraries-sdk.linux.Dockerfile +++ b/eng/docker/libraries-sdk.linux.Dockerfile @@ -1,6 +1,6 @@ # Builds and copies library artifacts into target dotnet sdk image ARG BUILD_BASE_IMAGE=mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-f39df28-20191023143754 -ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/sdk:5.0-buster-slim +ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-buster-slim FROM $BUILD_BASE_IMAGE as corefxbuild diff --git a/eng/docker/libraries-sdk.windows.Dockerfile b/eng/docker/libraries-sdk.windows.Dockerfile index 4c498e11e80c26..e88f52d7ce7e36 100644 --- a/eng/docker/libraries-sdk.windows.Dockerfile +++ b/eng/docker/libraries-sdk.windows.Dockerfile @@ -1,6 +1,6 @@ # escape=` # Simple Dockerfile which copies clr and library build artifacts into target dotnet sdk image -ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/sdk:5.0-nanoserver-1809 +ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-nanoserver-1809 FROM $SDK_BASE_IMAGE as target ARG TESTHOST_LOCATION=".\\artifacts\\bin\\testhost" diff --git a/src/libraries/System.Net.Http/tests/StressTests/HttpStress/Dockerfile b/src/libraries/System.Net.Http/tests/StressTests/HttpStress/Dockerfile index 5c16f4b433c0a4..00b1dd4e35e7e1 100644 --- a/src/libraries/System.Net.Http/tests/StressTests/HttpStress/Dockerfile +++ b/src/libraries/System.Net.Http/tests/StressTests/HttpStress/Dockerfile @@ -1,4 +1,4 @@ -ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/sdk:5.0-buster-slim +ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-buster-slim FROM $SDK_BASE_IMAGE RUN echo "DOTNET_SDK_VERSION="$DOTNET_SDK_VERSION diff --git a/src/libraries/System.Net.Http/tests/StressTests/HttpStress/windows.Dockerfile b/src/libraries/System.Net.Http/tests/StressTests/HttpStress/windows.Dockerfile index 5e8160eeb363e6..43f1da1deca245 100644 --- a/src/libraries/System.Net.Http/tests/StressTests/HttpStress/windows.Dockerfile +++ b/src/libraries/System.Net.Http/tests/StressTests/HttpStress/windows.Dockerfile @@ -1,5 +1,5 @@ # escape=` -ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/sdk:5.0-nanoserver-1809 +ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-nanoserver-1809 FROM $SDK_BASE_IMAGE # Use powershell as the default shell From 1fd8e4d01c14c7eab920ba9013bc225bd0ade57c Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Tue, 15 Dec 2020 11:36:11 +0100 Subject: [PATCH 13/15] Test invalid credentials --- .../Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs | 1 - .../src/System/Net/Security/SslSessionsCache.cs | 2 +- .../tests/FunctionalTests/SslStreamSystemDefaultsTest.cs | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs index 383bd9646e289b..5c14f4c69ffa6e 100644 --- a/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs +++ b/src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs @@ -4,7 +4,6 @@ #nullable enable using System.Diagnostics; using System.Globalization; -using System.Runtime.ConstrainedExecution; using System.Runtime.InteropServices; using System.Security.Authentication.ExtendedProtection; using Microsoft.Win32.SafeHandles; 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 7538080ec2a648..ee1f24a9daef5c 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 @@ -165,7 +165,7 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri lock (s_cachedCreds) { credentials = GetCachedCredential(key); - if (credentials == null || credentials.IsClosed) + if (credentials == null || credentials.IsClosed || credentials.IsInvalid) { SafeCredentialReference? cached = SafeCredentialReference.CreateReference(creds); diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSystemDefaultsTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSystemDefaultsTest.cs index d58bcc8c1f2de9..4136320067cd2f 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSystemDefaultsTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSystemDefaultsTest.cs @@ -61,7 +61,7 @@ public static IEnumerable OneOrBothUseDefaulData() if (PlatformDetection.SupportsTls13) { - //yield return new object[] { null, SslProtocols.Tls13 }; + yield return new object[] { null, SslProtocols.Tls13 }; yield return new object[] { SslProtocols.Tls13, null }; } From 46631be336c3bb46fc0cb70bf6fdc69955c78184 Mon Sep 17 00:00:00 2001 From: Jan Jahoda Date: Tue, 15 Dec 2020 14:27:23 +0100 Subject: [PATCH 14/15] fix windows test csproj --- .../tests/Unit/System.Net.Mail.Unit.Tests.csproj | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Mail/tests/Unit/System.Net.Mail.Unit.Tests.csproj b/src/libraries/System.Net.Mail/tests/Unit/System.Net.Mail.Unit.Tests.csproj index 929e636d3c9623..92c660864602e4 100644 --- a/src/libraries/System.Net.Mail/tests/Unit/System.Net.Mail.Unit.Tests.csproj +++ b/src/libraries/System.Net.Mail/tests/Unit/System.Net.Mail.Unit.Tests.csproj @@ -141,10 +141,6 @@ Link="Common\System\Net\NegotiationInfoClass.cs" /> - - + Date: Wed, 16 Dec 2020 11:49:12 +0100 Subject: [PATCH 15/15] Clean up cred cache shrink method --- .../System/Net/Security/SslSessionsCache.cs | 85 ++++++++++--------- 1 file changed, 46 insertions(+), 39 deletions(-) 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 ee1f24a9daef5c..3aa020bd4dae2c 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 @@ -178,45 +178,8 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri s_cachedCreds[key] = cached; if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Caching New Handle = {creds}, Current Cache Count = {s_cachedCreds.Count}"); - // - // A simplest way of preventing infinite cache grows. - // - // Security relief (DoS): - // A number of active creds is never greater than a number of _outstanding_ - // security sessions, i.e. SSL connections. - // So we will try to shrink cache to the number of active creds once in a while. - // - // We won't shrink cache in the case when NO new handles are coming to it. - // - if ((s_cachedCreds.Count % CheckExpiredModulo) == 0) - { - KeyValuePair[] toRemoveAttempt = s_cachedCreds.ToArray(); - - for (int i = 0; i < toRemoveAttempt.Length; ++i) - { - cached = toRemoveAttempt[i].Value; - - if (cached.Target == null) - { - s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out _); - continue; - } - - creds = cached.Target; - cached.Dispose(); - - if (creds != null && !creds.IsClosed && !creds.IsInvalid && (cached = SafeCredentialReference.CreateReference(creds)) != null) - { - s_cachedCreds[toRemoveAttempt[i].Key] = cached; - } - else - { - s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out _); - } - - } - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Scavenged cache, New Cache Count = {s_cachedCreds.Count}"); - } + ShrinkCredentialCache(); + } else { @@ -228,6 +191,50 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri { if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"CacheCredential() Ignoring incoming handle = {creds} since found already cached Handle = {credentials}"); } + + static void ShrinkCredentialCache() + { + + // + // A simplest way of preventing infinite cache grows. + // + // Security relief (DoS): + // A number of active creds is never greater than a number of _outstanding_ + // security sessions, i.e. SSL connections. + // So we will try to shrink cache to the number of active creds once in a while. + // + // We won't shrink cache in the case when NO new handles are coming to it. + // + if ((s_cachedCreds.Count % CheckExpiredModulo) == 0) + { + KeyValuePair[] toRemoveAttempt = s_cachedCreds.ToArray(); + + for (int i = 0; i < toRemoveAttempt.Length; ++i) + { + SafeCredentialReference? cahced = toRemoveAttempt[i].Value; + SafeFreeCredentials? creds = cahced.Target; + + if (creds == null) + { + s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out _); + continue; + } + + cahced.Dispose(); + cahced = SafeCredentialReference.CreateReference(creds); + if (cahced != null) + { + s_cachedCreds[toRemoveAttempt[i].Key] = cahced; + } + else + { + s_cachedCreds.TryRemove(toRemoveAttempt[i].Key, out _); + } + + } + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Scavenged cache, New Cache Count = {s_cachedCreds.Count}"); + } + } } } }