Reject bare CR in managed HttpListener request parsing - #133665
Open
Dev-next-gen wants to merge 1 commit into
Open
Reject bare CR in managed HttpListener request parsing#133665Dev-next-gen wants to merge 1 commit into
Dev-next-gen wants to merge 1 commit into
Conversation
The managed HttpListener dropped a CR that wasn't followed by a LF and joined the text on either side of it, both in the request line and headers (HttpConnection.ReadLine) and in chunk-size lines (ChunkStream.GetChunkSize). "Content-Le\rngth: 5" was treated as a Content-Length header and "1\r0\n" as a 0x10 byte chunk, while http.sys rejects all of these with a 400. Reject the bare CR instead. In ChunkStream this moves the existing "Missing \n" check to where it can actually fire; it sat after the read loop, where offset < size is always false.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Author
|
@dotnet-policy-service agree |
Author
|
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
This was referenced Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reading the chunk-size change in #132747 I noticed that the managed HttpListener silently drops a CR that isn't followed by a LF, and glues together the text on either side of it. I replayed a few raw requests against the managed listener (Linux, .NET 8.0.30) and against http.sys (Windows 11, .NET 8.0.29):
Content-Le\rngth: 5+ bodyHelloContent-Length: 5, body readX-Test: ab\rcdabcdGET /c\rd HTTP/1.1RawUrlis/cdHost: localhost\r\r\n1\r0\nBoth line readers do the same thing.
HttpConnection.ReadLinemoves toLineState.CRon a CR but keeps appending the bytes that follow, and the next LF ends the line, so a CR anywhere in the request line or a header just disappears.ChunkStream.GetChunkSizebehaves the same way. It does have aMissing \ncheck meant for this case, but it sits after the read loop, and the loop only exits without abreakonceoffset == size, sooffset < sizeis never true there.RFC 9112 section 2.2 says a recipient of a bare CR must treat the element as invalid or replace the CR with SP. Dropping it does neither, and
Content-Le\rngthbecomingContent-Lengthis the same kind of disagreement with an intermediary that #132163 and #132747 closed for whitespace.The change rejects a CR that isn't immediately followed by a LF in both places:
ReadLinethrows, whichProcessInputalready turns into a 400, and theMissing \ncheck inGetChunkSizemoves into the loop where it can fire. A bare LF is still accepted as a line terminator, and a CR and LF split across two reads still work, since the CR state carries over between calls as before.On the behavior change: the public API is untouched, and the only requests that now fail are ones that are invalid per the RFC and that http.sys already rejects with a 400 behind the same API, so this brings the managed implementation in line with Windows, as #130910 did for Content-Length.
Tests:
InvalidClientRequestTests, not gated on the implementation. I replayed exactly those four requests, built the wayGetContext_InvalidRequest_DoesNotGetContextbuilds them, against http.sys on Windows 11 and each got400 Bad Request; against the shipped managed listener each one got a context.Read_ChunkSizeWithBareCR_ThrowsHttpListenerException, managed only, next to the whitespace tests, because http.sys answers 400 before a context is handed out.I couldn't build the repo on the machines I had (no .NET 11 SDK), so I have not run the xunit tests themselves and am relying on CI for those. What I did run: a net8.0 harness that compiles
ChunkStream.csfrom the tree as-is andHttpConnection.ReadLineextracted verbatim. At main, 7 of its bare-CR checks fail; with this change they all pass, and the healthy-path checks (CR/LF split across reads, byte-by-byte input, bare LF endings, chunk extensions, several chunks, whitespace in the chunk size still rejected) pass on both sides.I also noticed that
ChunkStream.ReadTrailernever appends the trailer characters themselves to_saved, so trailer fields are dropped, and that0\r\n\r\r\nleaves the stream waiting for more data. I left that alone here; I can open an issue if it's useful.AI tools used