From 83a330a683c99bf89398a6b554d8d9bf21b6b969 Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Tue, 9 Sep 2025 17:11:12 +0000 Subject: [PATCH] Merged PR 52828: Fix chunked request parsing #### AI description (iteration 1) #### PR Classification Bug fix to ensure correct parsing of HTTP chunked requests. #### PR Summary This pull request refines chunked request parsing by enforcing stricter checks on chunk extensions in accordance with RFC 9112, and it adds thorough tests for both valid and invalid input scenarios. The changes improve error handling and request rejection when encountering malformed chunk extensions. - **`src/Servers/Kestrel/test/FunctionalTests/ChunkedRequestTests.cs`**: Added tests to validate behavior for requests with invalid newlines and various chunk extension formats. - **`src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs`**: Updated parsing logic to correctly detect CRLF sequences and reject improperly formatted chunk extensions, including support for an insecure parsing switch. - **`src/Servers/Kestrel/Core/test/MessageBodyTests.cs`**: Modified test inputs to align with the updated chunk extension parsing. - **`src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs`**: Introduced a new rejection reason, `BadChunkExtension`, for invalid chunk extensions. - **`eng/PatchConfig.props`**: Updated patch configuration for version 2.3.4 to include the Kestrel core package changes. --- eng/PatchConfig.props | 1 + .../Core/src/BadHttpRequestException.cs | 3 + src/Servers/Kestrel/Core/src/CoreStrings.resx | 3 + .../src/Internal/Http/Http1MessageBody.cs | 60 ++++++-- .../src/Internal/Http/PipelineExtensions.cs | 46 +++++- .../Internal/Http/RequestRejectionReason.cs | 3 +- .../src/Properties/CoreStrings.Designer.cs | 28 ++-- .../Kestrel/Core/test/MessageBodyTests.cs | 4 +- .../FunctionalTests/ChunkedRequestTests.cs | 135 ++++++++++++++++++ 9 files changed, 251 insertions(+), 32 deletions(-) diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 0d92eaa0ebe5..ac9eea8b3533 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -44,6 +44,7 @@ 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", + "", + ""); + } + } + } } }