diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index 55f5bde688f0..c6fb576b6011 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -740,4 +740,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a {frameType} frame to a control stream that was too large. + + Bad chunk extension. + \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs index 5e426ed25721..6f2b39a205b7 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs @@ -16,6 +16,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; @@ -27,6 +28,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) { @@ -345,15 +348,31 @@ private void ParseChunkedPrefix(in ReadOnlySequence 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 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 @@ -361,9 +380,10 @@ private void ParseExtension(ReadOnlySequence buffer, out SequencePosition examined = buffer.End; AddAndCheckObservedBytes(buffer.Length); return; - }; + } var extensionCursor = extensionCursorPosition.Value; + var charsToByteCRExclusive = buffer.Slice(0, extensionCursor).Length; var suffixBuffer = buffer.Slice(extensionCursor); @@ -378,7 +398,9 @@ private void ParseExtension(ReadOnlySequence 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; @@ -387,13 +409,22 @@ private void ParseExtension(ReadOnlySequence 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); } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs index 827192823023..91467c6cb046 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs @@ -16,6 +16,7 @@ internal enum RequestRejectionReason UnexpectedEndOfRequestContent, BadChunkSuffix, BadChunkSizeData, + BadChunkExtension, ChunkedRequestIncomplete, InvalidRequestTarget, InvalidCharactersInHeaderName, @@ -31,5 +32,5 @@ internal enum RequestRejectionReason ConnectMethodRequired, MissingHostHeader, MultipleHostHeaders, - InvalidHostHeader + InvalidHostHeader, } diff --git a/src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs b/src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs index 05ae34f89802..6bfa5bfe60c4 100644 --- a/src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs +++ b/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; diff --git a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs index bf21a25153de..fa27c98f399a 100644 --- a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs +++ b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs @@ -338,14 +338,14 @@ public async Task ReadExitsGivenIncompleteChunkedExtension() var stream = new HttpRequestStream(Mock.Of(), 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 diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs index 5140f2c7e649..0e37009b4544 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs @@ -4,6 +4,7 @@ using System.Buffers; using System.Globalization; using System.Text; +using Microsoft.AspNetCore.Hosting.Server; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.InternalTesting; using Microsoft.AspNetCore.Server.Kestrel.Core; @@ -18,6 +19,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; @@ -1120,4 +1185,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}", + "", + ""); + } + } + } }