Bugzilla@Mozilla – Bug 459906
XSS vulnerability in session restore
Last modified: 2011-06-29 07:27:36 PDT
Summon comment box
User-Agent: Opera/9.60 (X11; Linux x86_64; U; en) Presto/2.1.1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3 It is possible to run code on a different domain due to a race condition in session restore. The attached testcase demonstrates the vulnerability. It worked for me in Firefox 2.0.0.17 and Firefox 3.0.3 on Windows XP. Reproducible: Always
Created attachment 343096 [details] testcase
I cannot get this to repro on 3.0.3 or 2.0.0.17 on XP. If this can be made to work consistently I think severity = sg:high. Even though its mitigated by only working during session restore, you could use a DoS bug to force that condition easily enough. If it also requires some odd timing circumstances or specific load order that I would downgrade it to sg:moderate.
The testcase won't work when loaded over HTTPS.
Also reproduced in Firefox 3.0.3 on Mac OS X 10.5, with no extensions enabled.
The testcase also won't work if you dismiss the "dummy alert" before the target page has at least partially loaded (it needs to have document.body available). An attacker could easily work around this by priming the cache before the crash, and loading a lightweight page on the target site.
I can reproduce in 1.9.0 following those clarifications (I wasn't waiting long enough on the dummy alert). I thought we had killed off this class of bugs with XOWs? I couldn't reproduce in 1.8: the frame never loads mozilla.com until after I dismiss the alert. I was using a debug build in case that makes a difference.
Created attachment 343228 [details] Improved testcase
The improved testcase removes the dummy alert by using sync XMLHttp instead, uses a smaller target page (a .js file that is served as text/html), and primes the cache to make sure the target page will load fast enough. It should reproduce the bug more reliably than the original TC.
Created attachment 343261 [details] Minefield's SessionStore as an extension I can confirm that this bug happens on a clean profile with Firefox 3.0. It doesn't seem to affect the latest Minefield nightlies, though, so this seems to have been inadvertently fixed (at least this specific issue). I've unfortunately got no time for further investigation at the moment. I'm however attaching a version of SessionStore containing all the latest patches which doesn't seem to be affected as a work-around and as a basis for experimentation on Firefox 3.0.
BTW: Is Firefox 3.1 Beta 1 affected as well?
Found it: Bug 450595 fixes this issue for me. That's a four line patch which we can take with hardly any impact at all. Daniel: Please approve attachment #333916 [details] [review] for the 1.9.0 branch and get it landed before the next release!
Created attachment 343846 [details] Updated testcase This testcase shows that Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081019 Minefield/3.1b2pre is still vulnerable.
Thanks, David. AFAICT this issue consists of two bugs: 1. The designMode property isn't correctly wrapped in an XPCNativeWrapper. Checking for that property at http://hg.mozilla.org/mozilla-central/annotate/93111c5c69fd/browser/components/sessionstore/src/nsSessionStore.js#l1235 leads to the testcase's getter to be triggered. 2. Loading http://www.mozilla.com/js/util.js in the first iframe leads to a broken session history structure, where that frame's nsISHContainer still contains the testcase's iframe's iframe - which then inherits the owner of the iframe containing http://www.mozilla.com/js/util.js: mozilla.com. If this is correct, this could be not one Firefox/SessionStore bug but two Core ones. Who's got expertise in these areas?
Actually, I was only close. Here's what happens: 1. The designMode property isn't correctly wrapped in an XPCNativeWrapper. Once the testcase is loaded, it wrongly claims that we've got a Midas component in the first iframe which SessionStore backups by getting the innerHTML property (which contains a further iframe with a script payload). 2. When restoring, we once again check for designMode and restore that innerHTML property. However, before restoring it, the testcase loads a URL of its choice (http://www.mozilla.com/js/util.js) which sets the desired owner - then the restored payload can do as if it were part of mozilla.com. We'll thus have to review Midas restoration for the possibility of getting undesired content injected in innerHTML in SessionStore besides fixing the missing designMode wrapper.
(In reply to comment #14) > 1. The designMode property isn't correctly wrapped in an XPCNativeWrapper. Once > the testcase is loaded, it wrongly claims that we've got a Midas component in > the first iframe which SessionStore backups by getting the innerHTML property > (which contains a further iframe with a script payload). It doesn't even wrongly claim - it just sets designMode = "on". We'll thus have to sanitize innerHTML no matter what (toStaticHTML or similar would be very helpful at this point). The main evil bit is thus that the testcase knows exactly when the iframe's innerHTML is going to be restored through the insufficiently wrapped designMode getter and can then delay us setting it long enough to load the different page first. At this point, we might also want to ensure that the iframe we're setting innerHTML to in fact is the same as where we got it from.
(In reply to comment #15) > (toStaticHTML or similar would be very helpful at this point). Last addendum for tonight: iframes have to be removed as well as script tags, as they might contain javascript/data urls.
Created attachment 343853 [details] [review] improper fix Mailnews uses mozISanitizingHTMLSerializer for sanitizing HTML email. Unfortunately that ones isn't exposed to JavaScript. This patch outlines what will have to be done to secure our careless usage of innerHTML (note: that regex really isn't for production code!). Any suggestions for how to proceed? Do we only take the second part of the patch (which in itself is enough to disable this testcase)? Or do we have a reasonable way of sanitizing HTML from script that I don't know of? Dietrich: This review request is only for the second half of the patch.
Can the location.href check be circumvented in the same way? I'm not exactly sure how XOW's work, but would window.__defineGetter__('location', function() { return { href: "http://www.mozilla.com/js/util.js" }; }) work?
or this: delete this.document; this.__defineGetter__('document', function() { return { location: { href: 'http://www.mozilla.com/js/util.js' } } } )
(In reply to comment #18) Not in my tests. I get the [Exception... "Security error" code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"] I'd have expected for designMode as well. Overwriting the document seems possible though, but even though the getter is called, I get the correct XPCNativeWrapper for the document that I'd expect.
This is sounding too messy to rush into the current set of releases, moving the blocking flags to next time. If there's a breakthrough with a simple, safe patch we might be able to squeeze it in but I think we're better off taking our time and getting it right. This sounds right up moz_bug_r_a4's alley, there might be additional problems in this area as well.
Created attachment 343991 [details] [review] Alternative workaround As Simon found, this is caused by a lack of wrappers. Specifically, we lose the XPCNativeWrapper around 'this' when calling through setTimeout. Because of this, in the call to setTimeout, relying on 'this' to be the same XPCNativeWrapper that aContent was fails and we end up wandering into content code. Fixing this for real is actually somewhat involved for branch because of the path that we go through to call setTimeout functions. I'll look into it a little bit later. For now, this patch is a minimal way check if designMode is on without explicitly constructing an XPCNativeWrapper around 'this'.
The 'let' isn't necessary, fwiw, since aContent stays constant for the life of the function.
(In reply to comment #18) > window.__defineGetter__('location', function() { return { href: > "http://www.mozilla.com/js/util.js" }; }) work? We specially disallow this particular testcase from the days before wrappers when this actually did trip up security checks. See bug 143369. (In reply to comment #19) > this.__defineGetter__('document', function() { return { location: { href: > 'http://www.mozilla.com/js/util.js' } } } ) This would work against us today, but with proper wrapping shouldn't matter since chrome should never see that there's a content getter for document. Instead, XPCNativeWrappers should simply return the native document for that property.
Nice job, mrbkap. --dbloom (I've heard alambert wants to set up #f00f again...)
Note: I filed bug 460882 on fixing setTimeout.
Comment on attachment 343991 [details] [review] Alternative workaround Works great. I'd be fine without the let and with less indentation, but anyway r+=me, thanks. Branch drivers: Virtually risk-less patch (really just replaces an implicit this with the explicit variable name).
Checked in without the let: <http://hg.mozilla.org/mozilla-central/rev/a736074be4dc>.
Comment on attachment 343991 [details] [review] Alternative workaround Moving out to next release so we can get more baking of the patch.
(In reply to comment #29) > Moving out to next release so we can get more baking of the patch. FYI: There's hardly anything to bake with this patch - and most nightly testers will never hit that code path at all (never have), so you'd have to bake for a significant amount of time to be sure that nothing's broken. To verify this patch, make sure that the testcase no longer works and that - when you enter something in http://www.mozilla.org/editor/midasdemo/, close the tab and reopen it - everything is correctly restored.
Created attachment 344552 [details] [review] What I checked in This is what I actually checked in. Carrying forward existing flags.
Comment on attachment 343853 [details] [review] improper fix Marking this obsolete. Simon, is there a followup bug on the first hunk here?
Comment on attachment 344552 [details] [review] What I checked in I agree with Simon - we should take this in 1.9.0.4.
Comment on attachment 344552 [details] [review] What I checked in Approved for 1.8.1.18 and 1.9.0.4, a=dveditz for release-drivers
(In reply to comment #32) > is there a followup bug on the first hunk here? I filed bug 461523 for deciding on what to do about that bit.
Checked into the 1.8 and 1.9 branches.
Verified for 1.8.1.18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008102804 BonEcho/2.0.0.18pre using testcase in comment 0. Verified for 1.9.0.4 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4pre) Gecko/2008102805 GranParadiso/3.0.4pre.
no nsSessionStore.js on 1.8.0 branch.