Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Cookies separator ',' #390

Closed
jods4 opened this issue Sep 8, 2015 · 13 comments
Closed

Cookies separator ',' #390

jods4 opened this issue Sep 8, 2015 · 13 comments
Assignees

Comments

@jods4
Copy link

jods4 commented Sep 8, 2015

Because of this line of code:

while ((current < input.Length) && ((input[current] == ',') || (input[current] == ';')))

ASP.NET 5 accepts both ; and , as cookie value separators.
I know RFC 2965 said a server MUST accept ; and SHOULD accept , but newer RFC 6265 only speaks about ;. Specifically section 5.4 The Cookie Header, paragraph 4.2:

If there is an unprocessed cookie in the cookie-list, output the characters %x3B and %x20 ("; ").

I am opening this ticket because I have a real-world issue. My application is hosted on a sub-domain and because of that I receive third party cookies -- I know it's not the best situation but there's nothing I can do about it.

Some of those cookies contain JSON-like content:

{ "ccbm": 63, "ccus": 1440404858617, "ccfp": "oui"}

Which breaks the ASP.NET 5 cookie parser, which in turn breaks ASP.NET Identity middleware and my requests are refused because 401 Unauthorized.

@muratg muratg added this to the 1.0.0-RC1 milestone Sep 8, 2015
@muratg
Copy link

muratg commented Sep 8, 2015

@jods4
Copy link
Author

jods4 commented Sep 8, 2015

@muratg @Tratcher Especially 4.2.1 Syntax

The user agent sends stored cookies to the origin server in the
Cookie header. If the server conforms to the requirements in
Section 4.1 (and the user agent conforms to the requirements in
Section 5), the user agent will send a Cookie header that conforms to
the following grammar:

cookie-header = "Cookie:" OWS cookie-string OWS
cookie-string = cookie-pair *( ";" SP cookie-pair )

@Tratcher Tratcher self-assigned this Sep 8, 2015
@Tratcher Tratcher added the bug label Sep 8, 2015
@jods4
Copy link
Author

jods4 commented Sep 14, 2015

This is an important issue for me and the fix is trivial.
Would you take a PR and include it in beta-8?

@Tratcher
Copy link
Member

@jods4 Look at section 4.1.1 for the rest of the definition

cookie-pair       = cookie-name "=" cookie-value
cookie-value     = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash

Comma is explicitly disallowed in a cookie value.

We recognize comma as a separator because it is the standard separator between multi-line HTTP headers if they get merged to a single line.

People are always putting invalid junk in cookies... you're going to need to work around this by either preprocessing the headers to remove/modify the invalid values, or use a custom parser.

@jods4
Copy link
Author

jods4 commented Sep 14, 2015

@Tratcher Interesting. Bear with me as I'm no cookie expert:

First, the paragraph you quote speaks for Set-Cookie http header and I'm assuming the same restrictions apply to other means of setting a cookie, e.g. Javascript.

Although it is forbidden inside the value, the comma is still not a valid separator either, is it? Technically it doesn't change the fact that splitting on it is not correct behavior?

In fact, no comma should appear at all in the header. From a logic point of view, it probably means that the presence of a comma implies undefined behavior?

Now as you said there is junk in the outside world. If there is a comma in the header (trust me there is 😦), what should the server do?
Unless there is a browser that is known to send comma separated cookies (which is unlikely in my opinion, since this is not the RFC spec) I think it would be best if ASP.NET splits on semicolon only, which means: (a) it would be more compatible with (bad) code in the wild; (b) it would (somehow?) follow the RFC more closely; (c) it would make (marginally) less comparisons.

If you don't want to take action on this, can I ask you what's the best place to clean the cookies before ASP.NET parses them?

PS: I think you should consider that this is bound to pop up again in the future.

@jods4
Copy link
Author

jods4 commented Sep 14, 2015

I read more of the RFC.
Interestingly section 5.1 describes algorithms that user agents must/should implement.

5.2 is especially interesting because it explains how to parse the Set-Cookie header. Right from the start the RFC says:

NOTE: The algorithm below is more permissive than the grammar in
Section 4.1. [...] User agents use this algorithm so as to
interoperate with servers that do not follow the recommendations in
Section 4.

And indeed the algorithm is very permissive: basically it splits on ; and then on the first =. The rest makes up the name and the value, whatever is inside.

5.4 explains how to build the sent Cookie header, which -- you guessed it -- simply concatenates the previous values separated by = and ;.

I think that "to interoperate" it would make sense for ASP.NET to implement the same algorithms.

@Tratcher
Copy link
Member

Comma is supported because servers like IIS take this:

Cookie: a=1
Cookie: b=2

And convert it to this:

Cookie: a=1, b=2

Which is valid per the HTTP RFC regardless of the cookie RFC.

We also have to interoperate with older clients that use the old cookie RFC.

@jods4
Copy link
Author

jods4 commented Sep 16, 2015

@Tratcher Nitpicking but no it is not valid per the Cookie RFC. It states that browsers must only send a single Cookie header. So sending two is invalid behavior anyway. Is there any browser that does that?

Any hint as to what the best place to "fix" it in application code is? This is a real problem that I have to work around anyway.

PS: I am a bit disappointed that you choose to interoperate with buggy browsers (sending two cookies header or sending cookies separated by commas, both of which are invalid), which are few and rather standard-compliant nowadays; rather than interoperability with buggy websites that set invalid cookie values (which is legion, especially since JSON is a popular structured format).

@Tratcher
Copy link
Member

We interop with dozens of non-browser clients as well. It's a big wild web. JSON is explicitly invalid by both specs, there's no reason to try supporting it.

Your best bet is to get the raw header (in midldeware or app code) and parse it yourself, and optionally write back sanitized values to the headers.

@jods4
Copy link
Author

jods4 commented Sep 17, 2015

OK. We are discussing several invalid (and mutually incompatible) options, so in the end it's your call which one is more worth interoperating with than the other.

Your best bet is to get the raw header (in midldeware or app code) and parse it yourself, and optionally write back sanitized values to the headers.

I will try that. I have to write values back because the consumer of the cookies is the Cookie Auth Middleware, which I have no control upon.

If I process the header early enough can I just set it back? Is there something to do to invalidate a potentially already parsed cookie collection, or should I (can I?) set the cookie collection myself?

@skermajo
Copy link

I had a similar situation where a root domain was setting invalid cookie values (whitespace specifically) that broke the aspnet 5 cookie parser and subsequently the cookie auth middleware.

The hacky solution I ended up with was to specifically name the auth cookie and attach a custom cookie manager to the cookie auth middleware. When the auth middleware came looking for it's cookie value, the manager parsed the request cookie header directly with my own custom parsing that allowed for the misbehaved cookies. It was then trivial to pass the value back out to the auth middleware.

@davidfowl
Copy link
Member

Maybe we just need a better place to sanitize cookie values?

@Tratcher
Copy link
Member

Inline middleware is one of the better options. That or hooking into the IRequestCookiesFeature.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants