Merged PR 52770: Fix chunked request parsing

Author: BrennanConroy

src/Servers/Kestrel/Core/src/CoreStrings.resx

@@ -737,4 +737,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
   <data name="Http3ControlStreamFrameTooLarge" xml:space="preserve">
     <value>The client sent a {frameType} frame to a control stream that was too large.</value>
   </data>
+  <data name="BadRequest_BadChunkExtension" xml:space="preserve">
+    <value>Bad chunk extension.</value>
+  </data>
 </root>

src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs

@@ -15,6 +15,7 @@ internal sealed class Http1ChunkedEncodingMessageBody : 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;
 
@@ -26,6 +27,8 @@ internal sealed class Http1ChunkedEncodingMessageBody : Http1MessageBody
     private readonly Pipe _requestBodyPipe;
     private ReadResult _readResult;
 
+    private static readonly bool InsecureChunkedParsing = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.EnableInsecureChunkedRequestParsing", out var value) && value;
+
     public Http1ChunkedEncodingMessageBody(Http1Connection context, bool keepAlive)
         : base(context, keepAlive)
     {
@@ -343,25 +346,42 @@ private void ParseChunkedPrefix(in ReadOnlySequence<byte> buffer, out SequencePo
         KestrelBadHttpRequestException.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<byte> buffer, out SequencePosition consumed, out SequencePosition examined)
     {
-        // Chunk-extensions not currently parsed
-        // Just drain the data
-        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
                 consumed = buffer.End;
                 examined = buffer.End;
                 AddAndCheckObservedBytes(buffer.Length);
                 return;
-            };
+            }
 
             var extensionCursor = extensionCursorPosition.Value;
+
             var charsToByteCRExclusive = buffer.Slice(0, extensionCursor).Length;
 
             var suffixBuffer = buffer.Slice(extensionCursor);
@@ -376,7 +396,9 @@ private void ParseExtension(ReadOnlySequence<byte> buffer, out SequencePosition
             suffixBuffer = suffixBuffer.Slice(0, 2);
             var suffixSpan = suffixBuffer.ToSpan();
 
-            if (suffixSpan[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;
@@ -385,13 +407,22 @@ private void ParseExtension(ReadOnlySequence<byte> buffer, out SequencePosition
                 examined = suffixBuffer.End;
                 AddAndCheckObservedBytes(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;
                 AddAndCheckObservedBytes(charsToByteCRExclusive + 1);
             }
+            else
+            {
+                consumed = suffixBuffer.End;
+                examined = suffixBuffer.End;
+
+                // We have \rX or \nX, that's an invalid extension.
+                KestrelBadHttpRequestException.Throw(RequestRejectionReason.BadChunkExtension);
+            }
         } while (_mode == Mode.Extension);
     }
 

src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs

@@ -16,6 +16,7 @@ internal enum RequestRejectionReason
     UnexpectedEndOfRequestContent,
     BadChunkSuffix,
     BadChunkSizeData,
+    BadChunkExtension,
     ChunkedRequestIncomplete,
     InvalidRequestTarget,
     InvalidCharactersInHeaderName,
@@ -32,5 +33,5 @@ internal enum RequestRejectionReason
     MissingHostHeader,
     MultipleHostHeaders,
     InvalidHostHeader,
-    RequestBodyExceedsContentLength
+    RequestBodyExceedsContentLength,
 }

src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs

@@ -49,6 +49,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;

src/Servers/Kestrel/Core/test/MessageBodyTests.cs

@@ -338,14 +338,14 @@ public async Task ReadExitsGivenIncompleteChunkedExtension()
             var stream = new HttpRequestStream(Mock.Of<IHttpBodyControlFeature>(), reader);
             reader.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());
             try

src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs

@@ -9,6 +9,7 @@
 using System.Linq;
 using System.Text;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Hosting.Server;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
 using Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests.TestTransport;
@@ -21,6 +22,70 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests;
 
 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);
+
+        await 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",
+                    "Content-Length: 0",
+                    "Connection: close",
+                    $"Date: {testContext.DateHeaderValue}",
+                    "",
+                    "");
+            }
+        }
+    }
+
+    [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);
+
+        await 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",
+                "Content-Length: 2",
+                $"Date: {testContext.DateHeaderValue}",
+                "",
+                "xy");
+            }
+        }
+    }
+
     private async Task App(HttpContext httpContext)
     {
         var request = httpContext.Request;
@@ -1115,4 +1180,86 @@ await connection.Receive(
             }
         }
     }
+
+    [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);
+
+        await using (var server = new TestServer(async httpContext =>
+        {
+            var request = httpContext.Request;
+            var readTask = request.BodyReader.ReadAsync();
+            tcs.TrySetResult();
+            var readResult = await readTask;
+            request.BodyReader.AdvanceTo(readResult.Buffer.End);
+        }, 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",
+                    "Content-Length: 0",
+                    "Connection: close",
+                    $"Date: {testContext.DateHeaderValue}",
+                    "",
+                    "");
+            }
+        }
+    }
+
+    [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);
+
+        await using (var server = new TestServer(async httpContext =>
+        {
+            var request = httpContext.Request;
+            var readTask = request.BodyReader.ReadAsync();
+            tcs.TrySetResult();
+            var readResult = await readTask;
+            request.BodyReader.AdvanceTo(readResult.Buffer.End);
+        }, 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",
+                    "Content-Length: 0",
+                    "Connection: close",
+                    $"Date: {testContext.DateHeaderValue}",
+                    "",
+                    "");
+            }
+        }
+    }
 }