From 2ac023c8e9df61fe4557212cbd13956042fb47c5 Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Thu, 24 Jun 2021 16:31:07 +0200 Subject: [PATCH] process more TLS frames at one when available (#50815) * process more TLS frames when available * add SslStream.Implementation.cs * remove extra comment * add back DefaultRequestHeaders_SentUnparsed * feedback from review * fix winhttp * fix linker test * feedback from review * feedback from review * feedback from review * make EnsureFullTlsFrame async * feedback from review --- .../Interop.Ssl.cs | 2 +- .../Interop.OpenSsl.cs | 3 - .../System/Net/Http/SchSendAuxRecordHttpTest.cs | 127 --------- .../tests/FunctionalTests/PlatformHandlerTest.cs | 12 - ...Net.Http.WinHttpHandler.Functional.Tests.csproj | 2 - .../FunctionalTests/SocketsHttpHandlerTest.cs | 5 - .../System.Net.Http.Functional.Tests.csproj | 2 - .../src/System/Net/Logging/NetEventSource.cs | 2 +- .../src/System/Net/Security/SecureChannel.cs | 20 +- .../Net/Security/SslStream.Implementation.cs | 298 ++++++++++++++------- .../System/Net/Security/SslStreamPal.Android.cs | 13 +- .../src/System/Net/Security/SslStreamPal.OSX.cs | 18 +- .../src/System/Net/Security/SslStreamPal.Unix.cs | 7 +- .../System/Net/Security/SslStreamPal.Windows.cs | 8 +- .../src/System/Net/Security/TlsFrameHelper.cs | 5 + .../SslStreamSchSendAuxRecordTest.cs | 125 --------- .../System.Net.Security.Tests.csproj | 1 - 17 files changed, 239 insertions(+), 411 deletions(-) delete mode 100644 src/libraries/Common/tests/System/Net/Http/SchSendAuxRecordHttpTest.cs delete mode 100644 src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSchSendAuxRecordTest.cs diff --git a/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs b/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs index 762d352..6f0c53b 100644 --- a/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs @@ -166,7 +166,7 @@ internal static partial class Interop out int bytesRead); internal static unsafe PAL_SSLStreamStatus SSLStreamRead( SafeSslHandle sslHandle, - Span buffer, + ReadOnlySpan buffer, out int bytesRead) { fixed (byte* bufferPtr = buffer) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 42276ab..604ac8b 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -2,19 +2,16 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Buffers; using System.Collections.Generic; using System.Diagnostics; using System.Globalization; using System.IO; -using System.Net.Http; using System.Net.Security; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security.Authentication; using System.Security.Authentication.ExtendedProtection; using System.Security.Cryptography; -using System.Security.Cryptography.X509Certificates; using Microsoft.Win32.SafeHandles; internal static partial class Interop diff --git a/src/libraries/Common/tests/System/Net/Http/SchSendAuxRecordHttpTest.cs b/src/libraries/Common/tests/System/Net/Http/SchSendAuxRecordHttpTest.cs deleted file mode 100644 index 0348db4..0000000 --- a/src/libraries/Common/tests/System/Net/Http/SchSendAuxRecordHttpTest.cs +++ /dev/null @@ -1,127 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Net.Test.Common; -using System.Security.Authentication; -using System.Threading.Tasks; - -using Xunit; -using Xunit.Abstractions; - -namespace System.Net.Http.Functional.Tests -{ -#if WINHTTPHANDLER_TEST - using HttpClientHandler = System.Net.Http.WinHttpClientHandler; -#endif - - public abstract class SchSendAuxRecordHttpTest : HttpClientHandlerTestBase - { - public SchSendAuxRecordHttpTest(ITestOutputHelper output) : base(output) { } - - private class CircularBuffer - { - public CircularBuffer(int size) => _buffer = new char[size]; - - private char[] _buffer; - private int _lastBytesWriteIndex = 0; - private int _size = 0; - - public void Add(string value) - { - foreach (char ch in value) - { - _buffer[_lastBytesWriteIndex] = ch; - - _lastBytesWriteIndex = ++_lastBytesWriteIndex % _buffer.Length; - _size = Math.Min(_buffer.Length, ++_size); - } - } - - public bool Equals(string value) - { - if (value.Length != _size) - return false; - - for (int i = 0; i < _size; i++) - { - if (_buffer[(_lastBytesWriteIndex + i) % _buffer.Length] != value[i]) - return false; - } - - return true; - } - } - - [Fact] - [PlatformSpecific(TestPlatforms.Windows)] - public async Task HttpClient_ClientUsesAuxRecord_Ok() - { - var options = new HttpsTestServer.Options(); - options.AllowedProtocols = SslProtocols.Tls; - - using (var server = new HttpsTestServer(options)) - using (HttpClientHandler handler = CreateHttpClientHandler()) - using (HttpClient client = CreateHttpClient(handler)) - { - handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates; - server.Start(); - - var tasks = new Task[2]; - - bool serverAuxRecordDetected = false; - bool serverAuxRecordDetectedInconclusive = false; - int serverTotalBytesReceived = 0; - int serverChunks = 0; - - CircularBuffer buffer = new CircularBuffer(4); - - tasks[0] = server.AcceptHttpsClientAsync((requestString) => - { - - buffer.Add(requestString); - - serverTotalBytesReceived += requestString.Length; - - if (serverTotalBytesReceived == 1 && serverChunks == 0) - { - serverAuxRecordDetected = true; - } - - serverChunks++; - - // Test is inconclusive if any non-CBC cipher is used: - if (server.Stream.CipherAlgorithm == CipherAlgorithmType.None || - server.Stream.CipherAlgorithm == CipherAlgorithmType.Null || - server.Stream.CipherAlgorithm == CipherAlgorithmType.Rc4) - { - serverAuxRecordDetectedInconclusive = true; - } - - // Detect end of HTML request - if (buffer.Equals("\r\n\r\n")) - { - return Task.FromResult(HttpsTestServer.Options.DefaultResponseString); - } - else - { - return Task.FromResult(null); - } - }); - - string requestUriString = "https://localhost:" + server.Port.ToString(); - tasks[1] = client.GetStringAsync(requestUriString); - - await tasks.WhenAllOrAnyFailed(15 * 1000); - - if (serverAuxRecordDetectedInconclusive) - { - _output.WriteLine("Test inconclusive: The Operating system preferred a non-CBC or Null cipher."); - } - else - { - Assert.True(serverAuxRecordDetected, "Server reports: Client auxiliary record not detected."); - } - } - } - } -} diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs b/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs index 9e384c2..9e6d360 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs @@ -135,11 +135,6 @@ namespace System.Net.Http.Functional.Tests public PlatformHandler_HttpClientHandler_Proxy_Test(ITestOutputHelper output) : base(output) { } } - public sealed class PlatformHandler_SchSendAuxRecordHttpTest : SchSendAuxRecordHttpTest - { - public PlatformHandler_SchSendAuxRecordHttpTest(ITestOutputHelper output) : base(output) { } - } - public sealed class PlatformHandler_HttpClientHandlerTest : HttpClientHandlerTest { public PlatformHandler_HttpClientHandlerTest(ITestOutputHelper output) : base(output) { } @@ -299,13 +294,6 @@ namespace System.Net.Http.Functional.Tests public PlatformHandler_HttpClientHandler_Proxy_Http2_Test(ITestOutputHelper output) : base(output) { } } - public sealed class PlatformHandler_SchSendAuxRecordHttp_Http2_Test : SchSendAuxRecordHttpTest - { - protected override Version UseVersion => HttpVersion20.Value; - - public PlatformHandler_SchSendAuxRecordHttp_Http2_Test(ITestOutputHelper output) : base(output) { } - } - [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows10Version1607OrGreater))] public sealed class PlatformHandler_HttpClientHandler_Http2_Test : HttpClientHandlerTest { diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj b/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj index c72ec6b..4b6596d 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj @@ -127,8 +127,6 @@ Link="Common\System\Net\Http\RepeatedFlushContent.cs" /> - - diff --git a/src/libraries/System.Net.Security/src/System/Net/Logging/NetEventSource.cs b/src/libraries/System.Net.Security/src/System/Net/Logging/NetEventSource.cs index 63af260..f71c6a9 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Logging/NetEventSource.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Logging/NetEventSource.cs @@ -13,7 +13,7 @@ namespace System.Net /// The buffer to be logged. /// The calling member. [NonEvent] - public static void DumpBuffer(object thisOrContextObject, ReadOnlyMemory buffer, [CallerMemberName] string? memberName = null) + public static void DumpBuffer(object thisOrContextObject, ReadOnlySpan buffer, [CallerMemberName] string? memberName = null) { if (Log.IsEnabled()) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs index 5d234b5..2cbc44c 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs @@ -879,7 +879,7 @@ namespace System.Net.Security --*/ internal SecurityStatusPal Encrypt(ReadOnlyMemory buffer, ref byte[] output, out int resultSize) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.DumpBuffer(this, buffer); + if (NetEventSource.Log.IsEnabled()) NetEventSource.DumpBuffer(this, buffer.Span); byte[] writeBuffer = output; @@ -903,24 +903,12 @@ namespace System.Net.Security return secStatus; } - internal SecurityStatusPal Decrypt(byte[]? payload, ref int offset, ref int count) + internal SecurityStatusPal Decrypt(Span buffer, out int outputOffset, out int outputCount) { - if ((uint)offset > (uint)(payload == null ? 0 : payload.Length)) - { - Debug.Fail("Argument 'offset' out of range."); - throw new ArgumentOutOfRangeException(nameof(offset)); - } - - if ((uint)count > (uint)(payload == null ? 0 : payload.Length - offset)) - { - Debug.Fail("Argument 'count' out of range."); - throw new ArgumentOutOfRangeException(nameof(count)); - } - - SecurityStatusPal status = SslStreamPal.DecryptMessage(_securityContext!, payload!, ref offset, ref count); + SecurityStatusPal status = SslStreamPal.DecryptMessage(_securityContext!, buffer, out outputOffset, out outputCount); if (NetEventSource.Log.IsEnabled() && status.ErrorCode == SecurityStatusPalErrorCode.OK) { - NetEventSource.DumpBuffer(this, payload!, offset, count); + NetEventSource.DumpBuffer(this, buffer.Slice(outputOffset, outputCount)); } return status; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index 81f2c92..17858bb 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -43,6 +43,7 @@ namespace System.Net.Security private const int ReadBufferSize = 4096 * 4 + FrameOverhead; // We read in 16K chunks + headers. private const int InitialHandshakeBufferSize = 4096 + FrameOverhead; // try to fit at least 4K ServerCertificate private ArrayBuffer _handshakeBuffer; + private bool _receivedEOF; // Used by Telemetry to ensure we log connection close exactly once // 0 = no handshake @@ -183,17 +184,6 @@ namespace System.Net.Security } } - private SecurityStatusPal DecryptData() - { - ThrowIfExceptionalOrNotAuthenticated(); - return PrivateDecryptData(_internalBuffer, ref _decryptedBytesOffset, ref _decryptedBytesCount); - } - - private SecurityStatusPal PrivateDecryptData(byte[]? buffer, ref int offset, ref int count) - { - return _context!.Decrypt(buffer, ref offset, ref 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. @@ -813,6 +803,117 @@ namespace System.Net.Security _decryptedBytesCount = 0; _decryptedBytesOffset = 0; } + else if (_decryptedBytesCount == 0) + { + _decryptedBytesOffset = 0; + } + } + + + private bool HaveFullTlsFrame(out int frameSize) + { + if (_internalBufferCount < SecureChannel.ReadHeaderSize) + { + frameSize = int.MaxValue; + return false; + } + + frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + return _internalBufferCount >= frameSize; + } + + + private async ValueTask EnsureFullTlsFrameAsync(TIOAdapter adapter) + where TIOAdapter : IReadWriteAdapter + { + int frameSize; + if (HaveFullTlsFrame(out frameSize)) + { + return frameSize; + } + + // We may have enough space to complete frame, but we may still do extra IO if the frame is small. + // So we will attempt larger read - that is trade of with extra copy. + // This may be updated at some point based on size of existing chunk, rented buffer and size of 'buffer'. + ResetReadBuffer(); + + // _internalOffset is 0 after ResetReadBuffer and we use _internalBufferCount to determined where to read. + while (_internalBufferCount < frameSize) + { + // We either don't have full frame or we don't have enough data to even determine the size. + int bytesRead = await adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)).ConfigureAwait(false); + if (bytesRead == 0) + { + if (_internalBufferCount != 0) + { + // we got EOF in middle of TLS frame. Treat that as error. + throw new IOException(SR.net_io_eof); + } + + return 0; + } + + _internalBufferCount += bytesRead; + if (frameSize == int.MaxValue && _internalBufferCount > SecureChannel.ReadHeaderSize) + { + // recalculate frame size if needed e.g. we could not get it before. + frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + } + } + + return frameSize; + } + + private SecurityStatusPal DecryptData(int frameSize) + { + Debug.Assert(_decryptedBytesCount == 0); + + // Set _decryptedBytesOffset/Count to the current frame we have (including header) + // DecryptData will decrypt in-place and modify these to point to the actual decrypted data, which may be smaller. + _decryptedBytesOffset = _internalOffset; + _decryptedBytesCount = frameSize; + SecurityStatusPal status; + + lock (_handshakeLock) + { + ThrowIfExceptionalOrNotAuthenticated(); + status = _context!.Decrypt(new Span(_internalBuffer, _internalOffset, frameSize), out int decryptedOffset, out int decryptedCount); + _decryptedBytesCount = decryptedCount; + if (decryptedCount > 0) + { + _decryptedBytesOffset = _internalOffset + decryptedOffset; + } + + if (status.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) + { + // The status indicates that peer wants to renegotiate. (Windows only) + // In practice, there can be some other reasons too - like TLS1.3 session creation + // of alert handling. We need to pass the data to lsass and it is not safe to do parallel + // write any more as that can change TLS state and the EncryptData() can fail in strange ways. + + // To handle this we call DecryptData() under lock and we create TCS waiter. + // EncryptData() checks that under same lock and if it exist it will not call low-level crypto. + // Instead it will wait synchronously or asynchronously and it will try again after the wait. + // The result will be set when ReplyOnReAuthenticationAsync() is finished e.g. lsass business is over. + // If that happen before EncryptData() runs, _handshakeWaiter will be set to null + // and EncryptData() will work normally e.g. no waiting, just exclusion with DecryptData() + + + if (_sslAuthenticationOptions!.AllowRenegotiation || SslProtocol == SslProtocols.Tls13 || _nestedAuth != 0) + { + // create TCS only if we plan to proceed. If not, we will throw later outside of the lock. + // Tls1.3 does not have renegotiation. However on Windows this error code is used + // for session management e.g. anything lsass needs to see. + // We also allow it when explicitly requested using RenegotiateAsync(). + _handshakeWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + } + } + } + + // Treat the bytes we just decrypted as consumed + ConsumeBufferedBytes(frameSize); + + return status; } private async ValueTask ReadAsyncInternal(TIOAdapter adapter, Memory buffer, bool renegotiation = false) @@ -826,109 +927,63 @@ namespace System.Net.Security } } + ThrowIfExceptionalOrNotAuthenticated(); + Debug.Assert(_internalBuffer is null || _internalBufferCount > 0 || _decryptedBytesCount > 0, "_internalBuffer allocated when no data is buffered."); + int processedLength = 0; + int payloadBytes = 0; try { - while (true) + if (_decryptedBytesCount != 0) { - if (_decryptedBytesCount != 0) + if (renegotiation) { - if (renegotiation) - { - throw new InvalidOperationException(SR.net_ssl_renegotiate_data); - } - - return CopyDecryptedData(buffer); + throw new InvalidOperationException(SR.net_ssl_renegotiate_data); } - if (buffer.Length == 0 && _internalBuffer is null && !renegotiation) + processedLength = CopyDecryptedData(buffer); + if (processedLength == buffer.Length || !HaveFullTlsFrame(out payloadBytes)) { - // User requested a zero-byte read, and we have no data available in the buffer for processing. - // This zero-byte read indicates their desire to trade off the extra cost of a zero-byte read - // for reduced memory consumption when data is not immediately available. - // So, we will issue our own zero-byte read against the underlying stream and defer buffer allocation - // until data is actually available from the underlying stream. - // Note that if the underlying stream does not supporting blocking on zero byte reads, then this will - // complete immediately and won't save any memory, but will still function correctly. - await adapter.ReadAsync(Memory.Empty).ConfigureAwait(false); + // We either filled whole buffer or used all buffered frames. + return processedLength; } - ResetReadBuffer(); - - // Read the next frame header. - if (_internalBufferCount < SecureChannel.ReadHeaderSize) - { - // We don't have enough bytes buffered, so issue an initial read to try to get enough. This is - // done in this method both to better consolidate error handling logic (the first read is the special - // case that needs to differentiate reading 0 from > 0, and everything else needs to throw if it - // doesn't read enough), and to minimize the chances that in the common case the FillBufferAsync - // helper needs to yield and allocate a state machine. - int readBytes = await adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)).ConfigureAwait(false); - if (readBytes == 0) - { - return 0; - } - - _internalBufferCount += readBytes; - if (_internalBufferCount < SecureChannel.ReadHeaderSize) - { - await FillBufferAsync(adapter, SecureChannel.ReadHeaderSize).ConfigureAwait(false); - } - } - Debug.Assert(_internalBufferCount >= SecureChannel.ReadHeaderSize); + buffer = buffer.Slice(processedLength); + } - // Parse the frame header to determine the payload size (which includes the header size). - int payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); - if (payloadBytes < 0) - { - throw new IOException(SR.net_frame_read_size); - } + if (_receivedEOF) + { + Debug.Assert(_internalBufferCount == 0); + // We received EOF during previous read but had buffered data to return. + return 0; + } - // Read in the rest of the payload if we don't have it. - if (_internalBufferCount < payloadBytes) - { - await FillBufferAsync(adapter, payloadBytes).ConfigureAwait(false); - } + if (buffer.Length == 0 && _internalBuffer is null) + { + // User requested a zero-byte read, and we have no data available in the buffer for processing. + // This zero-byte read indicates their desire to trade off the extra cost of a zero-byte read + // for reduced memory consumption when data is not immediately available. + // So, we will issue our own zero-byte read against the underlying stream and defer buffer allocation + // until data is actually available from the underlying stream. + // Note that if the underlying stream does not supporting blocking on zero byte reads, then this will + // complete immediately and won't save any memory, but will still function correctly. + await adapter.ReadAsync(Memory.Empty).ConfigureAwait(false); + } - // Set _decrytpedBytesOffset/Count to the current frame we have (including header) - // DecryptData will decrypt in-place and modify these to point to the actual decrypted data, which may be smaller. - _decryptedBytesOffset = _internalOffset; - _decryptedBytesCount = payloadBytes; + Debug.Assert(_decryptedBytesCount == 0); + Debug.Assert(_decryptedBytesOffset == 0); - SecurityStatusPal status; - lock (_handshakeLock) + while (true) + { + payloadBytes = await EnsureFullTlsFrameAsync(adapter).ConfigureAwait(false); + if (payloadBytes == 0) { - status = DecryptData(); - if (status.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) - { - // The status indicates that peer wants to renegotiate. (Windows only) - // In practice, there can be some other reasons too - like TLS1.3 session creation - // of alert handling. We need to pass the data to lsass and it is not safe to do parallel - // write any more as that can change TLS state and the EncryptData() can fail in strange ways. - - // To handle this we call DecryptData() under lock and we create TCS waiter. - // EncryptData() checks that under same lock and if it exist it will not call low-level crypto. - // Instead it will wait synchronously or asynchronously and it will try again after the wait. - // The result will be set when ReplyOnReAuthenticationAsync() is finished e.g. lsass business is over. - // If that happen before EncryptData() runs, _handshakeWaiter will be set to null - // and EncryptData() will work normally e.g. no waiting, just exclusion with DecryptData() - - if (_sslAuthenticationOptions!.AllowRenegotiation || SslProtocol == SslProtocols.Tls13 || _nestedAuth != 0) - { - // create TCS only if we plan to proceed. If not, we will throw in block bellow outside of the lock. - // Tls1.3 does not have renegotiation. However on Windows this error code is used - // for session management e.g. anything lsass needs to see. - // We also allow it when explicitly requested using RenegotiateAsync(). - _handshakeWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - } - } + _receivedEOF = true; + break; } - // Treat the bytes we just decrypted as consumed - // Note, we won't do another buffer read until the decrypted bytes are processed - ConsumeBufferedBytes(payloadBytes); - + SecurityStatusPal status = DecryptData(payloadBytes); if (status.ErrorCode != SecurityStatusPalErrorCode.OK) { byte[]? extraBuffer = null; @@ -962,12 +1017,50 @@ namespace System.Net.Security if (status.ErrorCode == SecurityStatusPalErrorCode.ContextExpired) { - return 0; + _receivedEOF = true; + break; } throw new IOException(SR.net_io_decrypt, SslStreamPal.GetException(status)); } + + if (_decryptedBytesCount > 0) + { + // This will either copy data from rented buffer or adjust final buffer as needed. + // In both cases _decryptedBytesOffset and _decryptedBytesCount will be updated as needed. + int copyLength = CopyDecryptedData(buffer); + processedLength += copyLength; + if (copyLength == buffer.Length) + { + // We have more decrypted data after we filled provided buffer. + break; + } + + buffer = buffer.Slice(copyLength); + } + + if (processedLength == 0) + { + // We did not get any real data so far. + continue; + } + + if (!HaveFullTlsFrame(out payloadBytes)) + { + // We don't have another frame to process but we have some data to return to caller. + break; + } + + TlsFrameHelper.TryGetFrameHeader(_internalBuffer.AsSpan(_internalOffset), ref _lastFrame.Header); + if (_lastFrame.Header.Type != TlsContentType.AppData) + { + // Alerts, handshake and anything else will be processed separately. + // This may not be necessary but it improves compatibility with older versions. + break; + } } + + return processedLength; } catch (Exception e) { @@ -1101,6 +1194,10 @@ namespace System.Net.Security _internalOffset += byteCount; _internalBufferCount -= byteCount; + if (_internalBufferCount == 0) + { + _internalOffset = 0; + } } private int CopyDecryptedData(Memory buffer) @@ -1116,6 +1213,11 @@ namespace System.Net.Security _decryptedBytesCount -= copyBytes; } + if (_decryptedBytesCount == 0) + { + _decryptedBytesOffset = 0; + } + return copyBytes; } @@ -1333,7 +1435,7 @@ namespace System.Net.Security payloadSize = ((buffer[3] << 8) | buffer[4]) + 5; break; default: - break; + throw new IOException(SR.net_frame_read_size); } return payloadSize; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs index 24f1e8e..27c854c 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Android.cs @@ -112,17 +112,20 @@ namespace System.Net.Security public static SecurityStatusPal DecryptMessage( SafeDeleteSslContext securityContext, - byte[] buffer, - ref int offset, - ref int count) + Span buffer, + out int offset, + out int count) { + offset = 0; + count = 0; + try { SafeSslHandle sslHandle = securityContext.SslContext; - securityContext.Write(buffer.AsSpan(offset, count)); + securityContext.Write(buffer); - PAL_SSLStreamStatus ret = Interop.AndroidCrypto.SSLStreamRead(sslHandle, buffer.AsSpan(offset, count), out int read); + PAL_SSLStreamStatus ret = Interop.AndroidCrypto.SSLStreamRead(sslHandle, buffer, out int read); if (ret == PAL_SSLStreamStatus.Error) return new SecurityStatusPal(SecurityStatusPalErrorCode.InternalError); diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs index 53c4d12..0414082 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs @@ -143,22 +143,23 @@ namespace System.Net.Security public static SecurityStatusPal DecryptMessage( SafeDeleteSslContext securityContext, - byte[] buffer, - ref int offset, - ref int count) + Span buffer, + out int offset, + out int count) { + offset = 0; + count = 0; + try { SafeSslHandle sslHandle = securityContext.SslContext; - - securityContext.Write(buffer.AsSpan(offset, count)); + securityContext.Write(buffer); unsafe { - fixed (byte* offsetInput = &buffer[offset]) + fixed (byte* ptr = buffer) { - PAL_TlsIo status = Interop.AppleCrypto.SslRead(sslHandle, offsetInput, count, out int written); - + PAL_TlsIo status = Interop.AppleCrypto.SslRead(sslHandle, ptr, buffer.Length, out int written); if (status < 0) { return new SecurityStatusPal( @@ -167,6 +168,7 @@ namespace System.Net.Security } count = written; + offset = 0; switch (status) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index 98a21e1..0204561 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs @@ -62,11 +62,14 @@ namespace System.Net.Security } } - public static SecurityStatusPal DecryptMessage(SafeDeleteSslContext securityContext, byte[] buffer, ref int offset, ref int count) + public static SecurityStatusPal DecryptMessage(SafeDeleteSslContext securityContext, Span buffer, out int offset, out int count) { + offset = 0; + count = 0; + try { - int resultSize = Interop.OpenSsl.Decrypt(securityContext.SslContext, new Span(buffer, offset, count), out Interop.Ssl.SslErrorCode errorCode); + int resultSize = Interop.OpenSsl.Decrypt(securityContext.SslContext, buffer, out Interop.Ssl.SslErrorCode errorCode); SecurityStatusPal retVal = MapNativeErrorCode(errorCode); diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs index 59a781b..4060296 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs @@ -313,7 +313,7 @@ namespace System.Net.Security } } - public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, byte[] buffer, ref int offset, ref int count) + public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, Span buffer, out int offset, out int count) { const int NumSecBuffers = 4; // data + empty + empty + empty fixed (byte* bufferPtr = buffer) @@ -321,8 +321,8 @@ namespace System.Net.Security Interop.SspiCli.SecBuffer* unmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[NumSecBuffers]; Interop.SspiCli.SecBuffer* dataBuffer = &unmanagedBuffer[0]; dataBuffer->BufferType = SecurityBufferType.SECBUFFER_DATA; - dataBuffer->pvBuffer = (IntPtr)bufferPtr + offset; - dataBuffer->cbBuffer = count; + dataBuffer->pvBuffer = (IntPtr)bufferPtr; + dataBuffer->cbBuffer = buffer.Length; for (int i = 1; i < NumSecBuffers; i++) { @@ -341,6 +341,7 @@ namespace System.Net.Security // Decrypt may repopulate the sec buffers, likely with header + data + trailer + empty. // We need to find the data. count = 0; + offset = 0; for (int i = 0; i < NumSecBuffers; i++) { // Successfully decoded data and placed it at the following position in the buffer, @@ -351,6 +352,7 @@ namespace System.Net.Security offset = (int)((byte*)unmanagedBuffer[i].pvBuffer - bufferPtr); count = unmanagedBuffer[i].cbBuffer; + // output is ignored on Windows. We always decrypt in place and we set outputOffset to indicate where the data start. Debug.Assert(offset >= 0 && count >= 0, $"Expected offset and count greater than 0, got {offset} and {count}"); Debug.Assert(checked(offset + count) <= buffer.Length, $"Expected offset+count <= buffer.Length, got {offset}+{count}>={buffer.Length}"); diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs b/src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs index 8104387..8f5b5ed 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs @@ -90,6 +90,11 @@ namespace System.Net.Security public int Length; public override string ToString() => $"{Version}:{Type}[{Length}]"; + + public int GetFrameSize() + { + return Length + TlsFrameHelper.HeaderSize; + } } internal static class TlsFrameHelper diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSchSendAuxRecordTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSchSendAuxRecordTest.cs deleted file mode 100644 index 2389188..0000000 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamSchSendAuxRecordTest.cs +++ /dev/null @@ -1,125 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Net.Test.Common; -using System.Security.Authentication; -using System.Threading.Tasks; - -using Xunit; -using Xunit.Abstractions; - -namespace System.Net.Security.Tests -{ - using Configuration = System.Net.Test.Common.Configuration; - - public class SchSendAuxRecordTest - { - readonly ITestOutputHelper _output; - - public SchSendAuxRecordTest(ITestOutputHelper output) - { - _output = output; - } - - [Fact] - [PlatformSpecific(TestPlatforms.Windows)] - public async Task SslStream_ClientAndServerUsesAuxRecord_Ok() - { - using (var server = new HttpsTestServer()) - { - server.Start(); - - var tasks = new Task[2]; - - bool serverAuxRecordDetected = false; - bool serverAuxRecordDetectedInconclusive = false; - int serverTotalBytesReceived = 0; - int serverChunks = 0; - - tasks[0] = server.AcceptHttpsClientAsync((requestString) => - { - serverTotalBytesReceived += requestString.Length; - - if (serverTotalBytesReceived == 1 && serverChunks == 0) - { - serverAuxRecordDetected = true; - } - - serverChunks++; - - // Test is inconclusive if any non-CBC cipher is used: - if (server.Stream.CipherAlgorithm == CipherAlgorithmType.None || - server.Stream.CipherAlgorithm == CipherAlgorithmType.Null || - server.Stream.CipherAlgorithm == CipherAlgorithmType.Rc4) - { - serverAuxRecordDetectedInconclusive = true; - } - - if (serverTotalBytesReceived < 5) - { - return Task.FromResult(null); - } - else - { - return Task.FromResult(HttpsTestServer.Options.DefaultResponseString); - } - }); - - - var clientOptions = new HttpsTestClient.Options(new IPEndPoint(IPAddress.Loopback, server.Port)); - clientOptions.AllowedProtocols = SslProtocols.Tls; - - clientOptions.IgnoreSslPolicyErrors = - SslPolicyErrors.RemoteCertificateNameMismatch | SslPolicyErrors.RemoteCertificateChainErrors; - - var client = new HttpsTestClient(clientOptions); - - bool clientAuxRecordDetected = false; - bool clientAuxRecordDetectedInconclusive = false; - int clientTotalBytesReceived = 0; - int clientChunks = 0; - - tasks[1] = client.HttpsRequestAsync((responseString) => - { - if (responseString == null) - { - string requestString = string.Format( - HttpsTestClient.Options.DefaultRequestStringTemplate, - clientOptions.ServerName); - - return Task.FromResult(requestString); - } - - clientTotalBytesReceived += responseString.Length; - - if (clientTotalBytesReceived == 1 && clientChunks == 0) - { - clientAuxRecordDetected = true; - } - - // Test is inconclusive if any non-CBC cipher is used: - if (client.Stream.CipherAlgorithm == CipherAlgorithmType.None || - client.Stream.CipherAlgorithm == CipherAlgorithmType.Null || - client.Stream.CipherAlgorithm == CipherAlgorithmType.Rc4) - { - clientAuxRecordDetectedInconclusive = true; - } - - return Task.FromResult(null); - }); - - await Task.WhenAll(tasks).WaitAsync(TestConfiguration.PassingTestTimeout); - - if (serverAuxRecordDetectedInconclusive || clientAuxRecordDetectedInconclusive) - { - _output.WriteLine("Test inconclusive: The Operating system preferred a non-CBC or Null cipher."); - } - else - { - Assert.True(serverAuxRecordDetected, "Server reports: Client auxiliary record not detected."); - Assert.True(clientAuxRecordDetected, "Client reports: Server auxiliary record not detected."); - } - } - } - } -} diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj b/src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj index bb13140..3dc537a 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/System.Net.Security.Tests.csproj @@ -100,7 +100,6 @@ - -- 2.7.4