Closed Bug 1233784 (CVE-2016-1939) Opened 4 years ago Closed 4 years ago
firefox allows characters in cookie names that it shouldn't
+++ This bug was initially created as a clone of Bug #1191423 +++ So over in bug 1191423, we fixed problems with cookie values containing odd characters (vtab and such), but failed to fix the same issue with cookie names containing those odd characters. This bug is for that issue. I imagine this bug has the same potential security implications as 1191423, almost certainly no worse, and possibly not as bad (though I haven't done any serious analysis)
Here's the patch. See bug 1191423 comment 49 for the rationale.
Security issues seem the same, as the non standard characters will be appearing in the headers causing the same potential line break issues allowing an attacker to force a set-cookie on the victim with middle ware (nginx / apache / lightttp / django etc) not properly handling the non standard characters in the headers.
I think you skipped 0x1A through 0x1E there going from 0x19 to 0x20 ;)
hex != decimal
Attachment #8700254 - Flags: review?(jduell.mcbugs)
(In reply to Mark Straver from comment #3) > I think you skipped 0x1A through 0x1E there going from 0x19 to 0x20 ;) *facepalm* thanks!
Jason, given I'm on PTO as of, well, basically right now, if you r+ this, can you also get it landed and uplifted as appropriate? Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca28125f2f0d I added a one-line test
Comment on attachment 8700356 [details] [diff] [review] v2: with test Security consideration are exactly the same as for bug 1191423 comment 39. I personally didn't think that needed uplift, but it got it, so I'm a suspect witness :) Approval Request Comment [Feature/regressing bug #]: none [User impact if declined]: hypothetical sec risk [Describe test coverage new/current, TreeHerder]: xpcshell test [Risks and why]: very low, just blocking illegal characters in list. [String/UUID change made/needed]: none
Seems like a good idea to uplift to Beta and Aurora but we need this to land on Nightly first.
Comment on attachment 8700356 [details] [diff] [review] v2: with test Is there any chance this could cause some webcompat issue? Thanks
Attachment #8700356 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #11) > Is there any chance this could cause some webcompat issue? I'd say that's extremely unlikely; and if some site hits this they are IMO doing it very wrong to begin with, having control characters in there.
Comment on attachment 8700356 [details] [diff] [review] v2: with test OK, taking it in beta too then. Thanks!
Attachment #8700356 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Luckily, this is probably a sec-moderate since this bug was checked in without a security rating or sec-approval, violating our security bug process. https://wiki.mozilla.org/Security/Bug_Approval_Process
Oh damn. My bad--very sorry!
adding bounty nomination - bounty goes to email@example.com if awarded.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
(In reply to Mark Straver from comment #12) > (In reply to Sylvestre Ledru [:sylvestre] from comment #11) > > Is there any chance this could cause some webcompat issue? > > I'd say that's extremely unlikely; and if some site hits this they are IMO > doing it very wrong to begin with, having control characters in there. turns out this assessment was not correct. see: https://bugzilla.mozilla.org/show_bug.cgi?id=1244505
Depends on: 244505
I admit that I overlooked the space character. Since it's not exactly a control character, I guess it can't hurt allowing it? I never thought of a cookie name actually ever including spaces in the wild and I was always told to not include whitespace in names when designing so I assumed that was expected, standard practice applied everywhere? It doesn't seem like the RFC specifies at all what to do with whitespace characters in a name, it only talks about stripping surrounding whitespace.
the invention and RFC came in an age where browser development organizations were moving fast and furious and it actually worked against their best interest to write good and detailed interoperability specifications, and make sure they were followed. quirks that gave competitive advantage if you had the largest marketshare. Saying that we had proposed something that looked like a standard was a good thing to say, then it was good to move on to the next feature that need to be implemented to get competitive advantage, or keep pace. attention to detail was low on the priority list. know of any ages in internet development that sound like this? ;-) lou is an old friend and does a good job of explaining all this in http://www.montulli-blog.com/2013/05/the-reasoning-behind-web-cookies.html hence my comment in the other bug. beware when touching lou's 20 year old code. ;-)
and one more thing. I paid a bounty on this bug to todayisnew. I'm glad about paying that bounty for the bug he found in 20 year old code, but also a bit sad about also not catching the nuance that we now know caused problems. its a chance for us all to double down and be more careful; and to take more time to research, test, and do a good job.
Thanks for the kind words Chris :) If we are looking at fixing cookies: If we set the Cookie Max-Age value to a larger number: HTTP/1.1 200 OK Date: Thu, 18 Feb 2016 20:03:25 GMT Server: Apache X-Powered-By: PHP/5.5.30 Set-Cookie: BigMaxAgeValue1=1; expires=Thu, 01-Jan-2070 00:00:01 GMT; Max-Age=222294949494949411; Keep-Alive: timeout=2, max=200 Connection: Keep-Alive Content-Type: text/html Firefox does not know how to handle the Cookie Value and display blank cookies under the cookie manager. Not thinking much of a security risk for right now, but if fixing one, maybe fix the Max-Age Cookie value as well. Thanks :) -Eric
You need to log in before you can comment on or make changes to this bug.