Harden chunked encoding parsing

[Youssef1313]

Fixes #66794

• It's easier, IMO, to read the whole updated class compared to reading the diff.
• The removal of the AppContext switch is intentional. It was introduced in older releases servicing, AFAIK, and wasn't intended to ever be in main branch.

[Copilot]

Pull request overview

This PR hardens Kestrel’s HTTP/1.1 chunked transfer-encoding parsing to strictly reject invalid chunk extensions (notably CR/LF inside chunk-ext, including quoted-string cases) per RFC 9110/9112, and updates tests accordingly. It also removes the legacy insecure chunked parsing AppContext switch from the mainline implementation.

Changes:

• Tighten Http1ChunkedEncodingMessageBody chunk-size and chunk-extension parsing (including stricter token/quoted-string validation and CRLF handling).
• Expand/adjust chunked request test coverage for invalid/valid extension forms and boundary splitting.
• Update existing tests’ chunk-extension strings to avoid now-invalid characters (e.g., spaces in tokens).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs	Reworks chunk-size and chunk-ext parsing to be stricter and RFC-aligned (including quoted-string rules).
src/Servers/Kestrel/Core/test/MessageBodyTests.cs	Adds new tests for incomplete chunk-extension/value parsing and max hex digit chunk-size parsing.
src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs	Expands valid/invalid chunk extension theory cases; updates extension examples; adjusts RemoteExecutor options.
src/Servers/Kestrel/test/InMemory.FunctionalTests/MaxRequestBodySizeTests.cs	Updates chunk-extension strings in payloads to remain valid under stricter parsing.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cincuranet]

With MaxRequestBodySize = 10 and

Transfer-Encoding: chunked\r\n
\r\n
5;a="bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

the 413 is not returned.

With 5;a="aaaaaaaaaaaa... ← 1 MB of 'a', no closing quote, no CRLF, written in 64-byte segments, because the code never advances consumed until a whole ;name[=value] unit parses, every arrival re-scans the entire pending extension. Serious CPU burn. The quoted variant is worst because it's parsed byte-at-a-time through a Func<byte,bool> delegate.

Finally, I'm missing tests for stuff like 2;a="x\r\ny"\r\n and 2;a="x\<CR>"\r\n.

[cincuranet]

Do we want to allow this now?

[Youssef1313]

I'm implementing the spec completely and trying to be 100% compliant. I think that's best.

[Youssef1313]

With MaxRequestBodySize = 10 and

Transfer-Encoding: chunked\r\n
\r\n
5;a="bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

the 413 is not returned.

With 5;a="aaaaaaaaaaaa... ← 1 MB of 'a', no closing quote, no CRLF, written in 64-byte segments, because the code never advances consumed until a whole ;name[=value] unit parses, every arrival re-scans the entire pending extension. Serious CPU burn. The quoted variant is worst because it's parsed byte-at-a-time through a Func<byte,bool> delegate.

Finally, I'm missing tests for stuff like 2;a="x\r\ny"\r\n and 2;a="x\<CR>"\r\n.

Let me see what's the best way to handle this. I guess we will need more "internal" modes to allow us to track where we were at in parsing extensions and then we can consume things more frequently.

[DeagleGross]

Jiri raised a great point about code re-reading the whole payload on the next iteration; but I think we can do in the follow-up PR - this PR can focus on char rejection only. If you decide to do in follow-up - lets create a separate issue to track this?

If i would be doing this, i would create a struct that holds mode, readOffset (which keeps track of how much data we already parsed from the input buffer), and chunkSize which does not require us re-reading the first byte to determine size. Then you can continue from last not read byte.

[DeagleGross]

I think we cant remove this context switch - @BrennanConroy documented it in the security doc.

https://github.com/dotnet/AspNetCore.Docs/blob/30a7a34c76078d9c999edc5e069a1b012dbd046c/aspnetcore/fundamentals/servers/kestrel/security-considerations.md?plain=1#L328

[Youssef1313]

Doc getting updated in dotnet/AspNetCore.Docs#37484

[DeagleGross]

you can reference

aspnetcore/src/Shared/ServerInfrastructure/HttpCharacters.cs

Line 30 in 757a459

private static readonly SearchValues<byte> _allowedTokenBytes = SearchValues.Create("!#$%&'*+-.^_`|~0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"u8);

IndexOfInvalidTokenChar() already gives you vectorized lookup.