From bb674f396c310eeef51ba2de734b4ed24c61685a Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 4 Mar 2025 14:29:00 +0100 Subject: [PATCH 1/7] Link peer's X509 stack handle to parent SSL safe handle --- .../System.Security.Cryptography.Native/Interop.Ssl.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs index 9f3d05e43e96af..49a13bdd762c48 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs @@ -123,7 +123,14 @@ internal static unsafe ReadOnlySpan SslGetAlpnSelected(SafeSslHandle ssl) internal static partial IntPtr SslGetCertificate(IntPtr ssl); [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerCertChain")] - internal static partial SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl); + internal static partial SafeSharedX509StackHandle SslGetPeerCertChain_private(SafeSslHandle ssl); + + internal static SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl) + { + return SafeInteriorHandle.OpenInteriorHandle( + SslGetPeerCertChain_private, + ssl); + } [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerFinished")] internal static partial int SslGetPeerFinished(SafeSslHandle ssl, IntPtr buf, int count); From 95fbdec5ff2a5e14fe7c95129f4a887f9f890b38 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Wed, 5 Mar 2025 10:11:21 +0100 Subject: [PATCH 2/7] Update src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs Co-authored-by: Kevin Jones --- .../Unix/System.Security.Cryptography.Native/Interop.Ssl.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs index 49a13bdd762c48..b9d755d988203f 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs @@ -123,7 +123,7 @@ internal static unsafe ReadOnlySpan SslGetAlpnSelected(SafeSslHandle ssl) internal static partial IntPtr SslGetCertificate(IntPtr ssl); [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerCertChain")] - internal static partial SafeSharedX509StackHandle SslGetPeerCertChain_private(SafeSslHandle ssl); + private static partial SafeSharedX509StackHandle SslGetPeerCertChain_private(SafeSslHandle ssl); internal static SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl) { From abf96c8b800bcf21c233db7d014ad332c72b64df Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 14 Mar 2025 16:13:13 +0100 Subject: [PATCH 3/7] Add test for dispose parallel with handshake. --- .../FunctionalTests/SslStreamDisposeTest.cs | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs index de7aa502933b02..0eacfb274cb62f 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs @@ -102,5 +102,57 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( await Assert.ThrowsAnyAsync(() => client.ReadAsync(readBuffer, cts.Token).AsTask()); } } + + [Fact] + [OuterLoop("Computationally expensive")] + public async Task Dispose_ParallelWithHandshake_ThrowsODE() + { + using CancellationTokenSource cts = new CancellationTokenSource(); + cts.CancelAfter(TestConfiguration.PassingTestTimeout); + + await Parallel.ForEachAsync(System.Linq.Enumerable.Range(0, 10000), cts.Token, async (i, token) => + { + (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(); + using (client) + using (server) + using (X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate()) + using (X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate()) + { + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions() + { + TargetHost = Guid.NewGuid().ToString("N"), + RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true, + }; + + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions() + { + ServerCertificate = serverCertificate, + }; + + var clientTask = Task.Run(() => client.AuthenticateAsClientAsync(clientOptions, cts.Token)); + var serverTask = Task.Run(() => server.AuthenticateAsServerAsync(serverOptions, cts.Token)); + + // Dispose the instances while the handshake is in progress. + client.Dispose(); + server.Dispose(); + + await ValidateExceptionAsync(clientTask); + await ValidateExceptionAsync(serverTask); + } + }); + + static async Task ValidateExceptionAsync(Task task) + { + try + { + await task; + } + // either we disposed the stream, or the other side does and we get unexpected EOF + catch (Exception ex) when (ex is ObjectDisposedException or IOException) + { + return; + } + } + } } } From 4fe50173b32f61a3947f9b46769e097d2fe84638 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 9 May 2025 09:39:07 +0200 Subject: [PATCH 4/7] Revert "Add test for dispose parallel with handshake." This reverts commit abf96c8b800bcf21c233db7d014ad332c72b64df. --- .../FunctionalTests/SslStreamDisposeTest.cs | 52 ------------------- 1 file changed, 52 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs index 0eacfb274cb62f..de7aa502933b02 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs @@ -102,57 +102,5 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( await Assert.ThrowsAnyAsync(() => client.ReadAsync(readBuffer, cts.Token).AsTask()); } } - - [Fact] - [OuterLoop("Computationally expensive")] - public async Task Dispose_ParallelWithHandshake_ThrowsODE() - { - using CancellationTokenSource cts = new CancellationTokenSource(); - cts.CancelAfter(TestConfiguration.PassingTestTimeout); - - await Parallel.ForEachAsync(System.Linq.Enumerable.Range(0, 10000), cts.Token, async (i, token) => - { - (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(); - using (client) - using (server) - using (X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate()) - using (X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate()) - { - SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions() - { - TargetHost = Guid.NewGuid().ToString("N"), - RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true, - }; - - SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions() - { - ServerCertificate = serverCertificate, - }; - - var clientTask = Task.Run(() => client.AuthenticateAsClientAsync(clientOptions, cts.Token)); - var serverTask = Task.Run(() => server.AuthenticateAsServerAsync(serverOptions, cts.Token)); - - // Dispose the instances while the handshake is in progress. - client.Dispose(); - server.Dispose(); - - await ValidateExceptionAsync(clientTask); - await ValidateExceptionAsync(serverTask); - } - }); - - static async Task ValidateExceptionAsync(Task task) - { - try - { - await task; - } - // either we disposed the stream, or the other side does and we get unexpected EOF - catch (Exception ex) when (ex is ObjectDisposedException or IOException) - { - return; - } - } - } } } From fc0e11f7d2565ea79541b8c3414aec8b0002d07a Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Mon, 5 May 2025 09:34:18 +0200 Subject: [PATCH 5/7] Defer RemoteCertificate assignment after X509 Chain build (#114781) * Defer RemoteCertificate assignment after X509 Chain build * Add comment --- .../src/System/Net/Security/SslStream.Protocol.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 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 db0a4a6c84bf19..bb6407ea4e0e43 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 @@ -1057,8 +1057,9 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot return true; } - _remoteCertificate = certificate; - if (_remoteCertificate == null) + // don't assign to _remoteCertificate yet, this prevents weird exceptions if SslStream is disposed in parallel with X509Chain building + + if (certificate == null) { if (NetEventSource.Log.IsEnabled() && RemoteCertRequired) NetEventSource.Error(this, $"Remote certificate required, but no remote certificate received"); sslPolicyErrors |= SslPolicyErrors.RemoteCertificateNotAvailable; @@ -1100,15 +1101,17 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot sslPolicyErrors |= CertificateValidationPal.VerifyCertificateProperties( _securityContext!, chain, - _remoteCertificate, + certificate, _sslAuthenticationOptions.CheckCertName, _sslAuthenticationOptions.IsServer, TargetHostNameHelper.NormalizeHostName(_sslAuthenticationOptions.TargetHost)); } + _remoteCertificate = certificate; + if (remoteCertValidationCallback != null) { - success = remoteCertValidationCallback(this, _remoteCertificate, chain, sslPolicyErrors); + success = remoteCertValidationCallback(this, certificate, chain, sslPolicyErrors); } else { From ba937e91d2a83386437b5f5296486e6e66c84b64 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Tue, 25 Mar 2025 08:44:43 +0100 Subject: [PATCH 6/7] Fix SslStreamDisposeTest failures during parallel handshake (#113834) * Fix SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE test failures * Fix build --- .../FunctionalTests/SslStreamDisposeTest.cs | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs index de7aa502933b02..d3bfcc44488c72 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs @@ -4,6 +4,7 @@ using System.IO; using System.Security.Cryptography.X509Certificates; using System.Threading; +using System.Security.Authentication; using System.Threading.Tasks; using Xunit; @@ -102,5 +103,61 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( await Assert.ThrowsAnyAsync(() => client.ReadAsync(readBuffer, cts.Token).AsTask()); } } + + [Fact] + [OuterLoop("Computationally expensive")] + public async Task Dispose_ParallelWithHandshake_ThrowsODE() + { + using CancellationTokenSource cts = new CancellationTokenSource(); + cts.CancelAfter(TestConfiguration.PassingTestTimeout); + + await Parallel.ForEachAsync(System.Linq.Enumerable.Range(0, 10000), cts.Token, async (i, token) => + { + // use real Tcp streams to avoid specific behavior of ConnectedStreams when concurrently disposed + (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedTcpStreams(); + + using SslStream client = new SslStream(clientStream); + using SslStream server = new SslStream(serverStream); + using X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate(); + using X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate(); + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions() + { + TargetHost = Guid.NewGuid().ToString("N"), + RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true, + }; + + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions() + { + ServerCertificate = serverCertificate, + }; + + var clientTask = Task.Run(() => client.AuthenticateAsClientAsync(clientOptions, cts.Token)); + var serverTask = Task.Run(() => server.AuthenticateAsServerAsync(serverOptions, cts.Token)); + + // Dispose the instances while the handshake is in progress. + client.Dispose(); + server.Dispose(); + + await ValidateExceptionAsync(clientTask); + await ValidateExceptionAsync(serverTask); + }); + + static async Task ValidateExceptionAsync(Task task) + { + try + { + await task; + } + catch (Exception ex) when (ex + is ObjectDisposedException // disposed locally + or IOException // disposed remotely (received unexpected EOF) + or AuthenticationException) // disposed wrapped in AuthenticationException or error from platform library + { + // expected + return; + } + } + } } } From ee34d46f6727f782b913691e0129c3253fb36077 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Fri, 4 Apr 2025 09:16:06 +0200 Subject: [PATCH 7/7] Fix SslStreamDisposeTest for parallel handshake on Unix (#114100) * [Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix Fixes #113833 * fixup! [Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix Fixes #113833 * fixup! fixup! [Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix Fixes #113833 * Update src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix build --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../tests/FunctionalTests/SslStreamDisposeTest.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs index d3bfcc44488c72..9864029c7a0b34 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs @@ -60,6 +60,7 @@ public async Task Dispose_PendingReadAsync_ThrowsODE(bool bufferedRead) using CancellationTokenSource cts = new CancellationTokenSource(); cts.CancelAfter(TestConfiguration.PassingTestTimeout); + (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(leaveInnerStreamOpen: true); using (client) using (server) @@ -113,8 +114,7 @@ public async Task Dispose_ParallelWithHandshake_ThrowsODE() await Parallel.ForEachAsync(System.Linq.Enumerable.Range(0, 10000), cts.Token, async (i, token) => { - // use real Tcp streams to avoid specific behavior of ConnectedStreams when concurrently disposed - (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedTcpStreams(); + (Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams(); using SslStream client = new SslStream(clientStream); using SslStream server = new SslStream(serverStream); @@ -149,6 +149,11 @@ static async Task ValidateExceptionAsync(Task task) { await task; } + catch (InvalidOperationException ex) when (ex.StackTrace?.Contains("System.IO.StreamBuffer.WriteAsync") ?? true) + { + // Writing to a disposed ConnectedStream (test only, does not happen with NetworkStream) + return; + } catch (Exception ex) when (ex is ObjectDisposedException // disposed locally or IOException // disposed remotely (received unexpected EOF)