Closed Bug 1233784 (CVE-2016-1939) Opened 8 years ago Closed 8 years ago

firefox allows characters in cookie names that it shouldn't

Categories

(Core :: Networking: Cookies, defect)

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main44+])

Attachments

(2 files, 2 obsolete files)

+++ 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.
Attachment #8700083 - Flags: review?(jduell.mcbugs)
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)
Attachment #8700083 - Attachment is obsolete: true
Attachment #8700083 - 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!
Attachment #8700254 - Flags: review?(jduell.mcbugs) → review+
Attachment #8700254 - Attachment is obsolete: true
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
Attachment #8700356 - Flags: review+
Attachment #8700356 - Flags: approval-mozilla-beta?
Attachment #8700356 - Flags: approval-mozilla-aurora?
Seems like a good idea to uplift to Beta and Aurora but we need this to land on Nightly first.
https://hg.mozilla.org/mozilla-central/rev/ca28125f2f0d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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+
For vulnerability real life issues :) for both cookie value and cookie name:

Control characters can cause issues in the middle which allow bad things to happen :) This is old (ie6) but great these will no longer exist in FireFox allowable cookie characters :)


Some proxy servers that allow HT as a separator in the request line are:

Apache 2.0.54 (mod_proxy)
Squid 2.5.STABLE10-NT
Sun Java System Web Proxy Server 4.0


######

HTTP Request Splitting
HTTP Request Splitting is an attack that enables forcing the browser to send arbitrary HTTP requests, inflicting XSS and poisoning the browser's cache. The essence of the attack is the ability of the attacker, once the victim (browser) is forced to load the attacker's malicious HTML page, to manipulate one of the browser's functions to send 2 HTTP requests instead of one HTTP request. Two such mechanisms have been exploited to date: the XmlHttpRequest object (XHR for short) and the HTTP digest authentication mechanism. For this attack to work, the browser must use a forward HTTP proxy (not all of them "support" this attack), or the attack must be carried out against a host located on the same IP (from the browser's perspective) with the attacker's machine.
 
Basic attack example using XHR
Here's a JavaScript code (in the www.attacker.site domain) that can be used with IE 6.0 SP2 to send an arbitrary HTTP request to www.target.site (assuming the browser uses a forward proxy server). The arbitrary request is a GET request to /page,cgi?parameters, with HTTP/1.0 protocol, and with an additional "Foo:Bar" HTTP request header:
 var x = new ActiveXObject("Microsoft.XMLHTTP");
 
 x.open("GET\thttp://www.target.site/page.cgi?parameters\tHTTP
 /1.0\r\nHost:\twww.target.site\r\nFoo:Bar\r\n\r\nGET\thttp://nosuchhost/\tHTTP 
 /1.0\r\nBaz:","http://www.attacker.site/",false);
 x.send();
 alert(x.responseText);
From the browser's perspective, a single HTTP request is sent (with a long and very weird method specified by the sending HTML page...), whose target is www.attacker.site, i.e. not breaking the same origin policy, hence allowed.
Looking at the actual TCP stream, the forward proxy server receives:
 
 GET\thttp://www.target.site/page.cgi?parameters\tHTTP/1.0
 Host:\twww.target.site
 Foo:Bar
 GET\thttp://nosuchhost/\tHTTP/1.0
 Baz: http://www.attacker.site HTTP/1.0
 [...additional HTTP request headers added by the browser...]

Notice the use of HT (Horizontal Tab, ASCII 0x09) instead of SP (Space, ASCII 0x20) in the HTTP request line (the attacker has to resort to this because IE doesn't allow Space in the method field). This is clearly not allowed by the HTTP/1.1 RFC, yet many proxy servers do allow this syntax, and moreover, will convert HT to SP in the outgoing request (so the web server will have no idea that HTs were used).
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
Keywords: sec-moderate
Oh damn. My bad--very sorry!
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
adding bounty nomination -  bounty goes to todayisnew@gmail.com if awarded.
Flags: sec-bounty?
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
Alias: CVE-2016-1939
Flags: sec-bounty? → sec-bounty+
(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
Depends on: 1244505
No longer 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
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.