858215 - (CVE-2013-6167) document.cookie DOS

Status: RESOLVED DUPLICATE of bug 1797235

(Core :: Networking: Cookies, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Date: Wed, 3 Apr 2013 09:02:53 -0700
From: Reed Loden <[e-mail]>
To: [e-mail]
Subject: Fw: [oss-security] browser document.cookie DoS vulnerability
-----//-----
Is this filed?

Begin forwarded message:

Date: Wed, 3 Apr 2013 17:58:18 +0200
From: Stefan Bühler <[e-mail]>
To: [e-mail]
Subject: [oss-security] browser document.cookie DoS vulnerability


Hi,

Chromium 25.0.1364.160 (debian testing), Iceweasel/Firefox 19 and
probably many other browsers allow javascript to set broken cookie
values, leading to possible permanent "400 Bad Request" responses. The
broken value might be set by 3rd party libraries.

For example the google analytics code is vulnerable, as it sets cookie
values based on parameters in the referer query string. lighttpd does
not allow control characters in http header values, so any lighttpd
site using google analytics is vulnerable if you can get the user to
follow a link (img tag for example) to that site like this:

http://www.example.com/?utm_source=test&utm_medium=test&utm_campaign=te%05st


Afaik apache doesn't check the cookie values (or perhaps removes the
broken characters). Imho they are responsible for this mess :)

To be clear: the bug is in the browser / javascript implementation:

document.cookie MUST NOT allow cookie values which include certain
control characters. Javascript applications should not use 8-bit
characters.

(If browser vendors want to allow broken cookie values to be stored,
they MUST NOT send them to the server; in this case javascript
applications can still read the broken values. But I don't think this
is a good idea.)

The safe character set for HTTP header values is: %x20-7E; %x80-FF is
obsoleted by the current draft.

"A recipient MAY replace any linear white space with a single SP before
interpreting the field value", so horizontal tabs are not "safe" - they
might get converted to a space, but are not forbidden (also multiple
spaces can get replaced by a single one).

I think this could use a CVE.

The problem was reported in our lighttpd bug tracker:
http://redmine.lighttpd.net/issues/2188

Kind regards,
Stefan


Testing the bug:

Try one of the listed urls in the ticket (the error should trigger
after a reload). If you have noscript, request policy, referer control
or similar stuff running you are probably safe; to test the bug in this
case you need a Javascript console on a lighttpd site
(http://lighttpd.net for example), and enter:

document.cookie = "foo=bar\x05test"
Try to reload the page - it should return a 400 Bad Request page now.

document.cookie = "foo="
And it should work again.


HTTP references:

http://tools.ietf.org/html/rfc2616
message-header = field-name ":" [ field-value ]
field-name     = token
field-value    = *( field-content | LWS )
field-content  = *TEXT | *(token | separators | quoted-string)
LWS            = [CRLF] 1*( SP | HTAB )

# TEXT is superset of (token | separators | quoted-string)
TEXT = LWS | %x21-7E | %x80-FF

http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-22
header-field   = field-name ":" OWS field-value BWS
field-name     = token
field-value    = *( field-content / obs-fold )
field-content  = *( HTAB / SP / VCHAR / obs-text )
obs-fold       = CRLF ( SP / HTAB )

# obsolete text
obs-text = %x80-FF

Basic definitions:
# horizontal tab
HTAB = %x09
# space
SP = %x20
# visible ASCII
VCHAR = %x21-7E
# carriage return + line feed
CRLF = %0x0D %0x0A

Comment 1

[Benjamin Smedberg]

The "vulnerability" here is just a DOS attack, right? You can't actually go creating a well-formed request with arbitrary HTTP headers or anything like that?

This sounds like a sec-dos that we should just make public.

I suspect that throwing when a bad character was added via the DOM would be a compatibility nightmare, but could maybe try %-escaping the disallowed characters or something...

Comment 2

[Reed Loden [:reed]]

It was publicly filed on full-disclosure@, so opening anyway. Preemptively marking sec-low, csec-dos.

Comment 3

[Daniel Kahn Gillmor]

chromium just fixed this bug in their browser:

 https://src.chromium.org/viewvc/chrome?revision=224268&view=revision
 https://code.google.com/p/chromium/issues/detail?id=238041

firefox should probably also fix it.

Comment 4

[brook]

Attachment: cookiefix.patch

Comment 5

[brook]

I reproduced the bug loading a certain web page with the URL /?utm_source=test&utm_medium=test&utm_campaign=te%05st to create the DOS cookie. Attempt to load any page on that domain fails from then on.

I wrote a fix that rejects the DOS cookies which contain control codes similar to the fix that chrome uses http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/parsed_cookie.cc?r1=224268&r2=224267&pathrev=224268

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8421146 [details] [diff] [review]
cookiefix.patch

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

Great! Sorry for sitting on this for so long; would you mind adding some tests for this behaviour, too? http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/test/unit/test_parser_0001.js should be a good example to follow; you can run it via ./mach xpcshell-test netwerk/cookie/test/unit/test_parser_0001.js

::: netwerk/cookie/nsCookieService.cpp
@@ +2892,5 @@
>    if ((cookieAttributes.name.Length() + cookieAttributes.value.Length()) > kMaxBytesPerCookie) {
>      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader, "cookie too big (> 4kb)");
>      return newCookie;
>    }
> +  

nit: trailing whitespace

@@ +2897,5 @@
>    if (cookieAttributes.name.FindChar('\t') != kNotFound) {
>      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader, "invalid name character");
>      return newCookie;
>    }
> +  

nit: trailing whitespace

@@ +2903,5 @@
> +  if (ContainsControlCharacters(cookieAttributes.name) || ContainsControlCharacters(cookieAttributes.value)) {
> +    COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader, "invalid control characters");
> +    return newCookie;
> +  }
> +  

nit: trailing whitespace

@@ +3421,5 @@
> +// which might cause problems if set in a cookie
> +bool
> +nsCookieService::ContainsControlCharacters(const nsCString &s)
> +{
> +  unsigned int i;

nsCString::index_type instead of unsigned int

Comment 7

[Josh Matthews [:jdm]]

Brook, are you planning to keep working on this?

Comment 8

[Shian-Yow Wu [:swu]]

The current code already disallowed illegal characters in cookies when it's set via HTTP(bug 1191423).
https://dxr.mozilla.org/mozilla-central/rev/1806d405c8715949b39fa3a4fc142d14a60df590/netwerk/cookie/nsCookieService.cpp#3263

However, the issue is still reproducible if the cookie is set via console by document.cookie = "foo=bar\x05test".  Nickolas, I cannot access bug 1191423 to know the detail, is it on purpose to disallow illegal characters in cookies only when it's set via HTTP?

Comment 9

[u408661]

(In reply to Shian-Yow Wu [:swu] from comment #8)
> The current code already disallowed illegal characters in cookies when it's
> set via HTTP(bug 1191423).
> https://dxr.mozilla.org/mozilla-central/rev/
> 1806d405c8715949b39fa3a4fc142d14a60df590/netwerk/cookie/nsCookieService.
> cpp#3263
> 
> However, the issue is still reproducible if the cookie is set via console by
> document.cookie = "foo=bar\x05test".  Nickolas, I cannot access bug 1191423
> to know the detail, is it on purpose to disallow illegal characters in
> cookies only when it's set via HTTP?

Yes, that choice was made purposefully (given breakage of tests indicating potential real-world breakage) if we also disable via document.cookie. Additionally, in testing, other browsers behaved the same way, so this has compatibility concerns, as well.

Comment 10

[Shian-Yow Wu [:swu]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #9)
> Yes, that choice was made purposefully (given breakage of tests indicating
> potential real-world breakage) if we also disable via document.cookie.
> Additionally, in testing, other browsers behaved the same way, so this has
> compatibility concerns, as well.

Thanks, Nicholas!  Based on this fact, shall we close this bug as RESOLVED/WONTFIX?

Comment 11

[u408661]

That seems reasonable to me.

Comment 12

[Shian-Yow Wu [:swu]]

Close this bug based on comment 9.  Feel free to re-open if needed.

Comment 14

[Masato Kinugawa]

FYI, recently, Chrome patched this bug completely: https://bugs.chromium.org/p/chromium/issues/detail?id=1174647#c9
Also, Safari already doesn't store control characters in Cookie.
In modern browsers, only Firefox stores them via document.cookie and still has this DoS issue. I think it's time to re-open this issue.