CookieHeaderParserShared throws exception when last cookie contains invalid character

[JmlSaul]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

Cookie String: keya=valuea; keyb=valueb; errorcookie=dd,:("sa;
the last part of cookie string above is an invalid cookie and the whole cookie string ends with ;，this will throw exception like below, and all cookies fails to be read.

Exception: Nullable object must have a value.

at Microsoft.Net.Http.Headers.CookieHeaderParserShared.TryParseValues(StringValues values, IDictionary`2 store, Boolean enableCookieNameEncoding, Boolean supportsMultipleValues)
at Microsoft.AspNetCore.Http.RequestCookieCollection.ParseInternal(StringValues values, Boolean enableCookieNameEncoding)
at Microsoft.AspNetCore.Http.Features.RequestCookiesFeature.get_Cookies()
... try get value from cookies

Cookie String: keya=valuea; keyb=valueb; errorcookie=dd,:("sa
the last part of this cookie string above is an invalid cookie and the whole cookie string ends without ;，this will not throw exception, the parser will ignore errorcookie

Expected Behavior

error cookie be ignored, and normal cookie still readable.

Steps To Reproduce

No response

Exceptions (if any)

Nullable object must have a value.

at Microsoft.Net.Http.Headers.CookieHeaderParserShared.TryParseValues(StringValues values, IDictionary`2 store, Boolean enableCookieNameEncoding, Boolean supportsMultipleValues)
at Microsoft.AspNetCore.Http.RequestCookieCollection.ParseInternal(StringValues values, Boolean enableCookieNameEncoding)
at Microsoft.AspNetCore.Http.Features.RequestCookiesFeature.get_Cookies()

.NET Version

7.0

Anything else?

visual studio 2022 17.4.0

[Tratcher]

It's probably coming from this line:

aspnetcore/src/Http/Shared/CookieHeaderParserShared.cs

Line 32 in 860b12b

store[parsedName.Value.Value!] = Uri.UnescapeDataString(parsedValue.Value.Value!);

I wonder why TryParseValue didn't return false?

Would you like to contribute a fix for this?

[korteksz]

Hi @Tratcher,

I would like to try to fix it as my first contribution if it is not a problem.

[Tratcher]

@korteksz sure, first write the test and use that to figure out what the root issue is.

[korteksz]

@Tratcher

It's probably coming from this line:

aspnetcore/src/Http/Shared/CookieHeaderParserShared.cs

Line 32 in 860b12b

store[parsedName.Value.Value!] = Uri.UnescapeDataString(parsedValue.Value.Value!);

I wonder why TryParseValue didn't return false?

I think the problem is that if you have a cookie like errorcookie=dd,:("sa; then it gets parsed currently like:
Key: errorcookie Value: dd -> because it parses the cookie until it doesn't find a non-cookie character, in this case the ',' char. Then the while loop keeps iterating further until the last character which is a ';'. As far as this is a separator and we are at the end of the input value and because supportsMultipleValues is true we return true at the end, however our parsedName and parsedValue is null.

Update:
I've got a fix for that, but I don't know if its really appropriate and I don't know how should I proceed with the contribution. I assume I should commit my changes to the forked repo, but don't know for example how the review is happenning etc.

[adityamandaleeka]

@korteksz If you've got questions about the implementation, feel free to drop them here. Or you can just open a pull request with your proposed change (and include a test) and the team will review it and give you feedback. Guidance for how to do that is here: https://github.com/dotnet/aspnetcore/blob/main/CONTRIBUTING.md#how-to-submit-a-pr

Thanks for contributing!

[cnblogs-dudu]

Same issue in .NET 7.0.1.

[cnblogs-dudu]

The PR #45127 disappeared in the release .NET 7.0.1, please see CookieHeaderParserShared.cs#L29

[Tratcher]

@cnblogs-dudu this was only fixed in 8.0, not 7.0. Bug fixes are not automatically backported, you'd need to demonstrate that this issue is blocking your development. Do you understand why this cookie has invalid data, and how to avoid it or work around it?

[cnblogs-dudu]

@Tratcher There are lots of Nullable object must have a value error logs in our asp.net core app when upgrading from .NET 6 to .NET 7.

System.InvalidOperationException: Nullable object must have a value.
   at Microsoft.Net.Http.Headers.CookieHeaderParserShared.TryParseValues(StringValues values, IDictionary`2 store, Boolean enableCookieNameEncoding, Boolean supportsMultipleValues)
   at Microsoft.AspNetCore.Http.RequestCookieCollection.ParseInternal(StringValues values, Boolean enableCookieNameEncoding)
   at Microsoft.AspNetCore.Http.Features.RequestCookiesFeature.get_Cookies()
   at Microsoft.AspNetCore.Authentication.Cookies.ChunkingCookieManager.GetRequestCookie(HttpContext context, String key)
   at Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler.ReadCookieTicket()
   at Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler.HandleAuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.AuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(HttpContext context, String scheme)
   at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.AuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(HttpContext context, String scheme)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)

[kchinburarat]

I got same issue

eventdata.Exception.StackTraceString: at Microsoft.Net.Http.Headers.CookieHeaderParserShared.TryParseValues(StringValues values, IDictionary`2 store, Boolean enableCookieNameEncoding, Boolean supportsMultipleValues)
   at Microsoft.AspNetCore.Http.RequestCookieCollection.ParseInternal(StringValues values, Boolean enableCookieNameEncoding)
   at Agoda.Webgate.Services.MessageSender.CookieNamesHelper.GetIncomingCookieCollectionNames(HttpContext httpContext) in /build/Src/Agoda.Webgate.Services/MessageSender/WebgateMessage/CookieNamesHelper.cs:line 69
   at Agoda.Webgate.Services.MessageSender.WebgateMessageBuilder.BuildWebgateMessage(Int64 elapsedTime, HttpContext httpContext) in /build/Src/Agoda.Webgate.Services/MessageSender/WebgateMessage/WebgateMessageBuilder.cs:line 31
   at Agoda.Webgate.Services.MessageSender.WebgateMessageSender.Send(Int64 latency, HttpContext httpContext) in /build/Src/Agoda.Webgate.Services/MessageSender/WebgateMessage/WebgateMessageSender.cs:line 48
   
eventdata.Exception.Source: System.Private.CoreLib

System.Private.CoreLib: Nullable object must have a value.
  

Could you please release a fix version in .NET 7!! @Tratcher

[Tratcher]

Noisy error logs are not sufficient justification for a patch. There needs to be a functional business impact.

A) Are these legitimate requests?
B) Why do they contain invalid data in their cookies?
C) Is there a viable workaround?
D) What impact is this having on your business?

It should be possible to work around this by checking the cookie header first for known invalid data and removing it or terminating the request.