diff --git a/eng/Baseline.Designer.props b/eng/Baseline.Designer.props index ea74025028be..61a1e411b964 100644 --- a/eng/Baseline.Designer.props +++ b/eng/Baseline.Designer.props @@ -2,7 +2,7 @@ $(MSBuildAllProjects);$(MSBuildThisFileFullPath) - 2.3.5 + 2.3.6 @@ -889,7 +889,7 @@ - 2.3.0 + 2.3.6 diff --git a/eng/Baseline.xml b/eng/Baseline.xml index de74c4066f89..ec4682444a5b 100644 --- a/eng/Baseline.xml +++ b/eng/Baseline.xml @@ -4,7 +4,7 @@ This file contains a list of all the packages and their versions which were rele build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch. --> - + @@ -99,7 +99,7 @@ build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch. - + diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 0d92eaa0ebe5..a7ef0dd5ce54 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -43,6 +43,11 @@ Later on, this will be checked using this condition: + + Microsoft.AspNetCore.Server.Kestrel.Core; + + + diff --git a/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs b/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs index a29936b57c72..42f0bb0c413d 100644 --- a/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs +++ b/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs @@ -72,6 +72,9 @@ internal static BadHttpRequestException GetException(RequestRejectionReason reas case RequestRejectionReason.BadChunkSizeData: ex = new BadHttpRequestException(CoreStrings.BadRequest_BadChunkSizeData, StatusCodes.Status400BadRequest, reason); break; + case RequestRejectionReason.BadChunkExtension: + ex = new BadHttpRequestException(CoreStrings.BadRequest_BadChunkExtension, StatusCodes.Status400BadRequest, reason); + break; case RequestRejectionReason.ChunkedRequestIncomplete: ex = new BadHttpRequestException(CoreStrings.BadRequest_ChunkedRequestIncomplete, StatusCodes.Status400BadRequest, reason); break; diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index adfdbe110c22..44a43b53311b 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -518,4 +518,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The ASP.NET Core developer certificate is in an invalid state. To fix this issue, run the following commands 'dotnet dev-certs https --clean' and 'dotnet dev-certs https' to remove all existing ASP.NET Core development certificates and create a new untrusted developer certificate. On macOS or Windows, use 'dotnet dev-certs https --trust' to trust the new certificate. + + Bad chunk extension. + \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs index a323a6b611bb..a25ebacbcccb 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs @@ -393,6 +393,7 @@ private class ForChunkedEncoding : Http1MessageBody { // byte consts don't have a data type annotation so we pre-cast it private const byte ByteCR = (byte)'\r'; + private const byte ByteLF = (byte)'\n'; // "7FFFFFFF\r\n" is the largest chunk size that could be returned as an int. private const int MaxChunkPrefixBytes = 10; @@ -401,6 +402,13 @@ private class ForChunkedEncoding : Http1MessageBody private Mode _mode = Mode.Prefix; + private static readonly bool InsecureChunkedParsing; + + static ForChunkedEncoding() + { + InsecureChunkedParsing = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.EnableInsecureChunkedRequestParsing", out var value) && value; + } + public ForChunkedEncoding(bool keepAlive, Http1Connection context) : base(context) { @@ -554,16 +562,30 @@ private void ParseChunkedPrefix(ReadOnlySequence buffer, out SequencePosit BadHttpRequestException.Throw(RequestRejectionReason.BadChunkSizeData); } + // https://www.rfc-editor.org/rfc/rfc9112#section-7.1 + // chunk = chunk-size [ chunk-ext ] CRLF + // chunk-data CRLF + // https://www.rfc-editor.org/rfc/rfc9112#section-7.1.1 + // chunk-ext = *( BWS ";" BWS chunk-ext-name + // [BWS "=" BWS chunk-ext-val] ) + // chunk-ext-name = token + // chunk-ext-val = token / quoted-string private void ParseExtension(ReadOnlySequence buffer, out SequencePosition consumed, out SequencePosition examined) { - // Chunk-extensions not currently parsed - // Just drain the data - consumed = buffer.Start; - examined = buffer.Start; + // Chunk-extensions parsed for \r\n and throws for unpaired \r or \n. do { - SequencePosition? extensionCursorPosition = buffer.PositionOf(ByteCR); + SequencePosition? extensionCursorPosition; + if (InsecureChunkedParsing) + { + extensionCursorPosition = buffer.PositionOf(ByteCR); + } + else + { + extensionCursorPosition = buffer.PositionOfAny(ByteCR, ByteLF); + } + if (extensionCursorPosition == null) { // End marker not found yet @@ -571,13 +593,13 @@ private void ParseExtension(ReadOnlySequence buffer, out SequencePosition examined = buffer.End; AddAndCheckConsumedBytes(buffer.Length); return; - }; + } var extensionCursor = extensionCursorPosition.Value; var charsToByteCRExclusive = buffer.Slice(0, extensionCursor).Length; - var sufixBuffer = buffer.Slice(extensionCursor); - if (sufixBuffer.Length < 2) + var suffixBuffer = buffer.Slice(extensionCursor); + if (suffixBuffer.Length < 2) { consumed = extensionCursor; examined = buffer.End; @@ -585,25 +607,35 @@ private void ParseExtension(ReadOnlySequence buffer, out SequencePosition return; } - sufixBuffer = sufixBuffer.Slice(0, 2); - var sufixSpan = sufixBuffer.ToSpan(); + suffixBuffer = suffixBuffer.Slice(0, 2); + var suffixSpan = suffixBuffer.ToSpan(); - if (sufixSpan[1] == '\n') + if (InsecureChunkedParsing + ? (suffixSpan[1] == ByteLF) + : (suffixSpan[0] == ByteCR && suffixSpan[1] == ByteLF)) { // We consumed the \r\n at the end of the extension, so switch modes. _mode = _inputLength > 0 ? Mode.Data : Mode.Trailer; - consumed = sufixBuffer.End; - examined = sufixBuffer.End; + consumed = suffixBuffer.End; + examined = suffixBuffer.End; AddAndCheckConsumedBytes(charsToByteCRExclusive + 2); } - else + else if (InsecureChunkedParsing) { + examined = buffer.Start; // Don't consume suffixSpan[1] in case it is also a \r. buffer = buffer.Slice(charsToByteCRExclusive + 1); consumed = extensionCursor; AddAndCheckConsumedBytes(charsToByteCRExclusive + 1); } + else + { + consumed = suffixBuffer.End; + examined = suffixBuffer.End; + // We have \rX or \nX, that's an invalid extension. + BadHttpRequestException.Throw(RequestRejectionReason.BadChunkExtension); + } } while (_mode == Mode.Extension); } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/PipelineExtensions.cs b/src/Servers/Kestrel/Core/src/Internal/Http/PipelineExtensions.cs index e56c43b23cbc..7477aba365e3 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/PipelineExtensions.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/PipelineExtensions.cs @@ -216,7 +216,7 @@ private static unsafe void EncodeAsciiCharsToBytes(char* input, byte* output, in i += 4; } - trailing: + trailing: for (; i < length; i++) { char ch = *(input + i); @@ -269,5 +269,49 @@ private static byte[] CreateNumericBytesScratch() _numericBytesScratch = bytes; return bytes; } + + /// + /// Returns position of first occurrence of item in the + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static SequencePosition? PositionOfAny(in this ReadOnlySequence source, T value0, T value1) where T : IEquatable + { + if (source.IsSingleSegment) + { + int index = source.First.Span.IndexOfAny(value0, value1); + if (index != -1) + { + return source.GetPosition(index); + } + + return null; + } + else + { + return PositionOfAnyMultiSegment(source, value0, value1); + } + } + + private static SequencePosition? PositionOfAnyMultiSegment(in ReadOnlySequence source, T value0, T value1) where T : IEquatable + { + SequencePosition position = source.Start; + SequencePosition result = position; + while (source.TryGet(ref position, out ReadOnlyMemory memory)) + { + int index = memory.Span.IndexOfAny(value0, value1); + if (index != -1) + { + return source.GetPosition(index, result); + } + else if (position.GetObject() == null) + { + break; + } + + result = position; + } + + return null; + } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs index e1c96f203fd9..a9e4d02a977e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs @@ -15,6 +15,7 @@ public enum RequestRejectionReason UnexpectedEndOfRequestContent, BadChunkSuffix, BadChunkSizeData, + BadChunkExtension, ChunkedRequestIncomplete, InvalidRequestTarget, InvalidCharactersInHeaderName, @@ -32,6 +33,6 @@ public enum RequestRejectionReason MissingHostHeader, MultipleHostHeaders, InvalidHostHeader, - RequestBodyExceedsContentLength + RequestBodyExceedsContentLength, } } diff --git a/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs b/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs index 164573901fb2..73672f9d362a 100644 --- a/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs +++ b/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs @@ -388,20 +388,6 @@ internal static string BadRequest_UnrecognizedHTTPVersion internal static string FormatBadRequest_UnrecognizedHTTPVersion(object detail) => string.Format(CultureInfo.CurrentCulture, GetString("BadRequest_UnrecognizedHTTPVersion", "detail"), detail); - /// - /// Requests with 'Connection: Upgrade' cannot have content in the request body. - /// - internal static string BadRequest_UpgradeRequestCannotHavePayload - { - get => GetString("BadRequest_UpgradeRequestCannotHavePayload"); - } - - /// - /// Requests with 'Connection: Upgrade' cannot have content in the request body. - /// - internal static string FormatBadRequest_UpgradeRequestCannotHavePayload() - => GetString("BadRequest_UpgradeRequestCannotHavePayload"); - /// /// Failed to bind to http://[::]:{port} (IPv6Any). Attempting to bind to http://0.0.0.0:{port} instead. /// @@ -1890,6 +1876,20 @@ internal static string BadDeveloperCertificateState internal static string FormatBadDeveloperCertificateState() => GetString("BadDeveloperCertificateState"); + /// + /// Bad chunk extension. + /// + internal static string BadRequest_BadChunkExtension + { + get => GetString("BadRequest_BadChunkExtension"); + } + + /// + /// Bad chunk extension. + /// + internal static string FormatBadRequest_BadChunkExtension() + => GetString("BadRequest_BadChunkExtension"); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs index 5f006b12e3cc..051daf97a2a2 100644 --- a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs +++ b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs @@ -139,14 +139,14 @@ public async Task ReadExitsGivenIncompleteChunkedExtension() var stream = new HttpRequestStream(Mock.Of()); stream.StartAcceptingReads(body); - input.Add("5;\r\0"); + input.Add("5;\r"); var buffer = new byte[1024]; var readTask = stream.ReadAsync(buffer, 0, buffer.Length); Assert.False(readTask.IsCompleted); - input.Add("\r\r\r\nHello\r\n0\r\n\r\n"); + input.Add("\nHello\r\n0\r\n\r\n"); Assert.Equal(5, await readTask.DefaultTimeout()); Assert.Equal(0, await stream.ReadAsync(buffer, 0, buffer.Length)); diff --git a/src/Servers/Kestrel/test/FunctionalTests/ChunkedRequestTests.cs b/src/Servers/Kestrel/test/FunctionalTests/ChunkedRequestTests.cs index ecbb6f23b2a2..6cba89407d93 100644 --- a/src/Servers/Kestrel/test/FunctionalTests/ChunkedRequestTests.cs +++ b/src/Servers/Kestrel/test/FunctionalTests/ChunkedRequestTests.cs @@ -9,6 +9,7 @@ using System.Net.Sockets; using System.Text; using System.Threading.Tasks; +using Microsoft.AspNetCore.Hosting.Server; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; @@ -29,6 +30,68 @@ public class ChunkedRequestTests : LoggedTest } }; + [Theory] + [InlineData("2;\rxx\r\nxy\r\n0")] // \r in chunk extensions + [InlineData("2;\nxx\r\nxy\r\n0")] // \n in chunk extensions + public async Task RejectsInvalidChunkExtensions(string invalidChunkLine) + { + var testContext = new TestServiceContext(LoggerFactory); + using (var server = new TestServer(AppChunked, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "POST / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "Content-Type: text/plain", + "", + invalidChunkLine, + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 400 Bad Request", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + } + } + + [Theory] + [InlineData("2;a=b;b=c\r\nxy\r\n0")] // Multiple chunk extensions + [InlineData("2; \r\nxy\r\n0")] // Space in chunk extensions (BWS) + [InlineData("2;;;\r\nxy\r\n0")] // Multiple ';' in chunk extensions + [InlineData("2;novalue\r\nxy\r\n0")] // Name only chunk extension + //[InlineData("2 ;\r\nxy\r\n0")] // Technically allowed per spec, but we never supported it, and no one should be sending it + public async Task AllowsValidChunkExtensions(string chunkLine) + { + var testContext = new TestServiceContext(LoggerFactory); + using (var server = new TestServer(AppChunked, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "POST / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "Content-Type: text/plain", + "", + chunkLine, + "", + ""); + await connection.Receive( + "HTTP/1.1 200 OK", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 2", + "", + "xy"); + } + } + } + private async Task App(HttpContext httpContext) { var request = httpContext.Request; @@ -685,6 +748,78 @@ await connection.SendAll( } } } + + [Fact] + public async Task MultiReadWithInvalidNewlineAcrossReads() + { + // Inline so that we know when the first connection.Send has been parsed so we can send the next part + var testContext = new TestServiceContext(LoggerFactory) + { Scheduler = System.IO.Pipelines.PipeScheduler.Inline }; + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using (var server = new TestServer(async httpContext => + { + var readTask = httpContext.Request.Body.CopyToAsync(Stream.Null); + tcs.TrySetResult(true); + await readTask; + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.SendAll( + "GET / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "", + "1;\r"); + await tcs.Task; + await connection.SendAll( + "\r"); + await connection.ReceiveEnd( + "HTTP/1.1 400 Bad Request", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + } + } + + [Fact] + public async Task InvalidNewlineInFirstReadWithPartialChunkExtension() + { + // Inline so that we know when the first connection.Send has been parsed so we can send the next part + var testContext = new TestServiceContext(LoggerFactory) + { Scheduler = System.IO.Pipelines.PipeScheduler.Inline }; + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using (var server = new TestServer(async httpContext => + { + var readTask = httpContext.Request.Body.CopyToAsync(Stream.Null); + tcs.TrySetResult(true); + await readTask; + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.SendAll( + "GET / HTTP/1.1", + "Host:", + "Transfer-Encoding: chunked", + "", + "1;\n"); + await tcs.Task; + await connection.SendAll( + "t"); + await connection.ReceiveEnd( + "HTTP/1.1 400 Bad Request", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + } + } } } diff --git a/version.props b/version.props index 42d7cc7167b3..c1028f11baed 100644 --- a/version.props +++ b/version.props @@ -2,7 +2,7 @@ 2 3 - 6 + 7 true servicing