459906 - (CVE-2008-5019) XSS vulnerability in session restore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[David Bloom]

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

Comment 1

[David Bloom]

Attachment: testcase

Comment 2

[Lucas Adamski [:ladamski]]

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.

Comment 3

[David Bloom]

The testcase won't work when loaded over HTTPS.

Comment 4

[David Bloom]

Also reproduced in Firefox 3.0.3 on Mac OS X 10.5, with no extensions enabled.

Comment 5

[David Bloom]

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.

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[David Bloom]

Attachment: Improved testcase

Comment 8

[David Bloom]

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.

Comment 9

[Simon Bünzli]

Attachment: 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.

Comment 10

[Simon Bünzli]

BTW: Is Firefox 3.1 Beta 1 affected as well?

Comment 11

[Simon Bünzli]

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] [diff] [review] for the 1.9.0 branch and get it landed before the next release!

Comment 12

[David Bloom]

Attachment: 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.

Comment 13

[Simon Bünzli]

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?

Comment 14

[Simon Bünzli]

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.

Comment 15

[Simon Bünzli]

(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.

Comment 16

[Simon Bünzli]

(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.

Comment 17

[Simon Bünzli]

Attachment: 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.

Comment 18

[David Bloom]

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?

Comment 19

[David Bloom]

or this:
delete this.document;
this.__defineGetter__('document', function() { return { location: { href: 'http://www.mozilla.com/js/util.js' } } } )

Comment 20

[Simon Bünzli]

(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.

Comment 21

[Daniel Veditz [:dveditz]]

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.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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'.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

The 'let' isn't necessary, fwiw, since aContent stays constant for the life of the function.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 25

[David Bloom]

Nice job, mrbkap.

--dbloom

(I've heard alambert wants to set up #f00f again...)

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Note: I filed bug 460882 on fixing setTimeout.

Comment 27

[Simon Bünzli]

Comment on attachment 343991 [details] [diff] [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).

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Checked in without the let: <http://hg.mozilla.org/mozilla-central/rev/a736074be4dc>.

Comment 29

[Samuel Sidler (old account; do not CC)]

Comment on attachment 343991 [details] [diff] [review]
Alternative workaround

Moving out to next release so we can get more baking of the patch.

Comment 30

[Simon Bünzli]

(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.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I checked in

This is what I actually checked in. Carrying forward existing flags.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 343853 [details] [diff] [review]
improper fix

Marking this obsolete. Simon, is there a followup bug on the first hunk here?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 344552 [details] [diff] [review]
What I checked in

I agree with Simon - we should take this in 1.9.0.4.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 344552 [details] [diff] [review]
What I checked in

Approved for 1.8.1.18 and 1.9.0.4, a=dveditz for release-drivers

Comment 35

[Simon Bünzli]

(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.

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.8 and 1.9 branches.

Comment 37

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 38

[Alexander Sack]

no nsSessionStore.js on 1.8.0 branch.