From eca0f25b3f48f3f6fd2c93838e99786225909154 Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 31 Mar 2021 17:19:23 -0700 Subject: [PATCH 01/12] process more TLS frames when available --- .../Interop.OpenSsl.cs | 61 ++++----- .../Net/Http/SchSendAuxRecordHttpTest.cs | 127 ------------------ .../FunctionalTests/PlatformHandlerTest.cs | 12 -- .../FunctionalTests/SocketsHttpHandlerTest.cs | 26 ---- .../System.Net.Http.Functional.Tests.csproj | 2 - .../src/System/Net/Logging/NetEventSource.cs | 2 +- .../src/System/Net/Security/SecureChannel.cs | 20 +-- .../Net/Security/SslStreamPal.Android.cs | 3 +- .../System/Net/Security/SslStreamPal.OSX.cs | 17 +-- .../System/Net/Security/SslStreamPal.Unix.cs | 17 +-- .../Net/Security/SslStreamPal.Windows.cs | 20 +-- .../src/System/Net/Security/TlsFrameHelper.cs | 5 + .../SslStreamSchSendAuxRecordTest.cs | 125 ----------------- .../System.Net.Security.Tests.csproj | 1 - 14 files changed, 64 insertions(+), 374 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/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 71ee76ad2a6d0..a0f79b95e5a18 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 @@ -348,7 +345,7 @@ internal static int Encrypt(SafeSslHandle context, ReadOnlySpan input, ref return retVal; } - internal static int Decrypt(SafeSslHandle context, byte[] outBuffer, int offset, int count, out Ssl.SslErrorCode errorCode) + internal static int Decrypt(SafeSslHandle context, ReadOnlySpan input, Span output, out Ssl.SslErrorCode errorCode) { #if DEBUG ulong assertNoError = Crypto.ErrPeekError(); @@ -356,50 +353,43 @@ internal static int Decrypt(SafeSslHandle context, byte[] outBuffer, int offset, #endif errorCode = Ssl.SslErrorCode.SSL_ERROR_NONE; - int retVal = BioWrite(context.InputBio!, outBuffer, offset, count); + int retVal = BioWrite(context.InputBio!, input); Exception? innerError = null; - if (retVal == count) + if (retVal == input.Length) { unsafe { - fixed (byte* fixedBuffer = outBuffer) + fixed (byte* fixedBuffer = output) { - retVal = Ssl.SslRead(context, fixedBuffer + offset, outBuffer.Length); + retVal = Ssl.SslRead(context, fixedBuffer, output.Length); } } if (retVal > 0) { - count = retVal; + return retVal; } } - if (retVal != count) - { - errorCode = GetSslError(context, retVal, out innerError); - } + errorCode = GetSslError(context, retVal, out innerError); + retVal = 0; - if (retVal != count) + switch (errorCode) { - retVal = 0; - - switch (errorCode) - { - // indicate end-of-file - case Ssl.SslErrorCode.SSL_ERROR_ZERO_RETURN: - break; + // indicate end-of-file + case Ssl.SslErrorCode.SSL_ERROR_ZERO_RETURN: + break; - case Ssl.SslErrorCode.SSL_ERROR_WANT_READ: - // update error code to renegotiate if renegotiate is pending, otherwise make it SSL_ERROR_WANT_READ - errorCode = Ssl.IsSslRenegotiatePending(context) ? - Ssl.SslErrorCode.SSL_ERROR_RENEGOTIATE : - Ssl.SslErrorCode.SSL_ERROR_WANT_READ; - break; + case Ssl.SslErrorCode.SSL_ERROR_WANT_READ: + // update error code to renegotiate if renegotiate is pending, otherwise make it SSL_ERROR_WANT_READ + errorCode = Ssl.IsSslRenegotiatePending(context) ? + Ssl.SslErrorCode.SSL_ERROR_RENEGOTIATE : + Ssl.SslErrorCode.SSL_ERROR_WANT_READ; + break; - default: - throw new SslException(SR.Format(SR.net_ssl_decrypt_failed, errorCode), innerError); - } + default: + throw new SslException(SR.Format(SR.net_ssl_decrypt_failed, errorCode), innerError); } return retVal; @@ -507,23 +497,18 @@ private static int BioRead(SafeBioHandle bio, byte[] buffer, int count) return bytes; } - private static int BioWrite(SafeBioHandle bio, byte[] buffer, int offset, int count) + private static int BioWrite(SafeBioHandle bio, ReadOnlySpan buffer) { - Debug.Assert(buffer != null); - Debug.Assert(offset >= 0); - Debug.Assert(count >= 0); - Debug.Assert(buffer.Length >= offset + count); - int bytes; unsafe { fixed (byte* bufPtr = buffer) { - bytes = Ssl.BioWrite(bio, bufPtr + offset, count); + bytes = Ssl.BioWrite(bio, bufPtr, buffer.Length); } } - if (bytes != count) + if (bytes != buffer.Length) { throw CreateSslException(SR.net_ssl_write_bio_failed_error); } 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 0348db4d357ba..0000000000000 --- 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 6a53fbc9c0d42..cd7ec03a418bf 100644 --- a/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs +++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs @@ -134,11 +134,6 @@ public sealed class PlatformHandler_HttpClientHandler_Proxy_Test : HttpClientHan 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) { } @@ -302,13 +297,6 @@ public sealed class PlatformHandler_HttpClientHandler_Proxy_Http2_Test : HttpCli 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/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 7bf08ccfddf21..90f1ebcf59bc4 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -112,27 +112,6 @@ private sealed class SetOnFinalized public sealed class SocketsHttpHandler_HttpProtocolTests : HttpProtocolTests { public SocketsHttpHandler_HttpProtocolTests(ITestOutputHelper output) : base(output) { } - - [Fact] - public async Task DefaultRequestHeaders_SentUnparsed() - { - await LoopbackServer.CreateClientAndServerAsync(async uri => - { - using (HttpClient client = CreateHttpClient()) - { - client.DefaultRequestHeaders.TryAddWithoutValidation("Accept-Language", "en-US,en;q=0.5"); // validation would add spaces - client.DefaultRequestHeaders.TryAddWithoutValidation("From", "invalidemail"); // would fail to parse if validated - - var m = new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }; - (await client.SendAsync(TestAsync, m)).Dispose(); - } - }, async server => - { - List headers = await server.AcceptConnectionSendResponseAndCloseAsync(); - Assert.Contains(headers, header => header.Contains("Accept-Language: en-US,en;q=0.5")); - Assert.Contains(headers, header => header.Contains("From: invalidemail")); - }); - } } [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))] @@ -976,11 +955,6 @@ public async Task Http2GetAsync_TrailingHeaders_NoData_EmptyResponseObserved() } } - public sealed class SocketsHttpHandler_SchSendAuxRecordHttpTest : SchSendAuxRecordHttpTest - { - public SocketsHttpHandler_SchSendAuxRecordHttpTest(ITestOutputHelper output) : base(output) { } - } - [SkipOnMono("Tests hang with chrome. To be investigated", TestPlatforms.Browser)] public sealed class SocketsHttpHandler_HttpClientHandlerTest : HttpClientHandlerTest { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj index 96531b22b6aae..8244f7af631af 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj @@ -153,8 +153,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 63af2603db476..f71c6a9054478 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 @@ internal sealed partial class NetEventSource : EventSource /// 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 0e287e097c60d..5af516f726a02 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 @@ -870,7 +870,7 @@ is non-zero. --*/ 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; @@ -894,24 +894,12 @@ internal SecurityStatusPal Encrypt(ReadOnlyMemory buffer, ref byte[] outpu return secStatus; } - internal SecurityStatusPal Decrypt(byte[]? payload, ref int offset, ref int count) + internal SecurityStatusPal Decrypt(ReadOnlySpan input, Span output, ref int outputOffset, ref 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!, input, output, ref outputOffset, ref outputCount); if (NetEventSource.Log.IsEnabled() && status.ErrorCode == SecurityStatusPalErrorCode.OK) { - NetEventSource.DumpBuffer(this, payload!, offset, count); + NetEventSource.DumpBuffer(this, output.Slice(outputOffset, outputCount)); } return status; 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 6245f00ae4570..49d60503172d3 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 @@ -77,7 +77,8 @@ public static SecurityStatusPal EncryptMessage( public static SecurityStatusPal DecryptMessage( SafeDeleteContext securityContext, - byte[] buffer, + ReadOnlySpan input, + Span output, ref int offset, ref int count) { 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 5f1c1bb346676..e85ebb5526ed3 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 @@ -139,23 +139,23 @@ public static SecurityStatusPal EncryptMessage( public static SecurityStatusPal DecryptMessage( SafeDeleteContext securityContext, - byte[] buffer, - ref int offset, - ref int count) + ReadOnlySpan input, + Span output, + ref int outputOffset, + ref int outputCount) { try { SafeDeleteSslContext sslContext = (SafeDeleteSslContext)securityContext; SafeSslHandle sslHandle = sslContext.SslContext; - sslContext.Write(buffer.AsSpan(offset, count)); + sslContext.Write(input); unsafe { - fixed (byte* offsetInput = &buffer[offset]) + fixed (byte* ptr = output) { - PAL_TlsIo status = Interop.AppleCrypto.SslRead(sslHandle, offsetInput, count, out int written); - + PAL_TlsIo status = Interop.AppleCrypto.SslRead(sslHandle, ptr, output.Length, out int written); if (status < 0) { return new SecurityStatusPal( @@ -163,7 +163,8 @@ public static SecurityStatusPal DecryptMessage( Interop.AppleCrypto.CreateExceptionForOSStatus((int)status)); } - count = written; + outputCount = written; + outputOffset = 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 aeae1edc9fe94..648226017dbf6 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 @@ -5,7 +5,6 @@ using System.Diagnostics; using System.Security.Authentication; using System.Security.Authentication.ExtendedProtection; -using System.Security.Cryptography.X509Certificates; using Microsoft.Win32.SafeHandles; namespace System.Net.Security @@ -44,16 +43,18 @@ public static SafeFreeCredentials AcquireCredentialsHandle(SslStreamCertificateC public static SecurityStatusPal EncryptMessage(SafeDeleteContext securityContext, ReadOnlyMemory input, int headerSize, int trailerSize, ref byte[] output, out int resultSize) { - return EncryptDecryptHelper(securityContext, input, offset: 0, size: 0, encrypt: true, output: ref output, resultSize: out resultSize); + return EncryptDecryptHelper(securityContext, input.Span, Span.Empty, encrypt: true, newBuffer: ref output, resultSize: out resultSize); } - public static SecurityStatusPal DecryptMessage(SafeDeleteContext securityContext, byte[] buffer, ref int offset, ref int count) + public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, ReadOnlySpan input, Span output, ref int outputOffset, ref int outputCount) { - SecurityStatusPal retVal = EncryptDecryptHelper(securityContext, buffer, offset, count, false, ref buffer, out int resultSize); + byte[] buffer = Array.Empty(); + SecurityStatusPal retVal = EncryptDecryptHelper(securityContext!, input, output, encrypt: false, out int resultSize, ref buffer); if (retVal.ErrorCode == SecurityStatusPalErrorCode.OK || retVal.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) { - count = resultSize; + outputOffset = 0; + outputCount = resultSize; } return retVal; } @@ -150,7 +151,7 @@ private static SecurityStatusPal HandshakeInternal(SafeFreeCredentials credentia return Interop.Ssl.SslGetAlpnSelected(((SafeDeleteSslContext)context).SslContext); } - private static SecurityStatusPal EncryptDecryptHelper(SafeDeleteContext securityContext, ReadOnlyMemory input, int offset, int size, bool encrypt, ref byte[] output, out int resultSize) + private static SecurityStatusPal EncryptDecryptHelper(SafeDeleteContext securityContext, ReadOnlySpan input, Span output, bool encrypt, out int resultSize, ref byte[] newBuffer) { resultSize = 0; try @@ -160,11 +161,11 @@ private static SecurityStatusPal EncryptDecryptHelper(SafeDeleteContext security if (encrypt) { - resultSize = Interop.OpenSsl.Encrypt(scHandle, input.Span, ref output, out errorCode); + resultSize = Interop.OpenSsl.Encrypt(scHandle, input, ref newBuffer, out errorCode); } else { - resultSize = Interop.OpenSsl.Decrypt(scHandle, output, offset, size, out errorCode); + resultSize = Interop.OpenSsl.Decrypt(scHandle, input, output, out errorCode); } switch (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 b93e89957bc76..b3fe2f0456f02 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 @@ -39,6 +39,7 @@ public static Exception GetException(SecurityStatusPal status) internal const bool StartMutualAuthAsAnonymous = true; internal const bool CanEncryptEmptyMessage = true; + internal const bool DecryptsInPlace = true; public static void VerifyPackageInfo() { @@ -305,16 +306,16 @@ public static unsafe SecurityStatusPal EncryptMessage(SafeDeleteSslContext secur } } - public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, byte[] buffer, ref int offset, ref int count) + public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, ReadOnlySpan input, Span output, ref int outputOffset, ref int outputCount) { const int NumSecBuffers = 4; // data + empty + empty + empty - fixed (byte* bufferPtr = buffer) + fixed (byte* bufferPtr = input) { 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 = input.Length; for (int i = 1; i < NumSecBuffers; i++) { @@ -332,7 +333,7 @@ public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? secu // Decrypt may repopulate the sec buffers, likely with header + data + trailer + empty. // We need to find the data. - count = 0; + outputCount = 0; for (int i = 0; i < NumSecBuffers; i++) { // Successfully decoded data and placed it at the following position in the buffer, @@ -340,11 +341,12 @@ public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? secu // or we failed to decode the data, here is the encoded data. || (errorCode != Interop.SECURITY_STATUS.OK && unmanagedBuffer[i].BufferType == SecurityBufferType.SECBUFFER_EXTRA)) { - offset = (int)((byte*)unmanagedBuffer[i].pvBuffer - bufferPtr); - count = unmanagedBuffer[i].cbBuffer; + outputOffset = (int)((byte*)unmanagedBuffer[i].pvBuffer - bufferPtr); + outputCount = unmanagedBuffer[i].cbBuffer; - 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}"); + // output is ignored on Windows. We always decrypt in place and we set outputOffset to indicate where the data start. + Debug.Assert(outputOffset >= 0 && outputCount >= 0, $"Expected offset and count greater than 0, got {outputOffset} and {outputCount}"); + Debug.Assert(checked(outputOffset + outputCount) <= input.Length, $"Expected offset+count <= buffer.Length, got {outputOffset}+{outputCount}>={input.Length}"); break; } 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 8104387dc07ff..8f5b5ed2a07c5 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 @@ internal struct TlsFrameHeader 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 2389188bbef5e..0000000000000 --- 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 cf473c3ca94bc..e7b573d550444 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 @@ -98,7 +98,6 @@ - From a1954cc0e432a18ac01b25e1ddc531b5add55d18 Mon Sep 17 00:00:00 2001 From: wfurt Date: Thu, 1 Apr 2021 13:20:39 -0700 Subject: [PATCH 02/12] add SslStream.Implementation.cs --- .../Net/Security/SslStream.Implementation.cs | 151 ++++++++++++++---- 1 file changed, 117 insertions(+), 34 deletions(-) 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 0f7b5073c5a04..51f1403e896a1 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 @@ -41,6 +41,7 @@ private enum Framing 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 @@ -181,17 +182,6 @@ private SecurityStatusPal EncryptData(ReadOnlyMemory buffer, ref byte[] ou } } - 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. @@ -749,6 +739,10 @@ private void ReturnReadBufferIfEmpty() _decryptedBytesCount = 0; _decryptedBytesOffset = 0; } + else if (_decryptedBytesCount == 0) + { + _decryptedBytesOffset = 0; + } } private async ValueTask ReadAsyncInternal(TIOAdapter adapter, Memory buffer) @@ -759,17 +753,32 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, nameof(SslStream.ReadAsync), "read")); } + ThrowIfExceptionalOrNotAuthenticated(); + Debug.Assert(_internalBuffer is null || _internalBufferCount > 0 || _decryptedBytesCount > 0, "_internalBuffer allocated when no data is buffered."); try { - while (true) + + if (_decryptedBytesCount != 0) { - if (_decryptedBytesCount != 0) - { - return CopyDecryptedData(buffer); - } + int length = CopyDecryptedData(buffer); + ReturnReadBufferIfEmpty(); + return length; + } + if (receivedEOF) + { + // We received EOF during previous read but had buffered data to return. + return 0; + } + + Debug.Assert(_decryptedBytesOffset == 0); + Debug.Assert(_decryptedBytesCount == 0); + + int processedLength = 0; + while (true) + { if (buffer.Length == 0 && _internalBuffer is null) { // User requested a zero-byte read, and we have no data available in the buffer for processing. @@ -782,11 +791,25 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M await adapter.ReadAsync(Memory.Empty).ConfigureAwait(false); } - ResetReadBuffer(); - // Read the next frame header. - if (_internalBufferCount < SecureChannel.ReadHeaderSize) + int payloadBytes = int.MaxValue; + if (_internalBufferCount >= SecureChannel.ReadHeaderSize) { + //GetFrameSizepayloadBytes = GetFrameSize(_internalBuffer.Span.Slice(_internalOffset)); + payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + if (payloadBytes < 0) + { + throw new IOException(SR.net_frame_read_size); + } + } + + if (_internalBufferCount < payloadBytes) + { + // 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(); + // 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 @@ -795,7 +818,8 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M int readBytes = await adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)).ConfigureAwait(false); if (readBytes == 0) { - return 0; + receivedEOF = true; + break; } _internalBufferCount += readBytes; @@ -803,31 +827,44 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M { await FillBufferAsync(adapter, SecureChannel.ReadHeaderSize).ConfigureAwait(false); } - } - Debug.Assert(_internalBufferCount >= SecureChannel.ReadHeaderSize); - // 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 (payloadBytes == int.MaxValue) + { + // Parse the frame header to determine the payload size unless already done above. +// payloadBytes = GetFrameSize(_internalBuffer.Span.Slice(_internalOffset)); + payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + if (payloadBytes < 0) + { + throw new IOException(SR.net_frame_read_size); + } + } - // Read in the rest of the payload if we don't have it. - if (_internalBufferCount < payloadBytes) - { - await FillBufferAsync(adapter, payloadBytes).ConfigureAwait(false); + // Read in the rest of the payload if we don't have it. + if (_internalBufferCount < payloadBytes) + { + await FillBufferAsync(adapter, payloadBytes).ConfigureAwait(false); + } } + haveFullTlsFrame: // 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; + int decryptedCount = 0; + int decryptedOffset = 0; SecurityStatusPal status; lock (_handshakeLock) { - status = DecryptData(); + ThrowIfExceptionalOrNotAuthenticated(); + status = _context!.Decrypt(new ReadOnlySpan(_internalBuffer, _internalOffset, payloadBytes), _internalBuffer.AsSpan(_decryptedBytesOffset), ref decryptedOffset, ref decryptedCount); + _decryptedBytesCount = decryptedCount; + if (decryptedCount > 0) + { + _decryptedBytesOffset = _internalOffset + decryptedOffset; + } + if (status.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) { // The status indicates that peer wants to renegotiate. (Windows only) @@ -886,12 +923,54 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M if (status.ErrorCode == SecurityStatusPalErrorCode.ContextExpired) { - return 0; + receivedEOF = true; + break; } throw new IOException(SR.net_io_decrypt, SslStreamPal.GetException(status)); } + + if (_decryptedBytesCount == 0) + { + // This may be service frame to update keys or renegotiation. + // Keep waiting for real data. + if (processedLength > 0) + { + // We have some data. Hand them to caller before going for another iteration. + break; + } + + continue; + } + + // 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. + processedLength += CopyDecryptedData(buffer); + + if (_decryptedBytesCount > 0) + { + // We have more than enough to fill provided buffer. + break; + } + + // Check if we have enough data to process next TLS frame + if (_internalBufferCount > SecureChannel.ReadHeaderSize && _framing == Framing.SinceSSL3) + { + TlsFrameHelper.TryGetFrameHeader(_internalBuffer.AsSpan(_internalOffset), ref _lastFrame.Header); + payloadBytes = _lastFrame.Header.GetFrameSize(); + if (payloadBytes <= _internalBufferCount && _lastFrame.Header.Type == TlsContentType.AppData) + { + // skip over the already written decrypted data. + buffer = buffer.Slice(decryptedCount); + Debug.Assert(_decryptedBytesOffset <= _internalOffset); + goto haveFullTlsFrame; + } + } + + break; } + + return processedLength; } catch (Exception e) { @@ -1025,6 +1104,10 @@ private void ConsumeBufferedBytes(int byteCount) _internalOffset += byteCount; _internalBufferCount -= byteCount; + if (_internalBufferCount == 0) + { + _internalOffset = 0; + } } private int CopyDecryptedData(Memory buffer) From 8d191b64a900e1a31b021a333c05bd12618949ba Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 6 Apr 2021 14:45:11 -0700 Subject: [PATCH 03/12] remove extra comment --- .../src/System/Net/Security/SslStream.Implementation.cs | 1 - 1 file changed, 1 deletion(-) 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 51f1403e896a1..a29b62380ef0b 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 @@ -795,7 +795,6 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M int payloadBytes = int.MaxValue; if (_internalBufferCount >= SecureChannel.ReadHeaderSize) { - //GetFrameSizepayloadBytes = GetFrameSize(_internalBuffer.Span.Slice(_internalOffset)); payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); if (payloadBytes < 0) { From 3eb022ca06315afb8667d975885793be0be87f57 Mon Sep 17 00:00:00 2001 From: wfurt Date: Thu, 29 Apr 2021 00:06:17 -0700 Subject: [PATCH 04/12] add back DefaultRequestHeaders_SentUnparsed --- .../FunctionalTests/SocketsHttpHandlerTest.cs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 7a52a4192c82f..323a2d878806b 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -112,6 +112,27 @@ private sealed class SetOnFinalized public sealed class SocketsHttpHandler_HttpProtocolTests : HttpProtocolTests { public SocketsHttpHandler_HttpProtocolTests(ITestOutputHelper output) : base(output) { } + + [Fact] + public async Task DefaultRequestHeaders_SentUnparsed() + { + await LoopbackServer.CreateClientAndServerAsync(async uri => + { + using (HttpClient client = CreateHttpClient()) + { + client.DefaultRequestHeaders.TryAddWithoutValidation("Accept-Language", "en-US,en;q=0.5"); // validation would add spaces + client.DefaultRequestHeaders.TryAddWithoutValidation("From", "invalidemail"); // would fail to parse if validated + + var m = new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }; + (await client.SendAsync(TestAsync, m)).Dispose(); + } + }, async server => + { + List headers = await server.AcceptConnectionSendResponseAndCloseAsync(); + Assert.Contains(headers, header => header.Contains("Accept-Language: en-US,en;q=0.5")); + Assert.Contains(headers, header => header.Contains("From: invalidemail")); + }); + } } [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))] From cb216c83e65f57a73dc9d99be8d81633c923162d Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 30 Apr 2021 12:43:08 -0700 Subject: [PATCH 05/12] feedback from review --- .../Interop.Ssl.cs | 2 +- .../Net/Security/SslStream.Implementation.cs | 189 ++++++++++++------ .../Net/Security/SslStreamPal.Android.cs | 4 +- .../System/Net/Security/SslStreamPal.Unix.cs | 6 +- 4 files changed, 130 insertions(+), 71 deletions(-) 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 debe6ca458017..2357c276b8b95 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 @@ -162,7 +162,7 @@ private static unsafe extern PAL_SSLStreamStatus SSLStreamRead( 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/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index dd9b3e45f7137..de7aa31bc582a 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 @@ -747,6 +747,105 @@ private void ReturnReadBufferIfEmpty() } } + private bool haveFullTlsFrame(ref int frameSize) + { + if (_internalBufferCount < SecureChannel.ReadHeaderSize) + { + frameSize = int.MaxValue; + return false; + } + + frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + return _internalBufferCount >= frameSize; + } + + private ValueTask getFullFrameIfNeed(TIOAdapter adapter) + where TIOAdapter : IReadWriteAdapter + { + int frameSize; + if (_internalBufferCount >= SecureChannel.ReadHeaderSize) + { + frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + if (_internalBufferCount >= frameSize) + { + return new ValueTask(frameSize); + } + } + else + { + frameSize = int.MaxValue; + } + + // 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 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. + + ValueTask t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); + if (!t.IsCompletedSuccessfully) + { + return InternalGetFullFrameIfNeed(adapter, t, frameSize); + } + + int bytesRead = t.Result; + 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 new ValueTask(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 new ValueTask(frameSize); + + async ValueTask InternalGetFullFrameIfNeed(TIOAdapter adap, ValueTask t, int frameSize) + { + while (_internalBufferCount < frameSize) + { + int bytesRead = await t.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) + { + frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); + } + } + + return frameSize; + } + } + private async ValueTask ReadAsyncInternal(TIOAdapter adapter, Memory buffer) where TIOAdapter : IReadWriteAdapter { @@ -758,15 +857,22 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M ThrowIfExceptionalOrNotAuthenticated(); Debug.Assert(_internalBuffer is null || _internalBufferCount > 0 || _decryptedBytesCount > 0, "_internalBuffer allocated when no data is buffered."); + int processedLength = 0; + int payloadBytes = 0; try { if (_decryptedBytesCount != 0) { - int length = CopyDecryptedData(buffer); - ReturnReadBufferIfEmpty(); - return length; + processedLength = CopyDecryptedData(buffer); + if (_decryptedBytesCount != 0 || !haveFullTlsFrame(ref payloadBytes)) + { + // We either filled whole buffer or used all buffered frames. + return processedLength; + } + + buffer = buffer.Slice(processedLength); } if (receivedEOF) @@ -775,10 +881,9 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M return 0; } - Debug.Assert(_decryptedBytesOffset == 0); Debug.Assert(_decryptedBytesCount == 0); + Debug.Assert(_decryptedBytesOffset == 0); - int processedLength = 0; while (true) { if (buffer.Length == 0 && _internalBuffer is null) @@ -793,61 +898,13 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M await adapter.ReadAsync(Memory.Empty).ConfigureAwait(false); } - // Read the next frame header. - int payloadBytes = int.MaxValue; - if (_internalBufferCount >= SecureChannel.ReadHeaderSize) - { - payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); - if (payloadBytes < 0) - { - throw new IOException(SR.net_frame_read_size); - } - } - - if (_internalBufferCount < payloadBytes) + payloadBytes = await getFullFrameIfNeed(adapter).ConfigureAwait(false); + if (payloadBytes == 0) { - // 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(); - - // 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) - { - receivedEOF = true; - break; - } - - _internalBufferCount += readBytes; - if (_internalBufferCount < SecureChannel.ReadHeaderSize) - { - await FillBufferAsync(adapter, SecureChannel.ReadHeaderSize).ConfigureAwait(false); - } - - if (payloadBytes == int.MaxValue) - { - // Parse the frame header to determine the payload size unless already done above. -// payloadBytes = GetFrameSize(_internalBuffer.Span.Slice(_internalOffset)); - payloadBytes = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); - if (payloadBytes < 0) - { - throw new IOException(SR.net_frame_read_size); - } - } - - // Read in the rest of the payload if we don't have it. - if (_internalBufferCount < payloadBytes) - { - await FillBufferAsync(adapter, payloadBytes).ConfigureAwait(false); - } + receivedEOF = true; + break; } - haveFullTlsFrame: // 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; @@ -935,7 +992,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M { // This may be service frame to update keys or renegotiation. // Keep waiting for real data. - if (processedLength > 0) + if (processedLength > 0 && !haveFullTlsFrame(ref payloadBytes)) { // We have some data. Hand them to caller before going for another iteration. break; @@ -955,16 +1012,17 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M } // Check if we have enough data to process next TLS frame - if (_internalBufferCount > SecureChannel.ReadHeaderSize && _framing == Framing.SinceSSL3) + if (haveFullTlsFrame(ref payloadBytes)) { TlsFrameHelper.TryGetFrameHeader(_internalBuffer.AsSpan(_internalOffset), ref _lastFrame.Header); - payloadBytes = _lastFrame.Header.GetFrameSize(); - if (payloadBytes <= _internalBufferCount && _lastFrame.Header.Type == TlsContentType.AppData) + // Process another frame if possible. + // Alerts, handshake and anything else will be processed separately. + if (_lastFrame.Header.Type == TlsContentType.AppData) { // skip over the already written decrypted data. buffer = buffer.Slice(decryptedCount); Debug.Assert(_decryptedBytesOffset <= _internalOffset); - goto haveFullTlsFrame; + continue; } } @@ -1124,6 +1182,11 @@ private int CopyDecryptedData(Memory buffer) _decryptedBytesCount -= copyBytes; } + if (_decryptedBytesCount == 0) + { + _decryptedBytesOffset = 0; + } + return copyBytes; } @@ -1341,7 +1404,7 @@ private int GetFrameSize(ReadOnlySpan buffer) 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 5b569a606a23f..c9a14522f19fd 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 @@ -116,9 +116,9 @@ public static SecurityStatusPal DecryptMessage( { SafeSslHandle sslHandle = securityContext.SslContext; - securityContext.Write(buffer.AsSpan(offset, count)); + securityContext.Write(input); - PAL_SSLStreamStatus ret = Interop.AndroidCrypto.SSLStreamRead(sslHandle, buffer.AsSpan(offset, count), out int read); + PAL_SSLStreamStatus ret = Interop.AndroidCrypto.SSLStreamRead(sslHandle, input, 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.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index 7c4e8c7b2ade1..0a68c295a5bda 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 @@ -151,11 +151,7 @@ private static SecurityStatusPal HandshakeInternal(SafeFreeCredentials credentia return Interop.Ssl.SslGetAlpnSelected(context.SslContext); } -<<<<<<< HEAD - private static SecurityStatusPal EncryptDecryptHelper(SafeDeleteContext securityContext, ReadOnlySpan input, Span output, bool encrypt, out int resultSize, ref byte[] newBuffer) -======= - private static SecurityStatusPal EncryptDecryptHelper(SafeDeleteSslContext securityContext, ReadOnlyMemory input, int offset, int size, bool encrypt, ref byte[] output, out int resultSize) ->>>>>>> 1a18528d13228f0eef727ff49767726b2c8c50dd + private static SecurityStatusPal EncryptDecryptHelper(SafeDeleteSslContext securityContext, ReadOnlySpan input, Span output, bool encrypt, out int resultSize, ref byte[] newBuffer) { resultSize = 0; try From d88a170f4941fdafb6cf46faed5c7bcee93812cb Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 30 Apr 2021 14:42:22 -0700 Subject: [PATCH 06/12] fix winhttp --- .../System.Net.Http.WinHttpHandler.Functional.Tests.csproj | 2 -- 1 file changed, 2 deletions(-) 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 8b2cc572da15d..d697b60c779ef 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" /> - Date: Fri, 30 Apr 2021 16:38:46 -0700 Subject: [PATCH 07/12] fix linker test --- .../System/Net/Security/SslStream.Implementation.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 de7aa31bc582a..3b05b6071b01a 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 @@ -821,7 +821,7 @@ private ValueTask getFullFrameIfNeed(TIOAdapter adapter) async ValueTask InternalGetFullFrameIfNeed(TIOAdapter adap, ValueTask t, int frameSize) { - while (_internalBufferCount < frameSize) + while (true) { int bytesRead = await t.ConfigureAwait(false); if (bytesRead == 0) @@ -840,6 +840,14 @@ async ValueTask InternalGetFullFrameIfNeed(TIOAdapter adap, ValueTask { frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); } + + if (_internalBufferCount < frameSize) + { + t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); + continue; + } + + break; } return frameSize; From 92f2d2b0eaefdc5cc08734557973b94d89b48ba4 Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 7 May 2021 16:41:21 -0700 Subject: [PATCH 08/12] feedback from review --- .../src/System/Net/Security/SecureChannel.cs | 4 +- .../Net/Security/SslStream.Implementation.cs | 187 ++++++++---------- .../Net/Security/SslStreamPal.Android.cs | 7 +- .../System/Net/Security/SslStreamPal.OSX.cs | 8 +- .../System/Net/Security/SslStreamPal.Unix.cs | 6 +- .../Net/Security/SslStreamPal.Windows.cs | 3 +- 6 files changed, 105 insertions(+), 110 deletions(-) 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 5af516f726a02..3bc1dd9e981bc 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 @@ -894,9 +894,9 @@ internal SecurityStatusPal Encrypt(ReadOnlyMemory buffer, ref byte[] outpu return secStatus; } - internal SecurityStatusPal Decrypt(ReadOnlySpan input, Span output, ref int outputOffset, ref int outputCount) + internal SecurityStatusPal Decrypt(ReadOnlySpan input, Span output, out int outputOffset, out int outputCount) { - SecurityStatusPal status = SslStreamPal.DecryptMessage(_securityContext!, input, output, ref outputOffset, ref outputCount); + SecurityStatusPal status = SslStreamPal.DecryptMessage(_securityContext!, input, output, out outputOffset, out outputCount); if (NetEventSource.Log.IsEnabled() && status.ErrorCode == SecurityStatusPalErrorCode.OK) { NetEventSource.DumpBuffer(this, output.Slice(outputOffset, outputCount)); 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 3b05b6071b01a..6d0181cc38bdc 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 @@ -747,7 +747,7 @@ private void ReturnReadBufferIfEmpty() } } - private bool haveFullTlsFrame(ref int frameSize) + private bool HaveFullTlsFrame(out int frameSize) { if (_internalBufferCount < SecureChannel.ReadHeaderSize) { @@ -759,21 +759,13 @@ private bool haveFullTlsFrame(ref int frameSize) return _internalBufferCount >= frameSize; } - private ValueTask getFullFrameIfNeed(TIOAdapter adapter) + private ValueTask GetFullFrameIfNeed(TIOAdapter adapter) where TIOAdapter : IReadWriteAdapter { int frameSize; - if (_internalBufferCount >= SecureChannel.ReadHeaderSize) + if (HaveFullTlsFrame(out frameSize)) { - frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); - if (_internalBufferCount >= frameSize) - { - return new ValueTask(frameSize); - } - } - else - { - frameSize = int.MaxValue; + return new ValueTask(frameSize); } // We may have enough space to complete frame, but we may still do extra IO if the frame is small. @@ -784,8 +776,9 @@ private ValueTask getFullFrameIfNeed(TIOAdapter adapter) // _internalOffset is 0 after ResetReadBuffer and we use _internalBufferCount to determined where to read. while (_internalBufferCount < frameSize) { - // 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 + // We don't have enough bytes buffered so we need to read more. Same logic applies to finishing frame + // as well as cases when we don't have enough data to even determine the size. + // 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. @@ -854,6 +847,54 @@ async ValueTask InternalGetFullFrameIfNeed(TIOAdapter adap, ValueTask } } + private SecurityStatusPal DecryptData(int frameSize, out int decryptedCount, out int decryptedOffset) + { + // 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 = frameSize; + SecurityStatusPal status; + lock (_handshakeLock) + { + ThrowIfExceptionalOrNotAuthenticated(); + status = _context!.Decrypt(new ReadOnlySpan(_internalBuffer, _internalOffset, frameSize), _internalBuffer.AsSpan(_decryptedBytesOffset), out decryptedOffset, out 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) + { + // 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. + _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) where TIOAdapter : IReadWriteAdapter { @@ -870,11 +911,10 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M try { - if (_decryptedBytesCount != 0) { processedLength = CopyDecryptedData(buffer); - if (_decryptedBytesCount != 0 || !haveFullTlsFrame(ref payloadBytes)) + if (processedLength == buffer.Length || !HaveFullTlsFrame(out payloadBytes)) { // We either filled whole buffer or used all buffered frames. return processedLength; @@ -889,77 +929,31 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M return 0; } + 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); + } + Debug.Assert(_decryptedBytesCount == 0); Debug.Assert(_decryptedBytesOffset == 0); while (true) { - 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); - } - - payloadBytes = await getFullFrameIfNeed(adapter).ConfigureAwait(false); + payloadBytes = await GetFullFrameIfNeed(adapter).ConfigureAwait(false); if (payloadBytes == 0) { receivedEOF = true; break; } - // 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; - int decryptedCount = 0; - int decryptedOffset = 0; - - SecurityStatusPal status; - lock (_handshakeLock) - { - ThrowIfExceptionalOrNotAuthenticated(); - status = _context!.Decrypt(new ReadOnlySpan(_internalBuffer, _internalOffset, payloadBytes), _internalBuffer.AsSpan(_decryptedBytesOffset), ref decryptedOffset, ref 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) - { - // 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. - _handshakeWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - } - } - } - - // 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, out int decryptedCount, out int decryptedOffset); if (status.ErrorCode != SecurityStatusPalErrorCode.OK) { byte[]? extraBuffer = null; @@ -996,45 +990,38 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M throw new IOException(SR.net_io_decrypt, SslStreamPal.GetException(status)); } - if (_decryptedBytesCount == 0) + if (decryptedCount > 0) { - // This may be service frame to update keys or renegotiation. - // Keep waiting for real data. - if (processedLength > 0 && !haveFullTlsFrame(ref payloadBytes)) + // 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. + processedLength += CopyDecryptedData(buffer); + if (_decryptedBytesCount > 0) { - // We have some data. Hand them to caller before going for another iteration. + // We have more decrypted data after we filled provided buffer. break; } - continue; + buffer = buffer.Slice(decryptedCount); } - // 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. - processedLength += CopyDecryptedData(buffer); + if (processedLength == 0) + { + // We did not get any real data so far. + continue; + } - if (_decryptedBytesCount > 0) + if (!HaveFullTlsFrame(out payloadBytes)) { - // We have more than enough to fill provided buffer. + // We don't have anuther frame to process but we have some data to return to caller. break; } - // Check if we have enough data to process next TLS frame - if (haveFullTlsFrame(ref payloadBytes)) + TlsFrameHelper.TryGetFrameHeader(_internalBuffer.AsSpan(_internalOffset), ref _lastFrame.Header); + if (_lastFrame.Header.Type != TlsContentType.AppData) { - TlsFrameHelper.TryGetFrameHeader(_internalBuffer.AsSpan(_internalOffset), ref _lastFrame.Header); - // Process another frame if possible. // Alerts, handshake and anything else will be processed separately. - if (_lastFrame.Header.Type == TlsContentType.AppData) - { - // skip over the already written decrypted data. - buffer = buffer.Slice(decryptedCount); - Debug.Assert(_decryptedBytesOffset <= _internalOffset); - continue; - } + break; } - - break; } return processedLength; 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 c9a14522f19fd..16632237593c4 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 @@ -109,9 +109,12 @@ public static SecurityStatusPal DecryptMessage( SafeDeleteSslContext securityContext, ReadOnlySpan input, Span output, - ref int offset, - ref int count) + out int offset, + out int count) { + offset = 0; + count = 0; + try { SafeSslHandle sslHandle = securityContext.SslContext; 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 56db67f2efbc9..b8ac4c14eebde 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 @@ -140,13 +140,15 @@ public static SecurityStatusPal DecryptMessage( SafeDeleteSslContext securityContext, ReadOnlySpan input, Span output, - ref int outputOffset, - ref int outputCount) + out int outputOffset, + out int outputCount) { + outputOffset = 0; + outputCount = 0; + try { SafeSslHandle sslHandle = securityContext.SslContext; - securityContext.Write(input); unsafe 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 0a68c295a5bda..45944e65c98b9 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 @@ -46,14 +46,16 @@ public static SecurityStatusPal EncryptMessage(SafeDeleteSslContext securityCont return EncryptDecryptHelper(securityContext, input.Span, Span.Empty, encrypt: true, newBuffer: ref output, resultSize: out resultSize); } - public static SecurityStatusPal DecryptMessage(SafeDeleteSslContext securityContext, ReadOnlySpan input, Span output, ref int outputOffset, ref int outputCount) + public static SecurityStatusPal DecryptMessage(SafeDeleteSslContext securityContext, ReadOnlySpan input, Span output, out int outputOffset, out int outputCount) { byte[] buffer = Array.Empty(); + outputOffset = 0; + outputCount = 0; SecurityStatusPal retVal = EncryptDecryptHelper(securityContext, input, output, encrypt: false, out int resultSize, ref buffer); if (retVal.ErrorCode == SecurityStatusPalErrorCode.OK || retVal.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) { - outputOffset = 0; + // outputOffset = 0; outputCount = resultSize; } return retVal; 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 b3fe2f0456f02..9dc9edf474b05 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 @@ -306,7 +306,7 @@ public static unsafe SecurityStatusPal EncryptMessage(SafeDeleteSslContext secur } } - public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, ReadOnlySpan input, Span output, ref int outputOffset, ref int outputCount) + public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? securityContext, ReadOnlySpan input, Span output, out int outputOffset, out int outputCount) { const int NumSecBuffers = 4; // data + empty + empty + empty fixed (byte* bufferPtr = input) @@ -334,6 +334,7 @@ public static unsafe SecurityStatusPal DecryptMessage(SafeDeleteSslContext? secu // Decrypt may repopulate the sec buffers, likely with header + data + trailer + empty. // We need to find the data. outputCount = 0; + outputOffset = 0; for (int i = 0; i < NumSecBuffers; i++) { // Successfully decoded data and placed it at the following position in the buffer, From 2c32bfdd706afcec3c89760be96e33c2a1cf3d7c Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 14 Jun 2021 11:40:17 +0200 Subject: [PATCH 09/12] feedback from review --- .../Net/Security/SslStream.Implementation.cs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) 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 eea2f1844fe91..d9cae0c71fe00 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,7 +43,7 @@ private enum Framing 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; + private bool _receivedEOF; // Used by Telemetry to ensure we log connection close exactly once // 0 = no handshake @@ -823,7 +823,7 @@ private bool HaveFullTlsFrame(out int frameSize) } - private ValueTask GetFullFrameIfNeed(TIOAdapter adapter) + private ValueTask EnsureFullTlsFrame(TIOAdapter adapter) where TIOAdapter : IReadWriteAdapter { int frameSize; @@ -850,7 +850,7 @@ private ValueTask GetFullFrameIfNeed(TIOAdapter adapter) ValueTask t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); if (!t.IsCompletedSuccessfully) { - return InternalGetFullFrameIfNeed(adapter, t, frameSize); + return InternalEnsureFullTlsFrame(adapter, t, frameSize); } int bytesRead = t.Result; @@ -876,9 +876,9 @@ private ValueTask GetFullFrameIfNeed(TIOAdapter adapter) return new ValueTask(frameSize); - async ValueTask InternalGetFullFrameIfNeed(TIOAdapter adap, ValueTask t, int frameSize) + async ValueTask InternalEnsureFullTlsFrame(TIOAdapter adap, ValueTask t, int frameSize) { - while (true) + do { int bytesRead = await t.ConfigureAwait(false); if (bytesRead == 0) @@ -898,14 +898,14 @@ async ValueTask InternalGetFullFrameIfNeed(TIOAdapter adap, ValueTask frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); } - if (_internalBufferCount < frameSize) + if (_internalBufferCount >= frameSize) { - t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); - continue; + break; } - break; + t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); } + while (_internalBufferCount < frameSize); return frameSize; } @@ -997,7 +997,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M buffer = buffer.Slice(processedLength); } - if (receivedEOF && _internalBufferCount == 0) + if (_receivedEOF && _internalBufferCount == 0) { // We received EOF during previous read but had buffered data to return. return 0; @@ -1020,10 +1020,10 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M while (true) { - payloadBytes = await GetFullFrameIfNeed(adapter).ConfigureAwait(false); + payloadBytes = await EnsureFullTlsFrame(adapter).ConfigureAwait(false); if (payloadBytes == 0) { - receivedEOF = true; + _receivedEOF = true; break; } @@ -1061,7 +1061,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M if (status.ErrorCode == SecurityStatusPalErrorCode.ContextExpired) { - receivedEOF = true; + _receivedEOF = true; break; } From 65ac2d13678ed60900a8724f67ea6cdc4df081be Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 14 Jun 2021 14:16:04 +0200 Subject: [PATCH 10/12] feedback from review --- .../Net/Security/SslStream.Implementation.cs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) 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 d9cae0c71fe00..c411cd2256e9a 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 @@ -912,17 +912,20 @@ async ValueTask InternalEnsureFullTlsFrame(TIOAdapter adap, ValueTask } - private SecurityStatusPal DecryptData(int frameSize, out int decryptedCount, out int decryptedOffset) + private SecurityStatusPal DecryptData(int frameSize) { + Debug.Assert(_decryptedBytesCount == 0); + // 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 = frameSize; SecurityStatusPal status; + lock (_handshakeLock) { ThrowIfExceptionalOrNotAuthenticated(); - status = _context!.Decrypt(new Span(_internalBuffer, _internalOffset, frameSize), out decryptedOffset, out decryptedCount); + status = _context!.Decrypt(new Span(_internalBuffer, _internalOffset, frameSize), out int decryptedOffset, out int decryptedCount); _decryptedBytesCount = decryptedCount; if (decryptedCount > 0) { @@ -946,7 +949,7 @@ private SecurityStatusPal DecryptData(int frameSize, out int decryptedCount, out 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. + // 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(). @@ -1027,7 +1030,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M break; } - SecurityStatusPal status = DecryptData(payloadBytes, out int decryptedCount, out int decryptedOffset); + SecurityStatusPal status = DecryptData(payloadBytes); if (status.ErrorCode != SecurityStatusPalErrorCode.OK) { byte[]? extraBuffer = null; @@ -1068,18 +1071,19 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M throw new IOException(SR.net_io_decrypt, SslStreamPal.GetException(status)); } - if (decryptedCount > 0) + 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. - processedLength += CopyDecryptedData(buffer); - if (_decryptedBytesCount > 0) + int copyLength = CopyDecryptedData(buffer); + processedLength += copyLength; + if (copyLength == buffer.Length) { // We have more decrypted data after we filled provided buffer. break; } - buffer = buffer.Slice(decryptedCount); + buffer = buffer.Slice(copyLength); } if (processedLength == 0) From c98dba8841d17a4bbe09f9e5ccddc6d4502f9ccd Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 14 Jun 2021 16:09:34 +0200 Subject: [PATCH 11/12] make EnsureFullTlsFrame async --- .../Net/Security/SslStream.Implementation.cs | 54 +++---------------- 1 file changed, 6 insertions(+), 48 deletions(-) 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 c411cd2256e9a..0b77562862e4d 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 @@ -823,13 +823,13 @@ private bool HaveFullTlsFrame(out int frameSize) } - private ValueTask EnsureFullTlsFrame(TIOAdapter adapter) + private async ValueTask EnsureFullTlsFrameAsync(TIOAdapter adapter) where TIOAdapter : IReadWriteAdapter { int frameSize; if (HaveFullTlsFrame(out frameSize)) { - return new ValueTask(frameSize); + return frameSize; } // We may have enough space to complete frame, but we may still do extra IO if the frame is small. @@ -847,13 +847,7 @@ private ValueTask EnsureFullTlsFrame(TIOAdapter adapter) // 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. - ValueTask t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); - if (!t.IsCompletedSuccessfully) - { - return InternalEnsureFullTlsFrame(adapter, t, frameSize); - } - - int bytesRead = t.Result; + int bytesRead = await adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)).ConfigureAwait(false); if (bytesRead == 0) { if (_internalBufferCount != 0) @@ -862,11 +856,10 @@ private ValueTask EnsureFullTlsFrame(TIOAdapter adapter) throw new IOException(SR.net_io_eof); } - return new ValueTask(0); + return 0; } _internalBufferCount += bytesRead; - if (frameSize == int.MaxValue && _internalBufferCount > SecureChannel.ReadHeaderSize) { // recalculate frame size if needed e.g. we could not get it before. @@ -874,44 +867,9 @@ private ValueTask EnsureFullTlsFrame(TIOAdapter adapter) } } - return new ValueTask(frameSize); - - async ValueTask InternalEnsureFullTlsFrame(TIOAdapter adap, ValueTask t, int frameSize) - { - do - { - int bytesRead = await t.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) - { - frameSize = GetFrameSize(_internalBuffer.AsSpan(_internalOffset)); - } - - if (_internalBufferCount >= frameSize) - { - break; - } - - t = adapter.ReadAsync(_internalBuffer.AsMemory(_internalBufferCount)); - } - while (_internalBufferCount < frameSize); - - return frameSize; - } + return frameSize; } - private SecurityStatusPal DecryptData(int frameSize) { Debug.Assert(_decryptedBytesCount == 0); @@ -1023,7 +981,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M while (true) { - payloadBytes = await EnsureFullTlsFrame(adapter).ConfigureAwait(false); + payloadBytes = await EnsureFullTlsFrameAsync(adapter).ConfigureAwait(false); if (payloadBytes == 0) { _receivedEOF = true; From ac9d288b1010aa253108dad42b36538f2d9fd64e Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 23 Jun 2021 13:05:32 +0200 Subject: [PATCH 12/12] feedback from review --- .../Net/Security/SslStream.Implementation.cs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) 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 0b77562862e4d..17858bb0e68b6 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 @@ -840,13 +840,7 @@ private async ValueTask EnsureFullTlsFrameAsync(TIOAdapter adap // _internalOffset is 0 after ResetReadBuffer and we use _internalBufferCount to determined where to read. while (_internalBufferCount < frameSize) { - // We don't have enough bytes buffered so we need to read more. Same logic applies to finishing frame - // as well as cases when we don't have enough data to even determine the size. - // 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. - + // 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) { @@ -874,7 +868,7 @@ private SecurityStatusPal DecryptData(int frameSize) { Debug.Assert(_decryptedBytesCount == 0); - // Set _decrytpedBytesOffset/Count to the current frame we have (including header) + // 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; @@ -958,8 +952,9 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M buffer = buffer.Slice(processedLength); } - if (_receivedEOF && _internalBufferCount == 0) + if (_receivedEOF) { + Debug.Assert(_internalBufferCount == 0); // We received EOF during previous read but had buffered data to return. return 0; } @@ -1052,7 +1047,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M if (!HaveFullTlsFrame(out payloadBytes)) { - // We don't have anuther frame to process but we have some data to return to caller. + // We don't have another frame to process but we have some data to return to caller. break; } @@ -1060,6 +1055,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M 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; } }