1191423 - (CVE-2015-7208) allowing vertical tab in cookies leads to cookie injection on some servers

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[todayisnew]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.125 Safari/537.36

Steps to reproduce:

Description:

Firefox allows storing vertical tabs (ASCII 11) in cookies being set.
When later replayed by the browser, they can be interrupted by parser as a new line
This allows setting arbitrary cookies for user.

Example case:


1) 

User visits http://www.doyoulikepizza.com, it asks of course do you like pizza?

It parses this user submited data and stores it in a cookie named likespizza.

This data is cleaned for xss, or injection but allows through a vertical tab since there is no harm IE, and Chrome do not allow saving vertical tabs in cookies.

This cookie is roughly stored in the jar as:

likespizza:true;{vertialtab}currentsessionpassword:12345678


2) 

Malicious user sends link which injects value to set likespizza to create user cookie;

http://www.doyoulikepizza.com?pizzaparty.php?likespizza=true;{verticaltab}currentsessionpassword:12345678;

3) Impact:

When victum vists http://www.doyoulikepizza.com firefox sends the current stored cookie for doyoulikepizza.com

When we visit the site we should have the vertical tab just be parsed as a regular character

GET / http/1.0
Host: doyoulikepizza.com
Cookie: likespizza:true;{vertialtab}currentsession:12345678

But with the middle parser Nginx / lighttp / apache / django etc might parses the vertical tab as a /n or /r so we get a,

GET / http/1.0
Host: doyoulikepizza.com
Cookie: likespizza:true;
Cookie: currentsessionpassword:12345678;

Impact

Many sites facebook google etc, parse http only cookies from url or user submitted content, its an injection point for this type 
of exploit, the middle parser tried to be helpful but can't be trusted to properly parse the vertical tab.

Suggestion:

Firefox should filter non standard characters from cookie data, if other browsers do not support them then it is unlikely a developer will
use these characters in cookie data, also if wanted to be used we can base64 encode the data for the cookie to keep it safe and return the characters vertical tab etc inside.

Comment 1

[Jason Duell]

Nick--can you take this?  We don't have anyone handy who knows the cookie code well, but we need this fixed.

Comment 2

[Jason Duell]

> But with the middle parser Nginx / lighttp / apache / django etc might 
> parse the vertical tab as a /n or /r

Do we have evidence that any of these servers are parsing a vertical tab as a carriage return?  I.e. how real vs theoretical is this threat right now?

> IE, and Chrome do not allow saving vertical tabs in cookies.

If true that's probably reason enough for us to ban them too.

Comment 3

[u408661]

Does anyone know offhand if IE & Chrome strip vtab from cookies, or just reject cookies containing vtab entirely? (I can figure that out myself, but I always like to ask in case anyone reading knows already.)

Comment 4

[u408661]

So, I just tested... and chrome canary 47.0.2494.0 will happily save (and send!) a cookie with a vtab in it.

Comment 5

[Jason Duell]

Daniel,

So this bug is looking possibly spurious.  Do you or anyone else in security know if there really are server-side vulnerabilities with parsing vertical tabs (ASCII 11) as carriage returns?

Comment 6

[Jason Duell]

Original reporter (Today is still new :)

Do you have reproducible cases where this is happening that I can reach with a URI?  The doyoulikepizza site seems to be a blank website.

Comment 7

[todayisnew]

good day :) sorry for slow response lucky to have my first new born child here :) happy little girl.  The domain ilikepizza was an example not a real domain :)

I'll look into better case examples, I had tested against chrome and Internet explorer builds about 2 months back, I was looking to have my server send a %0a%0d to force the cookie onto a new line.  When I found Firefox was the only browser that would save out ASCII 11 (vertical tab) 

I will go through all characters ASCII 0-255 to save out to cookies for chrome / safari / Internet explorer / and Firefox current builds post back with results.  Hope the day treats you all well :)

-Eric

Comment 8

[Daniel Veditz [:dveditz]]

Control characters aren't valid in cookies, we should neither accept nor emit such cookies.
http://tools.ietf.org/html/rfc6265#section-4.1.1

Comment 9

[u408661]

While I agree with the RFC (that control characters in cookies are rather silly), so far every browser I've tested (Firefox, Safari, and Chrome - I'm working on getting a windows VM up on this machine to test IE) will happily accept and send cookies with control characters in them (or at least, vtabs - I'll follow up with tests of other control characters).

I'll also see if httparchive keeps cookie-related stats (though I doubt they do) to see if use of RFC-disallowed characters is prevalent on the internet, as making this change with (so far) such wide support for brokenness could end up breaking things for users who regularly visit non-compliant sites.

Comment 10

[u408661]

Just checked IE 11 on WinXP (the only windows/IE combo I have currently available to me), and it also happily accepts and sends vtabs in cookies.

Comment 11

[todayisnew]

Good day thanks for the patience :)

Hmm After doing further tests, it seems that Firefox is in the company with with Internet Explorer 9, that is still allowing control characters to be set  / resent as cookies.

Browsers That will not store Control characters  00-20 (Hex) as Cookies, and resend the request as cookies from browser to server:

- Google Chrome Current Build Desktop
- Safari Current Build Desktop
- Internet Explorer 10+ Desktop
- Opera 31.0 Desktop

Browsers That will store Control characters  00-20 (Hex)
- Internet Explorer 9
- Firefox current build

Control Characters:

Control Characters: Hex 00-20
NUL	null
SOH	start of heading
STX	start of text
ETX	end of text
EOT	end of transmission
ENQ	enquiry
ACK	acknowledge
BEL	bell
BS	backspace
HT	horizontal tab
VT	vertical tab
NP	new page (or FF, form feed)
SO	shift out
SI	shift in
DLE	data link escape
DC1	device control 1
DC2	device control 2
DC3	device control 3
DC4	device control 4
NAK	negative acknowledge
SYN	synchronous idle
ETB	end of transmission block
CAN	cancel
EM	end of medium
SUB	substitute
ESC	escape
FS	file separator
GS	group separator
RS	record separator
US	unit separator
SP	space


Example Php to test:

http://me6.com/xss/SimpleCookieTest.php

Source if want to make own:

<?php

header('Set-Cookie: firefoxtest=    !\"#$%&\'()*+,-./0123456789:<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~€‚ƒ„…†‡ˆ‰Š‹ŒŽ‘’“”•–—˜™š›œžŸ ¡¢£¤¥¦§¨©ª«¬­®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ');

?>


Results from fiddler dump:

GET http://me6.com/xss/SimpleCookieTest.php HTTP/1.1
Host: me6.com
User-Agent: Mozilla/5.0 (Android; Mobile; rv:29.0) Gecko/29.0 Firefox/29.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-GB,en;q=0.5
Accept-Encoding: gzip, deflate
Cookie: firefoxtest=    
 !\"#$%&'()*+,-./0123456789:<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~€‚ƒ„…†‡ˆ‰Š‹ŒŽ‘’“”•–—˜™š›œžŸ ¡¢£¤¥¦§¨©ª«¬­®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ
Connection: keep-alive


HTTP/1.1 200 OK
Date: Fri, 28 Aug 2015 18:12:47 GMT
Server: Apache
X-Powered-By: PHP/5.5.28
Set-Cookie: firefoxtest=    
 !\"#$%&'()*+,-./0123456789:<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~€‚ƒ„…†‡ˆ‰Š‹ŒŽ‘’“”•–—˜™š›œžŸ ¡¢£¤¥¦§¨©ª«¬­®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ
Content-Length: 0
Keep-Alive: timeout=2, max=200
Connection: Keep-Alive
Content-Type: text/html

Comment 12

[todayisnew]

Attachment: FireFoxScreenShot.jpg

Cookie Characters Set

Comment 13

[todayisnew]

Attachment: Fiddler logs of request

Comment 14

[todayisnew]

Attachment: SimpleCookieTest.php Test Php to set cookies containing characters for ascii 0-255

Comment 15

[todayisnew]

The FireFoxScreenShot.jpg shows a better view of the the spacing, the browser here renders out the new lines being set / resent in the cookie.  

In my experience and testing, Chrome, Safari, Opera do not allow control characters to be set or accepted, it will store an encoded vertical tab as a cookie as "%0B" in chrome.

In Firefox it will store a "♂" which is a vertical tab as its actual ASCII character.

Hope all is well on all of your sides of the screen.  It just seems if most other browsers are filtering, this content for years, would make Firefox more secure to not allow it.

Comment 16

[u408661]

Argh, and of course there was an issue with my test case (accounting for the difference in results) - I was using a framework that "helpfully" turned control characters into strings containing the octal escape sequence for that character.

I wrote yet another test that does not translate control characters into escape sequence strings, and sends one Set-Cookie for each of the ascii characters (other than ;, \0, \r, or \n - see below for why I skipped those). This was more productive. Here's the behavior I'm seeing

- As the reporter suggests, firefox will accept any old character (with the exception of ;, \0, \r, and \n) in a cookie. This was apparently a conscious decision made when the cookie parser was written, as the comments explicitly say we deviate from 6265 and only disallow the 4 characters listed above.

- Chrome appears to be mostly in-line with 6265, though I was able to get it to accept (and send) cookies containing 0x09 (tab), 0x20 (space), 0x22 (dquote), 0x2C (comma), 0x5C (backslash), and 0x7F (del).

- Safari appears to freak out at the first invalid cookie and not accept any cookies sent at all (at least in my case). It does, strangely, store the first invalid cookie name with an empty value and send that on later requests (so it stored the cookie named "ascii0x01" with an empty value and sent that on subsequent requests).

- I did not test IE, as this seems like enough information to go on for now. I will test later.

Looking at our cookie code, it seems like the easiest route to go is to behave like chrome. We could be more strict than them and fully follow 6265, but I don't see any particular reason not to (none of the extra characters they allow are particularly harmful, with the possible exception of dquote).

Comment 17

[todayisnew]

Nicholas its all good, the helper tools betray us on occasion :)

Thanks for the heads up, you found some mistakes I made with chrome sneaking extra tab space comma dquote backslash and del! :)

Comment 18

[u408661]

Attachment: patch

This disallows the same set of characters that chrome disallows in cookies.
The test doesn't test *all* disallowed values, as there's some disconnect
between xpcshell and gecko that results in some of the disallowed values not
making it through to SetCookieInternal, and therefore some empty cookies
being created, so the test just exists as a sanity check.

I'm not sure how we'll want to go about landing this - the implications of
this bug don't seem too horrid to me, as they require server-side
cooperation to exploit at 2 levels (1 - allowing setting cookies directly
from user input, and 2 - improper parsing of sent cookie values), but I'll
hold of on any landing or try pushing until I get guidance.

Comment 19

[todayisnew]

alot of sites Facebook Google set cookies based off the current browser URL, user can append any value to the URL after a ?<attackpayload> or a &randomvalue=<attackpayload>

Tesla for example will let you set arbitrary cookie values here:

http://www.teslamotors.com/models/design?advocate=anythingwewant%2B%3D

So if a middle parser apache Nginx Django etc does not know how to handel these cookies a csrf to the site with the users auth already in place to modify and create the attack cookie, then another request to put the malicious cookie into play.

Happy it got patched :) If no bug bounty I understand :)  you seem a lot more down to earth as a group of devs then my experience with bugs with chrome and google ill do some better research and get some better bugs :)

Is the new Firefox port to Android in scope for bug bounties, I can see the iOS version is still in testing?

Have a good one and keep up the good work :)

-Eric

Comment 20

[u408661]

Dan, how do we want to go about landing this one?

Comment 21

[Daniel Veditz [:dveditz]]

Clearly this is confirmed.

I believe this was mis-rated "sec-high" based on a misinterpretation of the bug summary -- that it allowed modifying the cookies of any arbitrary site from elsewhere. Rather you can inject cookies on some sites that meet certain conditions which is a more limited attack. Could still, in theory, allow for something like a session-fixation attack on particular sites. Lowering to sec-moderate.

Nick: in general the answer to comment 20 for sec bugs is you request sec-approval? on the patch and answer the questions in the template about risk and affected versions. Since I've downrated it to moderate you can just land. It'd be nice to get this on aurora, too. Not urgent to fix on beta.

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to todayisnew from comment #19)
> Happy it got patched :) If no bug bounty I understand :) [...]
> 
> Is the new Firefox port to Android in scope for bug bounties, I can see the
> iOS version is still in testing?

bounty question are off-topic for bugs and best addressed in mail to our security@ mailing list, but this bug is nominated for a bounty and the bounty folks will consider it whenever their next meeting is.

Firefox for Android is definitely in scope for bounties.

Firefox for iOS has not had any formal security review so it's not in scope yet. Since we're forced to use the iOS webkit view to do the actual browsing on that platform we need to figure out which security aspects we can actually be responsible for.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 8655107 [details] [diff] [review]
patch

[Triage Comment]
approved for Aurora landing
a=dveditz

Comment 24

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac366b0a2ea

Comment 25

[u408661]

Will land on aurora once inbound seems happy with the push

Comment 26

[u408661]

And backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c08bba1bd822 for causing https://treeherder.mozilla.org/logviewer.html#?job_id=13710043&repo=mozilla-inbound

Apparently some of our tests relied on our broken cookie parsing :)

Comment 27

[u408661]

Attachment: patch v2

Here's a new version that, by my testing, appears to not cause the giant mess of cookie-related test failures that the previous one caused (and should also fix the ASAN complaint about a buffer overrun - forgot to manually null-terminate my list of characters that we disallow).

The big difference here is that the blacklist now only applies to cookies set via the Set-Cookie header. "Why is that?" I hear you asking. Here's the fun part. Set-Cookie is defined by RFC 6265 as able to carry a subset of US-ASCII characters (as we've already noted). The problem is document.cookie, which is defined by the HTML5 spec as UTF-8. Who needs anything to match?

Jason, this seems right-ish to me, and like I said, many more tests are passing with this version than with my previous version, but one never knows.

Comment 28

[Jason Duell]

Hmm, in 

  http://www.w3.org/html/wg/drafts/html/master/single-page.html#dom-document-cookie

it says that setting document.cookie should "act as it would when receiving a set-cookie-string for the document's address via a "non-HTTP" API, consisting of the new value encoded as UTF-8".  My read of that is that the same set of illegal characters should be enforced for document.cookie setting as well as HTTP header cookie setting. I.e. we should only allow the "cookie-octet" chars from RFC 6265:

  %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash

(AFAICT the only thing the "non-HTTP"-ness affects when getting/setting a cookie is that you can't see httponly cookies.)

bz: you +r'd the patches ages ago that added the tests here (dom/html/test/test_non-ascii-cookie.html) that are barfing because they want to use values other than these.  Do you repent and see the error of your ways (i.e. we should change the tests), or is there reason to support the use of other chars, only when using document.cookie?

Nick: the only test that's failing is test_non-ascii-cookie.html, right?

Comment 29

[Jason Duell]

Note: see bug 784367 and maybe bug 826159 for the prior work here.

Comment 30

[u408661]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #28)
> Nick: the only test that's failing is test_non-ascii-cookie.html, right?

No, there were a ton of tests that failed with the old version of the patch, all of which are fixed (at least locally) by only applying this to Set-Cookie instead of document.cookie.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> or is there reason to support the use of other chars, only when using document.cookie

What do other browsers do?  That's the only real criterion for document.cookie.

In spec terms, as usual the RFC doesn't define a processing model and in particular doesn't say what should happen when the header value does not match the cookie-string production.  the HTML spec thus has no way to link to where it enters the processing model from the RFC (because the RFC doesn't define one), and hence it's not clear whether the cookie-string restrictions would in fact apply to the DOM API.  Yay, standards.

Once we figure out what other browsers do here, we should file an HTML spec bug to define the behavior here better.

Comment 32

[u408661]

OK, here's the state of other browsers (specifically, I tested using the cookie set via document.cookie from test_non-ascii-cookie.html):

Chrome (canary 47.0.2511.0): accepts the cookie both via document.cookie and Set-Cookie
Safari (8.0.8 (10600.8.9)): does not accept the cookie from either source (though it does create an empty cookie with the same name)
Edge (20.10240.16384.0): accepts the cookie via document.cookie, does NOT accept the cookie via Set-Cookie. NOTE: this is the same behavior that my proposed patch will introduce for Firefox.
IE 11: same as Edge (not really surprising)

Given the... inconsistent state of this across browsers (plus the fact that my proposed behavior aligns with at least one of the other major browsers), I feel relatively safe with this change. Of course, it's the internet, so there's always the possibility of me breaking everything for everyone :)

Comment 33

[Jason Duell]

Comment on attachment 8657343 [details] [diff] [review]
patch v2

Review of attachment 8657343 [details] [diff] [review]:
-----------------------------------------------------------------

I agree--this seems good enough for now, at least.  Ship it! :)

Comment 34

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03bdce0dcbb0

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03bdce0dcbb0

Comment 36

[todayisnew]

Thanks all for letting me join in and see the adventure along the way :)  

You do a lot of hard work and back and forth to make sure it gets fixed proper :)  Thought fining the bugs was hard, fixing them seems to be just as hard too ;)

Good luck with the adventures and I'll try to bring some more bugs (hopefully better examples and more clearly defined)

Hope the sun shines on you all, and have a great day!

-Eric

Comment 37

[Carsten Book [:Tomcat]]

Nicholas, this need approval requests for beta and aurora i think ? :)

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Jason can you request uplift for this fix if Nicholas is out? I'd like to get it into at least aurora.

Comment 39

[Jason Duell]

Comment on attachment 8657343 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: forever
[User impact if declined]: hypothetical security risk (we didn't identify any concrete attacks that work)
[Describe test coverage new/current, TreeHerder]:  tests added
[Risks and why]: could conceivably block some cookies that aren't malware.  But unlikely
[String/UUID change made/needed]: none

Given we haven't seen any concrete attack vectors here I'm not pushing hard for beta here, and wouldn't be put off if we just let this ride the trains. But see comment 21 and comment 23. I'm happy to defer to dveditz.

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8657343 [details] [diff] [review]
patch v2

42 being a special release, I would like to avoid the uplift to 42. It seems that it might generate some noise on the channel.
Anyway, taking it in aurora.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d0a3f5e5adc0

Comment 42

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Eric, does this fix address the use cases you reported?

Comment 43

[todayisnew]

I can check :) which build / version should I check against?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Thanks!  A Firefox 43 beta should be good, given when this was checked in.  https://www.mozilla.org/en-US/firefox/channel/#beta

Comment 45

[todayisnew]

Attachment: 01-fireFox43InvalidCookieValueFixed.png

Comment 46

[todayisnew]

Attachment: 02-fireFox43InvalidCharsInCookieNames.png

Comment 47

[todayisnew]

Attachment: 03- googleValidCookies.png

Comment 48

[todayisnew]

Hello :)

I hope life has treated everyone here well, and the summer has been find if you are in the northern hemisphere :)


1) I have tested and verified the new fix for cookies in build 43, it stores the cookies names as the url encoded versions:

See image: 01-fireFox43InvalidCookieValueFixed.png

2) I did another test and found though we have another potential issue, not sure if I should open a separate bug, or continue with it here.

If we do the same setting of cookies, but instead of the value, we set the cookie name to invalid characters, Firefox will happily set the cookie name values
for characters rfc says it should not, and other browsers do not do (chrome).

FireFox: 02-fireFox43InvalidCharsInCookieNames.png

Chrome: 03-googleValidCookies.png


4)I wrote a tool to test setting cookies for browser if it helps to see:


http://me6.com/xss/cookieTester.php?startChrPosition=<integer start position 0 to 255>&howManyCookiesToSet=<number of cookies to set from start point>&setType=<name or value>

Example:

Start as ascii character 0, set 100 cookies which we will set for the cookie name.

http://me6.com/xss/cookieTester.php?startChrPosition=0&howManyCookiesToSet=100&setType=name


Wish you all well on your side of your screens :)

-Eric

Comment 49

[u408661]

Yeah, the name portion is still a problem. I had a look at other browsers, and the status of compat there is much more consistent than with odd characters in the values. All browsers I tested (with the exception of Firefox) disallow any character 0x1e and below, as well as space (0x20). IE/Edge is the only browser other than Firefox that allows 0x1f in a name. Beyond that, it gets a bit messier, but not too crazy (safari is slightly stricter than everyone else). I think we're safe disallowing characters <= 0x20 to occur in the name. I'll clone this bug and attach a patch to fix it.