-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[WinHTTP] Validate header values for ASCII #116256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: ManickaP <11718369+ManickaP@users.noreply.github.com>
src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Show resolved
Hide resolved
| { | ||
| if (!IsAscii(headers)) | ||
| { | ||
| throw new HttpRequestException("Request headers must contain only ASCII characters."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message should be in the resx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the hardcoded exception message to the Strings.resx file and updated the code to use SR.net_http_headers_must_be_ascii. The message is now properly localized. 00a1e04
| { | ||
| for (int i = 0; i < value.Length; i++) | ||
| { | ||
| if (value[i] > 127) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManickaP, is restricting to ASCII correct, or should it support Latin-1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for historical reasons it should allow Latin-1. On the other hand, it should not allow "dangerous" controls like CR LF and NUL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated header validation to allow Latin-1 characters (0-255) instead of just ASCII (0-127) for historical reasons, while still blocking dangerous control characters (CR, LF, NUL). Changed validation approach to check individual header values rather than the serialized header string to avoid issues with protocol formatting. Commit ab6c173.
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…king dangerous controls Co-authored-by: ManickaP <11718369+ManickaP@users.noreply.github.com>
|
After a discussion with @MihaZupan, closing this in favor of #116335. |
WinHttpHandler was passing headers to WinHTTP.dll without validating that header values contain only ASCII characters, unlike SocketsHttpHandler which performs this validation.
Changes
Added ASCII validation to
WinHttpHandler.AddRequestHeaders():IsAscii()helper method to check for ASCII characters (char <= 127)ValidateHeadersForAscii()method that throwsHttpRequestExceptionfor non-ASCII headersWinHttpCookieContainerAdapter.GetCookieHeader()requestMessage.Headers.ToString()requestMessage.Content.Headers.ToString()Added comprehensive tests:
SendAsync_RequestWithNonAsciiHeaderValue_ThrowsHttpRequestException()- validates rejection of non-ASCII request headersSendAsync_RequestWithAsciiHeaderValue_Succeeds()- validates ASCII headers work normallySendAsync_RequestWithNonAsciiContentHeader_ThrowsHttpRequestException()- validates rejection of non-ASCII content headersBehavior
Now throws
HttpRequestExceptionwith message "Request headers must contain only ASCII characters." when header values contain characters > 127, matching the behavior and security posture of SocketsHttpHandler.Testing
Fixes #115112.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.