Skip to content

Commit 805cb73

Browse files
github-actions[bot]rzikmvcsjonesCopilot
authored
[release/9.0-staging] Link peer's X509 stack handle to parent SSL safe handle (#115380)
* Link peer's X509 stack handle to parent SSL safe handle * Update src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs Co-authored-by: Kevin Jones <vcsjones@github.com> * Add test for dispose parallel with handshake. * Revert "Add test for dispose parallel with handshake." This reverts commit abf96c8. * Defer RemoteCertificate assignment after X509 Chain build (#114781) * Defer RemoteCertificate assignment after X509 Chain build * Add comment * Fix SslStreamDisposeTest failures during parallel handshake (#113834) * Fix SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE test failures * Fix build * 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> --------- Co-authored-by: Radek Zikmund <r.zikmund.rz@gmail.com> Co-authored-by: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Co-authored-by: Kevin Jones <vcsjones@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 9744ebc commit 805cb73

File tree

3 files changed

+77
-5
lines changed

3 files changed

+77
-5
lines changed

‎src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,14 @@ internal static unsafe ReadOnlySpan<byte> SslGetAlpnSelected(SafeSslHandle ssl)
123123
internal static partial IntPtr SslGetCertificate(IntPtr ssl);
124124

125125
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerCertChain")]
126-
internal static partial SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl);
126+
private static partial SafeSharedX509StackHandle SslGetPeerCertChain_private(SafeSslHandle ssl);
127+
128+
internal static SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl)
129+
{
130+
return SafeInteriorHandle.OpenInteriorHandle(
131+
SslGetPeerCertChain_private,
132+
ssl);
133+
}
127134

128135
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerFinished")]
129136
internal static partial int SslGetPeerFinished(SafeSslHandle ssl, IntPtr buf, int count);

‎src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1057,8 +1057,9 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
10571057
return true;
10581058
}
10591059

1060-
_remoteCertificate = certificate;
1061-
if (_remoteCertificate == null)
1060+
// don't assign to _remoteCertificate yet, this prevents weird exceptions if SslStream is disposed in parallel with X509Chain building
1061+
1062+
if (certificate == null)
10621063
{
10631064
if (NetEventSource.Log.IsEnabled() && RemoteCertRequired) NetEventSource.Error(this, $"Remote certificate required, but no remote certificate received");
10641065
sslPolicyErrors |= SslPolicyErrors.RemoteCertificateNotAvailable;
@@ -1100,15 +1101,17 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
11001101
sslPolicyErrors |= CertificateValidationPal.VerifyCertificateProperties(
11011102
_securityContext!,
11021103
chain,
1103-
_remoteCertificate,
1104+
certificate,
11041105
_sslAuthenticationOptions.CheckCertName,
11051106
_sslAuthenticationOptions.IsServer,
11061107
TargetHostNameHelper.NormalizeHostName(_sslAuthenticationOptions.TargetHost));
11071108
}
11081109

1110+
_remoteCertificate = certificate;
1111+
11091112
if (remoteCertValidationCallback != null)
11101113
{
1111-
success = remoteCertValidationCallback(this, _remoteCertificate, chain, sslPolicyErrors);
1114+
success = remoteCertValidationCallback(this, certificate, chain, sslPolicyErrors);
11121115
}
11131116
else
11141117
{

‎src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.IO;
55
using System.Security.Cryptography.X509Certificates;
66
using System.Threading;
7+
using System.Security.Authentication;
78
using System.Threading.Tasks;
89

910
using Xunit;
@@ -59,6 +60,7 @@ public async Task Dispose_PendingReadAsync_ThrowsODE(bool bufferedRead)
5960
using CancellationTokenSource cts = new CancellationTokenSource();
6061
cts.CancelAfter(TestConfiguration.PassingTestTimeout);
6162

63+
6264
(SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(leaveInnerStreamOpen: true);
6365
using (client)
6466
using (server)
@@ -102,5 +104,65 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
102104
await Assert.ThrowsAnyAsync<ObjectDisposedException>(() => client.ReadAsync(readBuffer, cts.Token).AsTask());
103105
}
104106
}
107+
108+
[Fact]
109+
[OuterLoop("Computationally expensive")]
110+
public async Task Dispose_ParallelWithHandshake_ThrowsODE()
111+
{
112+
using CancellationTokenSource cts = new CancellationTokenSource();
113+
cts.CancelAfter(TestConfiguration.PassingTestTimeout);
114+
115+
await Parallel.ForEachAsync(System.Linq.Enumerable.Range(0, 10000), cts.Token, async (i, token) =>
116+
{
117+
(Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams();
118+
119+
using SslStream client = new SslStream(clientStream);
120+
using SslStream server = new SslStream(serverStream);
121+
using X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate();
122+
using X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate();
123+
124+
SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions()
125+
{
126+
TargetHost = Guid.NewGuid().ToString("N"),
127+
RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true,
128+
};
129+
130+
SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions()
131+
{
132+
ServerCertificate = serverCertificate,
133+
};
134+
135+
var clientTask = Task.Run(() => client.AuthenticateAsClientAsync(clientOptions, cts.Token));
136+
var serverTask = Task.Run(() => server.AuthenticateAsServerAsync(serverOptions, cts.Token));
137+
138+
// Dispose the instances while the handshake is in progress.
139+
client.Dispose();
140+
server.Dispose();
141+
142+
await ValidateExceptionAsync(clientTask);
143+
await ValidateExceptionAsync(serverTask);
144+
});
145+
146+
static async Task ValidateExceptionAsync(Task task)
147+
{
148+
try
149+
{
150+
await task;
151+
}
152+
catch (InvalidOperationException ex) when (ex.StackTrace?.Contains("System.IO.StreamBuffer.WriteAsync") ?? true)
153+
{
154+
// Writing to a disposed ConnectedStream (test only, does not happen with NetworkStream)
155+
return;
156+
}
157+
catch (Exception ex) when (ex
158+
is ObjectDisposedException // disposed locally
159+
or IOException // disposed remotely (received unexpected EOF)
160+
or AuthenticationException) // disposed wrapped in AuthenticationException or error from platform library
161+
{
162+
// expected
163+
return;
164+
}
165+
}
166+
}
105167
}
106168
}

0 commit comments

Comments
 (0)