From a0bc90f309e833b711dca1e6a8698f455e6424ec Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 9 Jul 2024 15:57:21 +0200 Subject: [PATCH 1/5] Add debug info --- .../src/System.Net.Security.csproj | 2 +- .../src/System/Net/Security/SslStream.IO.cs | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj index 2e263173ad81de..61521b58c6f912 100644 --- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj +++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj @@ -455,7 +455,7 @@ - + diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs index a29fbfa920062e..c5560b491f4906 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs @@ -461,6 +461,7 @@ private async ValueTask ReceiveHandshakeFrameAsync(Cancellation private ProtocolToken ProcessTlsFrame(int frameSize) { int chunkSize = frameSize; + int initFrameSize = frameSize; ReadOnlySpan availableData = _buffer.EncryptedReadOnlySpan; @@ -487,9 +488,20 @@ private ProtocolToken ProcessTlsFrame(int frameSize) chunkSize += frameSize; } - ProtocolToken token = NextMessage(availableData.Slice(0, chunkSize), out int consumed); - _buffer.DiscardEncrypted(consumed); - return token; + try + { + ProtocolToken token = NextMessage(availableData.Slice(0, chunkSize), out int consumed); + _buffer.DiscardEncrypted(consumed); + return token; + } + catch (ArgumentOutOfRangeException) + { + System.Console.WriteLine($"FrameSize: {initFrameSize}, {frameSize}"); + System.Console.WriteLine($"AvailableData: {availableData.Length}, ChunkSize: {chunkSize}"); + System.Console.WriteLine($"Disposed: {ReferenceEquals(_exception, s_disposedSentinel)}"); + + throw; + } } // From 3d7fd837724fca734d547cf541641e0601766045 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 10 Jul 2024 18:11:27 +0200 Subject: [PATCH 2/5] Fix partitioning of incoming TLS frames --- .../src/System/Net/Security/SslStream.IO.cs | 4 ++-- .../FunctionalTests/SslStreamFramingTest.cs | 24 ++++++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs index c5560b491f4906..5b8c94f0067a62 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs @@ -470,7 +470,7 @@ private ProtocolToken ProcessTlsFrame(int frameSize) { TlsFrameHeader nextHeader = default; - if (!TlsFrameHelper.TryGetFrameHeader(_buffer.EncryptedReadOnlySpan.Slice(chunkSize), ref nextHeader)) + if (!TlsFrameHelper.TryGetFrameHeader(availableData.Slice(chunkSize), ref nextHeader)) { break; } @@ -479,7 +479,7 @@ private ProtocolToken ProcessTlsFrame(int frameSize) // Can process more handshake frames in single step or during TLS1.3 post-handshake auth, but we should // avoid processing too much so as to preserve API boundary between handshake and I/O. - if ((nextHeader.Type != TlsContentType.Handshake && nextHeader.Type != TlsContentType.ChangeCipherSpec) && !_isRenego || frameSize > _buffer.EncryptedLength) + if ((nextHeader.Type != TlsContentType.Handshake && nextHeader.Type != TlsContentType.ChangeCipherSpec) && !_isRenego || frameSize > availableData.Length - chunkSize) { // We don't have full frame left or we already have app data which needs to be processed by decrypt. break; diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs index efc405da00eb7c..1328460f751767 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs @@ -39,6 +39,9 @@ public enum FramingType // 1 byte reads ByteByByte, + // Receive at most 257 B in one read, targets specifically receiving + Chunked, + // coalesce reads to biggest chunks possible Coalescing } @@ -75,6 +78,7 @@ public static TheoryData Handshak [Theory] [MemberData(nameof(HandshakeScenarioData))] + // [InlineData(FramingType.Prime257, SslProtocols.Tls12, ClientCertScenario.None)] public async Task Handshake_Success(FramingType framingType, SslProtocols sslProtocol, ClientCertScenario clientCertScenario) { (Stream stream1, Stream stream2) = TestHelper.GetConnectedStreams(); @@ -129,7 +133,6 @@ public async Task Handshake_Success(FramingType framingType, SslProtocols sslPro Assert.True(serverStream.ReadCalled, "Mocked read method was not used"); await TestHelper.PingPong(client, server); - } internal class ConfigurableReadStream : Stream @@ -168,6 +171,7 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation { case FramingType.ByteByByte: return await _stream.ReadAsync(buffer.Length > 0 ? buffer.Slice(0, 1) : buffer, cancellationToken); + case FramingType.Coalescing: { if (buffer.Length > 0) @@ -178,6 +182,24 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation } return await _stream.ReadAsync(buffer, cancellationToken); } + case FramingType.Chunked: + { + if (buffer.Length > 0) + { + // wait 10ms, this should be enough for the other side to write as much data + // as it will ever write before receiving something back. + await Task.Delay(10); + + const int maxRead = 1519; // arbitrarily chosen chunk size + + if (buffer.Length > maxRead) + { + buffer = buffer.Slice(0, maxRead); + } + } + return await _stream.ReadAsync(buffer, cancellationToken); + } + default: throw new NotImplementedException(); } From ee965c14f23658fc0e48c66ae9144e9e817023c5 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 10 Jul 2024 18:12:18 +0200 Subject: [PATCH 3/5] Revert unwanted change --- .../System.Net.Security/src/System.Net.Security.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj index 61521b58c6f912..2e263173ad81de 100644 --- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj +++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj @@ -455,7 +455,7 @@ - + From ba1cea1ce1b723dc372427ea63f4246c754d79da Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 10 Jul 2024 18:13:03 +0200 Subject: [PATCH 4/5] Revert unwanted changes --- .../src/System/Net/Security/SslStream.IO.cs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs index 5b8c94f0067a62..4b876aee9e8ba7 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs @@ -461,7 +461,6 @@ private async ValueTask ReceiveHandshakeFrameAsync(Cancellation private ProtocolToken ProcessTlsFrame(int frameSize) { int chunkSize = frameSize; - int initFrameSize = frameSize; ReadOnlySpan availableData = _buffer.EncryptedReadOnlySpan; @@ -488,20 +487,9 @@ private ProtocolToken ProcessTlsFrame(int frameSize) chunkSize += frameSize; } - try - { - ProtocolToken token = NextMessage(availableData.Slice(0, chunkSize), out int consumed); - _buffer.DiscardEncrypted(consumed); - return token; - } - catch (ArgumentOutOfRangeException) - { - System.Console.WriteLine($"FrameSize: {initFrameSize}, {frameSize}"); - System.Console.WriteLine($"AvailableData: {availableData.Length}, ChunkSize: {chunkSize}"); - System.Console.WriteLine($"Disposed: {ReferenceEquals(_exception, s_disposedSentinel)}"); - - throw; - } + ProtocolToken token = NextMessage(availableData.Slice(0, chunkSize), out int consumed); + _buffer.DiscardEncrypted(consumed); + return token; } // From c24a782db9c9ddaf7b5db445660abd6603ea3537 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 10 Jul 2024 18:14:14 +0200 Subject: [PATCH 5/5] Minor changes --- .../tests/FunctionalTests/SslStreamFramingTest.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs index 1328460f751767..f83e1b7027ebba 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamFramingTest.cs @@ -39,10 +39,10 @@ public enum FramingType // 1 byte reads ByteByByte, - // Receive at most 257 B in one read, targets specifically receiving + // Receive data at chunks, not necessarily respecting frame boundaries Chunked, - // coalesce reads to biggest chunks possible + // Coalesce reads to biggest chunks possible Coalescing } @@ -78,7 +78,6 @@ public static TheoryData Handshak [Theory] [MemberData(nameof(HandshakeScenarioData))] - // [InlineData(FramingType.Prime257, SslProtocols.Tls12, ClientCertScenario.None)] public async Task Handshake_Success(FramingType framingType, SslProtocols sslProtocol, ClientCertScenario clientCertScenario) { (Stream stream1, Stream stream2) = TestHelper.GetConnectedStreams();