Skip to content

Commit 52b5734

Browse files
Update branding to 2.3.7 & merge internal commits (#64089)
* 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. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> * Update branding to 2.3.7 * Fixup --------- Co-authored-by: Brennan Conroy <[email protected]>
1 parent fc3afb0 commit 52b5734

File tree

12 files changed

+260
-37
lines changed

12 files changed

+260
-37
lines changed

eng/Baseline.Designer.props

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
<Project>
33
<PropertyGroup>
44
<MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
5-
<AspNetCoreBaselineVersion>2.3.5</AspNetCoreBaselineVersion>
5+
<AspNetCoreBaselineVersion>2.3.6</AspNetCoreBaselineVersion>
66
</PropertyGroup>
77
<!-- Package: dotnet-dev-certs-->
88
<PropertyGroup Condition=" '$(PackageId)' == 'dotnet-dev-certs' ">
@@ -889,7 +889,7 @@
889889
</ItemGroup>
890890
<!-- Package: Microsoft.AspNetCore.Server.Kestrel.Core-->
891891
<PropertyGroup Condition=" '$(PackageId)' == 'Microsoft.AspNetCore.Server.Kestrel.Core' ">
892-
<BaselinePackageVersion>2.3.0</BaselinePackageVersion>
892+
<BaselinePackageVersion>2.3.6</BaselinePackageVersion>
893893
</PropertyGroup>
894894
<ItemGroup Condition=" '$(PackageId)' == 'Microsoft.AspNetCore.Server.Kestrel.Core' AND '$(TargetFramework)' == 'netstandard2.0' ">
895895
<BaselinePackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" Version="[2.3.0, )" />

eng/Baseline.xml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ This file contains a list of all the packages and their versions which were rele
44
build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch.
55
66
-->
7-
<Baseline Version="2.3.5">
7+
<Baseline Version="2.3.6">
88
<Package Id="dotnet-dev-certs" Version="2.1.1" />
99
<Package Id="dotnet-sql-cache" Version="2.1.1" />
1010
<Package Id="dotnet-user-secrets" Version="2.1.1" />
@@ -99,7 +99,7 @@ build of ASP.NET Core 2.1.x. Update this list when preparing for a new patch.
9999
<Package Id="Microsoft.AspNetCore.Routing" Version="2.3.0" />
100100
<Package Id="Microsoft.AspNetCore.Server.HttpSys" Version="2.3.0" />
101101
<Package Id="Microsoft.AspNetCore.Server.IISIntegration" Version="2.3.0" />
102-
<Package Id="Microsoft.AspNetCore.Server.Kestrel.Core" Version="2.3.0" />
102+
<Package Id="Microsoft.AspNetCore.Server.Kestrel.Core" Version="2.3.6" />
103103
<Package Id="Microsoft.AspNetCore.Server.Kestrel.Https" Version="2.3.0" />
104104
<Package Id="Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions" Version="2.3.0" />
105105
<Package Id="Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv" Version="2.3.0" />

eng/PatchConfig.props

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ Later on, this will be checked using this condition:
4343
</PackagesInPatch>
4444
</PropertyGroup>
4545
<PropertyGroup Condition=" '$(VersionPrefix)' == '2.3.6' ">
46+
<PackagesInPatch>
47+
Microsoft.AspNetCore.Server.Kestrel.Core;
48+
</PackagesInPatch>
49+
</PropertyGroup>
50+
<PropertyGroup Condition=" '$(VersionPrefix)' == '2.3.7' ">
4651
<PackagesInPatch>
4752
</PackagesInPatch>
4853
</PropertyGroup>

src/Servers/Kestrel/Core/src/BadHttpRequestException.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,9 @@ internal static BadHttpRequestException GetException(RequestRejectionReason reas
7272
case RequestRejectionReason.BadChunkSizeData:
7373
ex = new BadHttpRequestException(CoreStrings.BadRequest_BadChunkSizeData, StatusCodes.Status400BadRequest, reason);
7474
break;
75+
case RequestRejectionReason.BadChunkExtension:
76+
ex = new BadHttpRequestException(CoreStrings.BadRequest_BadChunkExtension, StatusCodes.Status400BadRequest, reason);
77+
break;
7578
case RequestRejectionReason.ChunkedRequestIncomplete:
7679
ex = new BadHttpRequestException(CoreStrings.BadRequest_ChunkedRequestIncomplete, StatusCodes.Status400BadRequest, reason);
7780
break;

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,4 +518,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
518518
<data name="BadDeveloperCertificateState" xml:space="preserve">
519519
<value>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.</value>
520520
</data>
521+
<data name="BadRequest_BadChunkExtension" xml:space="preserve">
522+
<value>Bad chunk extension.</value>
523+
</data>
521524
</root>

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

Lines changed: 46 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,7 @@ private class ForChunkedEncoding : Http1MessageBody
393393
{
394394
// byte consts don't have a data type annotation so we pre-cast it
395395
private const byte ByteCR = (byte)'\r';
396+
private const byte ByteLF = (byte)'\n';
396397
// "7FFFFFFF\r\n" is the largest chunk size that could be returned as an int.
397398
private const int MaxChunkPrefixBytes = 10;
398399

@@ -401,6 +402,13 @@ private class ForChunkedEncoding : Http1MessageBody
401402

402403
private Mode _mode = Mode.Prefix;
403404

405+
private static readonly bool InsecureChunkedParsing;
406+
407+
static ForChunkedEncoding()
408+
{
409+
InsecureChunkedParsing = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.EnableInsecureChunkedRequestParsing", out var value) && value;
410+
}
411+
404412
public ForChunkedEncoding(bool keepAlive, Http1Connection context)
405413
: base(context)
406414
{
@@ -554,56 +562,80 @@ private void ParseChunkedPrefix(ReadOnlySequence<byte> buffer, out SequencePosit
554562
BadHttpRequestException.Throw(RequestRejectionReason.BadChunkSizeData);
555563
}
556564

565+
// https://www.rfc-editor.org/rfc/rfc9112#section-7.1
566+
// chunk = chunk-size [ chunk-ext ] CRLF
567+
// chunk-data CRLF
568+
// https://www.rfc-editor.org/rfc/rfc9112#section-7.1.1
569+
// chunk-ext = *( BWS ";" BWS chunk-ext-name
570+
// [BWS "=" BWS chunk-ext-val] )
571+
// chunk-ext-name = token
572+
// chunk-ext-val = token / quoted-string
557573
private void ParseExtension(ReadOnlySequence<byte> buffer, out SequencePosition consumed, out SequencePosition examined)
558574
{
559-
// Chunk-extensions not currently parsed
560-
// Just drain the data
561-
consumed = buffer.Start;
562-
examined = buffer.Start;
575+
// Chunk-extensions parsed for \r\n and throws for unpaired \r or \n.
563576

564577
do
565578
{
566-
SequencePosition? extensionCursorPosition = buffer.PositionOf(ByteCR);
579+
SequencePosition? extensionCursorPosition;
580+
if (InsecureChunkedParsing)
581+
{
582+
extensionCursorPosition = buffer.PositionOf(ByteCR);
583+
}
584+
else
585+
{
586+
extensionCursorPosition = buffer.PositionOfAny(ByteCR, ByteLF);
587+
}
588+
567589
if (extensionCursorPosition == null)
568590
{
569591
// End marker not found yet
570592
consumed = buffer.End;
571593
examined = buffer.End;
572594
AddAndCheckConsumedBytes(buffer.Length);
573595
return;
574-
};
596+
}
575597

576598
var extensionCursor = extensionCursorPosition.Value;
577599
var charsToByteCRExclusive = buffer.Slice(0, extensionCursor).Length;
578600

579-
var sufixBuffer = buffer.Slice(extensionCursor);
580-
if (sufixBuffer.Length < 2)
601+
var suffixBuffer = buffer.Slice(extensionCursor);
602+
if (suffixBuffer.Length < 2)
581603
{
582604
consumed = extensionCursor;
583605
examined = buffer.End;
584606
AddAndCheckConsumedBytes(charsToByteCRExclusive);
585607
return;
586608
}
587609

588-
sufixBuffer = sufixBuffer.Slice(0, 2);
589-
var sufixSpan = sufixBuffer.ToSpan();
610+
suffixBuffer = suffixBuffer.Slice(0, 2);
611+
var suffixSpan = suffixBuffer.ToSpan();
590612

591-
if (sufixSpan[1] == '\n')
613+
if (InsecureChunkedParsing
614+
? (suffixSpan[1] == ByteLF)
615+
: (suffixSpan[0] == ByteCR && suffixSpan[1] == ByteLF))
592616
{
593617
// We consumed the \r\n at the end of the extension, so switch modes.
594618
_mode = _inputLength > 0 ? Mode.Data : Mode.Trailer;
595619

596-
consumed = sufixBuffer.End;
597-
examined = sufixBuffer.End;
620+
consumed = suffixBuffer.End;
621+
examined = suffixBuffer.End;
598622
AddAndCheckConsumedBytes(charsToByteCRExclusive + 2);
599623
}
600-
else
624+
else if (InsecureChunkedParsing)
601625
{
626+
examined = buffer.Start;
602627
// Don't consume suffixSpan[1] in case it is also a \r.
603628
buffer = buffer.Slice(charsToByteCRExclusive + 1);
604629
consumed = extensionCursor;
605630
AddAndCheckConsumedBytes(charsToByteCRExclusive + 1);
606631
}
632+
else
633+
{
634+
consumed = suffixBuffer.End;
635+
examined = suffixBuffer.End;
636+
// We have \rX or \nX, that's an invalid extension.
637+
BadHttpRequestException.Throw(RequestRejectionReason.BadChunkExtension);
638+
}
607639
} while (_mode == Mode.Extension);
608640
}
609641

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

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ private static unsafe void EncodeAsciiCharsToBytes(char* input, byte* output, in
216216
i += 4;
217217
}
218218

219-
trailing:
219+
trailing:
220220
for (; i < length; i++)
221221
{
222222
char ch = *(input + i);
@@ -269,5 +269,49 @@ private static byte[] CreateNumericBytesScratch()
269269
_numericBytesScratch = bytes;
270270
return bytes;
271271
}
272+
273+
/// <summary>
274+
/// Returns position of first occurrence of item in the <see cref="ReadOnlySequence{T}"/>
275+
/// </summary>
276+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
277+
public static SequencePosition? PositionOfAny<T>(in this ReadOnlySequence<T> source, T value0, T value1) where T : IEquatable<T>
278+
{
279+
if (source.IsSingleSegment)
280+
{
281+
int index = source.First.Span.IndexOfAny(value0, value1);
282+
if (index != -1)
283+
{
284+
return source.GetPosition(index);
285+
}
286+
287+
return null;
288+
}
289+
else
290+
{
291+
return PositionOfAnyMultiSegment(source, value0, value1);
292+
}
293+
}
294+
295+
private static SequencePosition? PositionOfAnyMultiSegment<T>(in ReadOnlySequence<T> source, T value0, T value1) where T : IEquatable<T>
296+
{
297+
SequencePosition position = source.Start;
298+
SequencePosition result = position;
299+
while (source.TryGet(ref position, out ReadOnlyMemory<T> memory))
300+
{
301+
int index = memory.Span.IndexOfAny(value0, value1);
302+
if (index != -1)
303+
{
304+
return source.GetPosition(index, result);
305+
}
306+
else if (position.GetObject() == null)
307+
{
308+
break;
309+
}
310+
311+
result = position;
312+
}
313+
314+
return null;
315+
}
272316
}
273317
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ public enum RequestRejectionReason
1515
UnexpectedEndOfRequestContent,
1616
BadChunkSuffix,
1717
BadChunkSizeData,
18+
BadChunkExtension,
1819
ChunkedRequestIncomplete,
1920
InvalidRequestTarget,
2021
InvalidCharactersInHeaderName,
@@ -32,6 +33,6 @@ public enum RequestRejectionReason
3233
MissingHostHeader,
3334
MultipleHostHeaders,
3435
InvalidHostHeader,
35-
RequestBodyExceedsContentLength
36+
RequestBodyExceedsContentLength,
3637
}
3738
}

src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs

Lines changed: 14 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,14 +139,14 @@ public async Task ReadExitsGivenIncompleteChunkedExtension()
139139
var stream = new HttpRequestStream(Mock.Of<IHttpBodyControlFeature>());
140140
stream.StartAcceptingReads(body);
141141

142-
input.Add("5;\r\0");
142+
input.Add("5;\r");
143143

144144
var buffer = new byte[1024];
145145
var readTask = stream.ReadAsync(buffer, 0, buffer.Length);
146146

147147
Assert.False(readTask.IsCompleted);
148148

149-
input.Add("\r\r\r\nHello\r\n0\r\n\r\n");
149+
input.Add("\nHello\r\n0\r\n\r\n");
150150

151151
Assert.Equal(5, await readTask.DefaultTimeout());
152152
Assert.Equal(0, await stream.ReadAsync(buffer, 0, buffer.Length));

0 commit comments

Comments
 (0)