Skip to content
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

HttpHeaders TryParse can throw #102584

Closed
MihaZupan opened this issue May 22, 2024 · 4 comments · Fixed by #102693
Closed

HttpHeaders TryParse can throw #102584

MihaZupan opened this issue May 22, 2024 · 4 comments · Fixed by #102693
Assignees
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented May 22, 2024

Found by https://github.com/dotnet/runtime/blob/main/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs

var headers = new HttpRequestMessage().Headers;
headers.TryAddWithoutValidation("Via", "a\t\u2000");
_ = headers.ToArray();
System.ArgumentException: The value cannot be an empty string or composed entirely of whitespace. (Parameter 'receivedBy')
   at System.ArgumentException.ThrowNullOrWhiteSpaceException(String argument, String paramName)
   at System.ArgumentException.ThrowIfNullOrWhiteSpace(String argument, String paramName)
   at System.Net.Http.Headers.ViaHeaderValue.CheckReceivedBy(String receivedBy)
   at System.Net.Http.Headers.ViaHeaderValue..ctor(String protocolVersion, String receivedBy, String protocolName, String comment)
   at System.Net.Http.Headers.ViaHeaderValue.GetViaLength(String input, Int32 startIndex, Object& parsedValue)
   at System.Net.Http.Headers.GenericHeaderParser.GetParsedValueLength(String value, Int32 startIndex, Object storeValue, Object& parsedValue)
   at System.Net.Http.Headers.BaseHeaderParser.TryParseValue(String value, Object storeValue, Int32& index, Object& parsedValue)
   at System.Net.Http.Headers.HttpHeaders.TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, HeaderStoreItemInfo info, String value, Boolean addWhenInvalid)
   at System.Net.Http.Headers.HttpHeaders.ParseSingleRawHeaderValue(HeaderStoreItemInfo info, HeaderDescriptor descriptor, String rawValue)
   at System.Net.Http.Headers.HttpHeaders.ParseRawHeaderValues(HeaderDescriptor descriptor, HeaderStoreItemInfo info)
   at System.Net.Http.Headers.HttpHeaders.GetEnumeratorCore()+MoveNext()
   at DotnetFuzzing.Fuzzers.HttpHeadersFuzzer.g__Test|10_0(HttpHeaders headers, String name, String value) in D:\a\_work\1\s\src\libraries\Fuzzing\DotnetFuzzing\Fuzzers\HttpHeadersFuzzer.cs:line 62
   at DotnetFuzzing.Fuzzers.HttpHeadersFuzzer.FuzzTarget(ReadOnlySpan`1 bytes) in D:\a\_work\1\s\src\libraries\Fuzzing\DotnetFuzzing\Fuzzers\HttpHeadersFuzzer.cs:line 50
   at SharpFuzz.Fuzzer.LibFuzzer.Run(ReadOnlySpanAction action, Boolean ignoreExceptions)

var headers = new HttpRequestMessage().Headers;
headers.TryAddWithoutValidation("Warning", "1 \u2000 \"\"");
_ = headers.ToArray();
System.ArgumentException: The value cannot be an empty string or composed entirely of whitespace. (Parameter 'agent')
   at System.ArgumentException.ThrowNullOrWhiteSpaceException(String argument, String paramName)
   at System.ArgumentException.ThrowIfNullOrWhiteSpace(String argument, String paramName)
   at System.Net.Http.Headers.WarningHeaderValue.CheckAgent(String agent)
   at System.Net.Http.Headers.WarningHeaderValue..ctor(Int32 code, String agent, String text)
   at System.Net.Http.Headers.WarningHeaderValue.GetWarningLength(String input, Int32 startIndex, Object& parsedValue)
   at System.Net.Http.Headers.GenericHeaderParser.GetParsedValueLength(String value, Int32 startIndex, Object storeValue, Object& parsedValue)
   at System.Net.Http.Headers.BaseHeaderParser.TryParseValue(String value, Object storeValue, Int32& index, Object& parsedValue)
   at System.Net.Http.Headers.HttpHeaders.TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, HeaderStoreItemInfo info, String value, Boolean addWhenInvalid)
   at System.Net.Http.Headers.HttpHeaders.ParseSingleRawHeaderValue(HeaderStoreItemInfo info, HeaderDescriptor descriptor, String rawValue)
   at System.Net.Http.Headers.HttpHeaders.ParseRawHeaderValues(HeaderDescriptor descriptor, HeaderStoreItemInfo info)
   at System.Net.Http.Headers.HttpHeaders.GetEnumeratorCore()+MoveNext()
   at DotnetFuzzing.Fuzzers.HttpHeadersFuzzer.g__Test|10_0(HttpHeaders headers, String name, String value) in D:\a\_work\1\s\src\libraries\Fuzzing\DotnetFuzzing\Fuzzers\HttpHeadersFuzzer.cs:line 62
   at DotnetFuzzing.Fuzzers.HttpHeadersFuzzer.FuzzTarget(ReadOnlySpan`1 bytes) in D:\a\_work\1\s\src\libraries\Fuzzing\DotnetFuzzing\Fuzzers\HttpHeadersFuzzer.cs:line 50
   at SharpFuzz.Fuzzer.LibFuzzer.Run(ReadOnlySpanAction action, Boolean ignoreExceptions)
@MihaZupan MihaZupan added bug area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors labels May 22, 2024
@MihaZupan MihaZupan added this to the 9.0.0 milestone May 22, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@MihaZupan
Copy link
Member Author

Given that this appears with \u2000, my guess would be this is due to a mismatch of which characters our parsing logic considers as whitespace vs. ArgumentException.ThrowIfNullOrWhiteSpace.

@GMPrakhar
Copy link
Contributor

I don't understand the issue here. The unicode character "\u2000" is en quad, which is a whitespace character. This is evident from the string method for checking whitespace chars:
image

Is the issue to change the fuzzer logic to not include these characters as well?

@MihaZupan
Copy link
Member Author

The issue is that enumerating a header collection should never throw, even if we encounter invalid values that were added via TryAddWithoutValidation.
In this case it's likely that the header value constructors are being too strict by validating any whitespace characters, whereas the parsing logic is more limited (may only recognize Ascii space and tab for example).

We should change the logic here such that we don't throw anymore, likely by only looking for the relevant whitespace characters as part of ctor validation.

On a side note, we shouldn't be using validating constructors from TryParse in the first place as that's just wasted cycles to do validation twice.

@MihaZupan MihaZupan removed the help wanted [up-for-grabs] Good issue for external contributors label May 25, 2024
@MihaZupan MihaZupan self-assigned this May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants