[WinHTTP] Validate header values for Latin1 #116335
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances header validation to ensure that header values contain only valid Latin-1 characters and no dangerous control characters (CR, LF, or NUL). The changes replace existing newline-only checks with more comprehensive ones, add a new helper for Latin-1 validation in WinHttpHandler, and include additional tests verifying both failure and success scenarios for header values.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| HttpRuleParser.cs | Updated to include NUL character checks in header validation. |
| HttpResponseMessage.cs | Replaced call to old validation with the new ContainsNewLineOrNull check. |
| NameValueHeaderValue.cs | Updated header validation to catch NUL characters. |
| HttpHeaders.cs | Modified header parsing to reject values with CR, LF, or NUL. |
| GenericHeaderParser.cs | Updated parsing logic to use the improved header validation method. |
| AuthenticationHeaderValue.cs | Updated header validation by replacing the old method with the new one. |
| WinHttpHandlerTest.cs | Added tests to cover dangerous control characters and valid Latin-1 cases. |
| WinHttpHandler.cs | Introduced a helper to ensure headers contain only valid Latin-1 characters. |
| Strings.resx | Added a new resource string for invalid header value error messages. |
|
Tagging subscribers to this area: @dotnet/ncl |
| { | ||
| // See https://www.rfc-editor.org/rfc/rfc9110.html#section-5.5-5: | ||
| // "Field values containing CR, LF, or NUL characters are invalid and dangerous" | ||
| if (c <= 0 || c > 255 || c == '\r' || c == '\n') |
There was a problem hiding this comment.
| if (c <= 0 || c > 255 || c == '\r' || c == '\n') | |
| if (c is <= 0 or > 255 or '\r' or '\n') |
In the future Roslyn may be able to optimize this with pattern matching.
|
|
||
| private static bool ContainsOnlyValidLatin1(string headerString) | ||
| { | ||
| foreach (char c in headerString) |
There was a problem hiding this comment.
This has to build on .NET Framework as well. So I opted for shared, simple implementation, rather than trying to micro-optimize for a subset of targets.
| { | ||
| // See https://www.rfc-editor.org/rfc/rfc9110.html#section-5.5-5: | ||
| // "Field values containing CR, LF, or NUL characters are invalid and dangerous" | ||
| if (c <= 0 || c > 255 || c == '\r' || c == '\n') |
There was a problem hiding this comment.
If we want to filter for CRLF here, we'd have to do it on a per-header basis.
The HttpHeadeers.ToString() will always fail this check.
Including new lines is also a valid scenario for TryAddWithoutValidation if someone wants to use the (obsolete) line folding feature (header value split across multiple lines as long as new lines are followed by a space or tab).
I don't know if WinHttp supports sending those, but if it does I think we should let those through.
My preference would be to rely on the header collection to do the relevant checks here on modern .NET, and you only check that values are <= 255.
For framework you can't know whether a value was added with validation or not, so we might have to fall back to checking line folds manually if we want to filter out new lines.
3 participants
Fixes #115112
Replaces #116256