Skip to content

Commit cbb1cd1

Browse files
github-actions[bot]rzikmvcsjonesCopilot
authored
[release/8.0-staging] Link peer's X509 stack handle to parent SSL safe handle (#115379)
* 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 3cae60b. * 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 296fa9e commit cbb1cd1

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
@@ -117,7 +117,14 @@ internal static unsafe ReadOnlySpan<byte> SslGetAlpnSelected(SafeSslHandle ssl)
117117
internal static partial IntPtr SslGetPeerCertificate(SafeSslHandle ssl);
118118

119119
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerCertChain")]
120-
internal static partial SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl);
120+
private static partial SafeSharedX509StackHandle SslGetPeerCertChain_private(SafeSslHandle ssl);
121+
122+
internal static SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl)
123+
{
124+
return SafeInteriorHandle.OpenInteriorHandle(
125+
SslGetPeerCertChain_private,
126+
ssl);
127+
}
121128

122129
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerFinished")]
123130
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
@@ -1029,8 +1029,9 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
10291029
return true;
10301030
}
10311031

1032-
_remoteCertificate = certificate;
1033-
if (_remoteCertificate == null)
1032+
// don't assign to _remoteCertificate yet, this prevents weird exceptions if SslStream is disposed in parallel with X509Chain building
1033+
1034+
if (certificate == null)
10341035
{
10351036
if (NetEventSource.Log.IsEnabled() && RemoteCertRequired) NetEventSource.Error(this, $"Remote certificate required, but no remote certificate received");
10361037
sslPolicyErrors |= SslPolicyErrors.RemoteCertificateNotAvailable;
@@ -1072,15 +1073,17 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
10721073
sslPolicyErrors |= CertificateValidationPal.VerifyCertificateProperties(
10731074
_securityContext!,
10741075
chain,
1075-
_remoteCertificate,
1076+
certificate,
10761077
_sslAuthenticationOptions.CheckCertName,
10771078
_sslAuthenticationOptions.IsServer,
10781079
TargetHostNameHelper.NormalizeHostName(_sslAuthenticationOptions.TargetHost));
10791080
}
10801081

1082+
_remoteCertificate = certificate;
1083+
10811084
if (remoteCertValidationCallback != null)
10821085
{
1083-
success = remoteCertValidationCallback(this, _remoteCertificate, chain, sslPolicyErrors);
1086+
success = remoteCertValidationCallback(this, certificate, chain, sslPolicyErrors);
10841087
}
10851088
else
10861089
{

‎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)