From b137e1d573f6c010ea0b6e4524c9acb34237666b Mon Sep 17 00:00:00 2001 From: Lakshmi Priya Sekar Date: Tue, 24 Oct 2017 15:34:44 -0700 Subject: [PATCH 1/3] Implement cancellation policy for sslstream. --- .../src/System/Net/Security/SslState.cs | 94 +++++++++++++------ .../src/System/Net/Security/SslStream.cs | 8 +- 2 files changed, 68 insertions(+), 34 deletions(-) diff --git a/src/System.Net.Security/src/System/Net/Security/SslState.cs b/src/System.Net.Security/src/System/Net/Security/SslState.cs index 3fd95ad1b9ad..530ca3a82f55 100644 --- a/src/System.Net.Security/src/System/Net/Security/SslState.cs +++ b/src/System.Net.Security/src/System/Net/Security/SslState.cs @@ -569,7 +569,7 @@ internal int CheckOldKeyDecryptedData(byte[] buffer, int offset, int count) // This method assumes that a SSPI context is already in a good shape. // For example it is either a fresh context or already authenticated context that needs renegotiation. // - internal void ProcessAuthentication(LazyAsyncResult lazyResult) + internal void ProcessAuthentication(LazyAsyncResult lazyResult, CancellationToken cancellationToken) { if (Interlocked.Exchange(ref _nestedAuth, 1) == 1) { @@ -592,7 +592,12 @@ internal void ProcessAuthentication(LazyAsyncResult lazyResult) // A trick to discover and avoid cached sessions. _CachedSession = CachedSessionStatus.Unknown; - ForceAuthentication(Context.IsServer, null, asyncRequest); + if (asyncRequest != null) + { + cancellationToken.ThrowIfCancellationRequested(); + } + + ForceAuthentication(Context.IsServer, null, asyncRequest, cancellationToken); // Not aync so the connection is completed at this point. if (lazyResult == null && NetEventSource.IsEnabled) @@ -651,7 +656,8 @@ internal void ReplyOnReAuthentication(byte[] buffer) AsyncProtocolRequest asyncRequest = new AsyncProtocolRequest(new LazyAsyncResult(this, null, new AsyncCallback(RehandshakeCompleteCallback))); // Buffer contains a result from DecryptMessage that will be passed to ISC/ASC asyncRequest.Buffer = buffer; - ForceAuthentication(false, buffer, asyncRequest); + // This is not called from AuthenticateAs* methods, no cancellation token available. + ForceAuthentication(false, buffer, asyncRequest, CancellationToken.None); } // @@ -659,7 +665,7 @@ internal void ReplyOnReAuthentication(byte[] buffer) // Incoming buffer is either null or is the result of "renegotiate" decrypted message // If write is in progress the method will either wait or be put on hold // - private void ForceAuthentication(bool receiveFirst, byte[] buffer, AsyncProtocolRequest asyncRequest) + private void ForceAuthentication(bool receiveFirst, byte[] buffer, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken) { if (CheckEnqueueHandshake(buffer, asyncRequest)) { @@ -671,17 +677,20 @@ private void ForceAuthentication(bool receiveFirst, byte[] buffer, AsyncProtocol // This will tell that we don't know the framing yet (what SSL version is) _Framing = Framing.Unknown; + // Throw if cancellation requested. + cancellationToken.ThrowIfCancellationRequested(); + try { if (receiveFirst) { // Listen for a client blob. - StartReceiveBlob(buffer, asyncRequest); + StartReceiveBlob(buffer, asyncRequest, cancellationToken); } else { // We start with the first blob. - StartSendBlob(buffer, (buffer == null ? 0 : buffer.Length), asyncRequest); + StartSendBlob(buffer, (buffer == null ? 0 : buffer.Length), asyncRequest, cancellationToken); } } catch (Exception e) @@ -763,7 +772,7 @@ internal void InternalEndProcessAuthentication(LazyAsyncResult lazyResult) // // Client side starts here, but server also loops through this method. // - private void StartSendBlob(byte[] incoming, int count, AsyncProtocolRequest asyncRequest) + private void StartSendBlob(byte[] incoming, int count, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken) { ProtocolToken message = Context.NextMessage(incoming, 0, count); _securityStatus = message.Status; @@ -790,6 +799,9 @@ private void StartSendBlob(byte[] incoming, int count, AsyncProtocolRequest asyn } else { + // Throw before starting async write + cancellationToken.ThrowIfCancellationRequested(); + asyncRequest.AsyncState = message; Task t = InnerStream.WriteAsync(message.Payload, 0, message.Size); if (t.IsCompleted) @@ -811,17 +823,22 @@ private void StartSendBlob(byte[] incoming, int count, AsyncProtocolRequest asyn } } - CheckCompletionBeforeNextReceive(message, asyncRequest); + CheckCompletionBeforeNextReceive(message, asyncRequest, cancellationToken); } // // This will check and logically complete / fail the auth handshake. // - private void CheckCompletionBeforeNextReceive(ProtocolToken message, AsyncProtocolRequest asyncRequest) + private void CheckCompletionBeforeNextReceive(ProtocolToken message, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken) { + if (asyncRequest != null) + { + cancellationToken.ThrowIfCancellationRequested(); + } + if (message.Failed) { - StartSendAuthResetSignal(null, asyncRequest, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_auth_SSPI, message.GetException()))); + StartSendAuthResetSignal(null, asyncRequest, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_auth_SSPI, message.GetException())), cancellationToken); return; } else if (message.Done && !_pendingReHandshake) @@ -830,7 +847,7 @@ private void CheckCompletionBeforeNextReceive(ProtocolToken message, AsyncProtoc if (!CompleteHandshake(ref alertToken)) { - StartSendAuthResetSignal(alertToken, asyncRequest, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_ssl_io_cert_validation, null))); + StartSendAuthResetSignal(alertToken, asyncRequest, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_ssl_io_cert_validation, null)), cancellationToken); return; } @@ -840,13 +857,13 @@ private void CheckCompletionBeforeNextReceive(ProtocolToken message, AsyncProtoc return; } - StartReceiveBlob(message.Payload, asyncRequest); + StartReceiveBlob(message.Payload, asyncRequest, cancellationToken); } // // Server side starts here, but client also loops through this method. // - private void StartReceiveBlob(byte[] buffer, AsyncProtocolRequest asyncRequest) + private void StartReceiveBlob(byte[] buffer, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken) { if (_pendingReHandshake) { @@ -858,12 +875,12 @@ private void StartReceiveBlob(byte[] buffer, AsyncProtocolRequest asyncRequest) if (!_pendingReHandshake) { // Renegotiate: proceed to the next step. - ProcessReceivedBlob(buffer, buffer.Length, asyncRequest); + ProcessReceivedBlob(buffer, buffer.Length, asyncRequest, cancellationToken); return; } } - //This is first server read. + // This is first server read. buffer = EnsureBufferSize(buffer, 0, SecureChannel.ReadHeaderSize); int readBytes = 0; @@ -873,6 +890,9 @@ private void StartReceiveBlob(byte[] buffer, AsyncProtocolRequest asyncRequest) } else { + // Throw before server read. + cancellationToken.ThrowIfCancellationRequested(); + asyncRequest.SetNextRequest(buffer, 0, SecureChannel.ReadHeaderSize, s_partialFrameCallback); FixedSizeReader.ReadPacketAsync(_innerStream, asyncRequest); if (!asyncRequest.MustCompleteSynchronously) @@ -883,11 +903,11 @@ private void StartReceiveBlob(byte[] buffer, AsyncProtocolRequest asyncRequest) readBytes = asyncRequest.Result; } - StartReadFrame(buffer, readBytes, asyncRequest); + StartReadFrame(buffer, readBytes, asyncRequest, cancellationToken); } // - private void StartReadFrame(byte[] buffer, int readBytes, AsyncProtocolRequest asyncRequest) + private void StartReadFrame(byte[] buffer, int readBytes, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken) { if (readBytes == 0) { @@ -921,6 +941,8 @@ private void StartReadFrame(byte[] buffer, int readBytes, AsyncProtocolRequest a } else { + cancellationToken.ThrowIfCancellationRequested(); + asyncRequest.SetNextRequest(buffer, readBytes, restBytes, s_readFrameCallback); FixedSizeReader.ReadPacketAsync(_innerStream, asyncRequest); if (!asyncRequest.MustCompleteSynchronously) @@ -935,10 +957,11 @@ private void StartReadFrame(byte[] buffer, int readBytes, AsyncProtocolRequest a readBytes = 0; } } - ProcessReceivedBlob(buffer, readBytes + restBytes, asyncRequest); + + ProcessReceivedBlob(buffer, readBytes + restBytes, asyncRequest, cancellationToken); } - private void ProcessReceivedBlob(byte[] buffer, int count, AsyncProtocolRequest asyncRequest) + private void ProcessReceivedBlob(byte[] buffer, int count, AsyncProtocolRequest asyncRequest, CancellationToken cancellationToken) { if (count == 0) { @@ -946,6 +969,11 @@ private void ProcessReceivedBlob(byte[] buffer, int count, AsyncProtocolRequest throw new AuthenticationException(SR.net_auth_eof, null); } + if (asyncRequest != null) + { + cancellationToken.ThrowIfCancellationRequested(); + } + if (_pendingReHandshake) { int offset = 0; @@ -956,19 +984,19 @@ private void ProcessReceivedBlob(byte[] buffer, int count, AsyncProtocolRequest Exception e = EnqueueOldKeyDecryptedData(buffer, offset, count); if (e != null) { - StartSendAuthResetSignal(null, asyncRequest, ExceptionDispatchInfo.Capture(e)); + StartSendAuthResetSignal(null, asyncRequest, ExceptionDispatchInfo.Capture(e), cancellationToken); return; } _Framing = Framing.Unknown; - StartReceiveBlob(buffer, asyncRequest); + StartReceiveBlob(buffer, asyncRequest, cancellationToken); return; } else if (status.ErrorCode != SecurityStatusPalErrorCode.Renegotiate) { // Fail re-handshake. ProtocolToken message = new ProtocolToken(null, status); - StartSendAuthResetSignal(null, asyncRequest, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_auth_SSPI, message.GetException()))); + StartSendAuthResetSignal(null, asyncRequest, ExceptionDispatchInfo.Capture(new AuthenticationException(SR.net_auth_SSPI, message.GetException())), cancellationToken); return; } @@ -980,15 +1008,20 @@ private void ProcessReceivedBlob(byte[] buffer, int count, AsyncProtocolRequest } } - StartSendBlob(buffer, count, asyncRequest); + StartSendBlob(buffer, count, asyncRequest, cancellationToken); } // // This is to reset auth state on remote side. // If this write succeeds we will allow auth retrying. // - private void StartSendAuthResetSignal(ProtocolToken message, AsyncProtocolRequest asyncRequest, ExceptionDispatchInfo exception) + private void StartSendAuthResetSignal(ProtocolToken message, AsyncProtocolRequest asyncRequest, ExceptionDispatchInfo exception, CancellationToken cancellationToken) { + if (asyncRequest != null) + { + cancellationToken.ThrowIfCancellationRequested(); + } + if (message == null || message.Size == 0) { // @@ -1094,7 +1127,8 @@ private static void WriteCallback(IAsyncResult transportResult) exception.Throw(); } - sslState.CheckCompletionBeforeNextReceive((ProtocolToken)asyncState, asyncRequest); + // Not allowing cancellation in the callback. + sslState.CheckCompletionBeforeNextReceive((ProtocolToken)asyncState, asyncRequest, CancellationToken.None); } catch (Exception e) { @@ -1117,7 +1151,7 @@ private static void PartialFrameCallback(AsyncProtocolRequest asyncRequest) SslState sslState = (SslState)asyncRequest.AsyncObject; try { - sslState.StartReadFrame(asyncRequest.Buffer, asyncRequest.Result, asyncRequest); + sslState.StartReadFrame(asyncRequest.Buffer, asyncRequest.Result, asyncRequest, CancellationToken.None); } catch (Exception e) { @@ -1148,7 +1182,7 @@ private static void ReadFrameCallback(AsyncProtocolRequest asyncRequest) asyncRequest.Offset = 0; } - sslState.ProcessReceivedBlob(asyncRequest.Buffer, asyncRequest.Offset + asyncRequest.Result, asyncRequest); + sslState.ProcessReceivedBlob(asyncRequest.Buffer, asyncRequest.Offset + asyncRequest.Result, asyncRequest, CancellationToken.None); } catch (Exception e) { @@ -1742,7 +1776,7 @@ private void AsyncResumeHandshake(object state) try { - ForceAuthentication(Context.IsServer, request.Buffer, request); + ForceAuthentication(Context.IsServer, request.Buffer, request, CancellationToken.None); } catch (Exception e) { @@ -1761,12 +1795,12 @@ private void AsyncResumeHandshakeRead(object state) if (_pendingReHandshake) { // Resume as read a blob. - StartReceiveBlob(asyncRequest.Buffer, asyncRequest); + StartReceiveBlob(asyncRequest.Buffer, asyncRequest, CancellationToken.None); } else { // Resume as process the blob. - ProcessReceivedBlob(asyncRequest.Buffer, asyncRequest.Buffer == null ? 0 : asyncRequest.Buffer.Length, asyncRequest); + ProcessReceivedBlob(asyncRequest.Buffer, asyncRequest.Buffer == null ? 0 : asyncRequest.Buffer.Length, asyncRequest, CancellationToken.None); } } catch (Exception e) diff --git a/src/System.Net.Security/src/System/Net/Security/SslStream.cs b/src/System.Net.Security/src/System/Net/Security/SslStream.cs index aa30e5e3ca20..7c64b7e6cd72 100644 --- a/src/System.Net.Security/src/System/Net/Security/SslStream.cs +++ b/src/System.Net.Security/src/System/Net/Security/SslStream.cs @@ -185,7 +185,7 @@ internal virtual IAsyncResult BeginAuthenticateAsClient(SslClientAuthenticationO _sslState.ValidateCreateContext(sslClientAuthenticationOptions); LazyAsyncResult result = new LazyAsyncResult(_sslState, asyncState, asyncCallback); - _sslState.ProcessAuthentication(result); + _sslState.ProcessAuthentication(result, cancellationToken); return result; } @@ -239,7 +239,7 @@ private IAsyncResult BeginAuthenticateAsServer(SslServerAuthenticationOptions ss _sslState.ValidateCreateContext(sslServerAuthenticationOptions); LazyAsyncResult result = new LazyAsyncResult(_sslState, asyncState, asyncCallback); - _sslState.ProcessAuthentication(result); + _sslState.ProcessAuthentication(result, cancellationToken); return result; } @@ -307,7 +307,7 @@ private void AuthenticateAsClient(SslClientAuthenticationOptions sslClientAuthen sslClientAuthenticationOptions._certSelectionDelegate = _certSelectionDelegate; _sslState.ValidateCreateContext(sslClientAuthenticationOptions); - _sslState.ProcessAuthentication(null); + _sslState.ProcessAuthentication(null, CancellationToken.None); } public virtual void AuthenticateAsServer(X509Certificate serverCertificate) @@ -343,7 +343,7 @@ private void AuthenticateAsServer(SslServerAuthenticationOptions sslServerAuthen sslServerAuthenticationOptions._certValidationDelegate = _certValidationDelegate; _sslState.ValidateCreateContext(sslServerAuthenticationOptions); - _sslState.ProcessAuthentication(null); + _sslState.ProcessAuthentication(null, CancellationToken.None); } #endregion From 05054d4d3c7381fc0722b08480eb6fc0baa199cc Mon Sep 17 00:00:00 2001 From: Lakshmi Priya Sekar Date: Tue, 24 Oct 2017 16:13:29 -0700 Subject: [PATCH 2/3] Add tests. --- .../FunctionalTests/SslStreamAlpnTests.cs | 66 +++++++++++++++++++ .../tests/UnitTests/Fakes/FakeSslState.cs | 2 +- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs b/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs index bcf10160df61..fd282c8e74a0 100644 --- a/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs +++ b/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs @@ -60,6 +60,72 @@ protected bool AllowAnyServerCertificate( } } + [Fact] + public void SslStream_StreamToStream_ClientCancellation_Throws() + { + VirtualNetwork network = new VirtualNetwork(); + using (var clientStream = new VirtualNetworkStream(network, false)) + using (var serverStream = new VirtualNetworkStream(network, true)) + using (var client = new SslStream(clientStream)) + using (var server = new SslStream(serverStream)) + using (X509Certificate2 certificate = Configuration.Certificates.GetServerCertificate()) + { + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions(); + clientOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate; + clientOptions.TargetHost = certificate.GetNameInfo(X509NameType.SimpleName, false); + + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions(); + serverOptions.ServerCertificate = certificate; + serverOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate; + + CancellationTokenSource cts = new CancellationTokenSource(); + + Task clientTask = client.AuthenticateAsClientAsync(clientOptions, cts.Token); + Task serverTask = server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); + + Assert.ThrowsAsync(async () => + { + cts.Cancel(); + await clientTask; + }); + + Assert.ThrowsAsync(async () => { await serverTask; }); + } + } + + [Fact] + public void SslStream_StreamToStream_ServerCancellation_Throws() + { + VirtualNetwork network = new VirtualNetwork(); + using (var clientStream = new VirtualNetworkStream(network, false)) + using (var serverStream = new VirtualNetworkStream(network, true)) + using (var client = new SslStream(clientStream)) + using (var server = new SslStream(serverStream)) + using (X509Certificate2 certificate = Configuration.Certificates.GetServerCertificate()) + { + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions(); + clientOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate; + clientOptions.TargetHost = certificate.GetNameInfo(X509NameType.SimpleName, false); + + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions(); + serverOptions.ServerCertificate = certificate; + serverOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate; + + CancellationTokenSource cts = new CancellationTokenSource(); + + Task clientTask = client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); + Task serverTask = server.AuthenticateAsServerAsync(serverOptions, cts.Token); + + Assert.ThrowsAsync(async () => { await clientTask; }); + + Assert.ThrowsAsync(async () => + { + cts.Cancel(); + await serverTask; + }); + } + } + [Fact] public void SslStream_StreamToStream_DuplicateOptions_Throws() { diff --git a/src/System.Net.Security/tests/UnitTests/Fakes/FakeSslState.cs b/src/System.Net.Security/tests/UnitTests/Fakes/FakeSslState.cs index 98f481df35cb..bb32bb901a2f 100644 --- a/src/System.Net.Security/tests/UnitTests/Fakes/FakeSslState.cs +++ b/src/System.Net.Security/tests/UnitTests/Fakes/FakeSslState.cs @@ -183,7 +183,7 @@ internal void Close() // This method assumes that a SSPI context is already in a good shape. // For example it is either a fresh context or already authenticated context that needs renegotiation. // - internal void ProcessAuthentication(LazyAsyncResult lazyResult) + internal void ProcessAuthentication(LazyAsyncResult lazyResult, CancellationToken cancellationToken) { } From 42bf8d1020440755bcbfedfb2ab40b16d147b31f Mon Sep 17 00:00:00 2001 From: Lakshmi Priya Sekar Date: Tue, 24 Oct 2017 16:47:57 -0700 Subject: [PATCH 3/3] Fix bug in previous test. --- .../tests/FunctionalTests/SslStreamAlpnTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs b/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs index fd282c8e74a0..220cf956c51e 100644 --- a/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs +++ b/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs @@ -204,8 +204,8 @@ public void SslStream_StreamToStream_Alpn_NonMatchingProtocols_Fail() ServerCertificate = certificate, }; - Assert.ThrowsAsync(() => { return client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); }); - Assert.ThrowsAsync(() => { return server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); }); + Assert.ThrowsAsync(async () => { await client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); }); + Assert.ThrowsAsync(async () => { await server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); }); } }