400556 - [FIX]Vulnerability allows script to see where user is headed, sniff history, and crash [@ nsDocShell::Destroy()] the browser too

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[David Bloom]

User-Agent:       Opera/9.50 (Macintosh; Intel Mac OS X; U; en)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8

See testcase.

The problem is that the resize handles on the designMode <IMG> are firing DOM mutation events after location.href has been updated.

I couldn't pull off any cross-site-scripting or spoofing with this, but I'm not intimately familiar with the browser's architecture...

Reproducible: Always

Steps to Reproduce:
See testcase.
Actual Results:  
The URL I was headed to was detected.
The browser can crash (if you give the testcase permission).

Expected Results:  
No detection of what URL I entered in the address bar to leave the page.
No crash.

Comment 1

[David Bloom]

Attachment: Testcase.

Comment 2

[David Bloom]

This can also be used to sniff history by using history.go(....).

Comment 3

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

That testcase doesn't seem to work for me. I tested both a 1.8 branch and trunk build on Mac.

Comment 4

[David Bloom]

Gavin: It's working for me on my OS X 10.4.10 machine and on my Windows machine...I also have all extensions (except trackback and DOM inspector) disabled.

Comment 5

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

Just to be clear, you're testing with the attached testcase, right? Could be a timing dependent issue, I guess... Still can't reproduce with a 1.8 branch build and new profile, on Mac.

Comment 6

[David Bloom]

Strange, I can't repro on a clean profile either...

I'm going to see if its an extension problem...I'll follow up in a bit. :)

Comment 7

[David Bloom]

It wasn't working because of the fast back/forward cache.

I'll make a new testcase with a dummy onunload to work around that.

Comment 8

[David Bloom]

Attachment: Testcase

Disabled fast back/forward so the testcase will work.

Comment 9

[David Bloom]

Attachment: Testcase 2: history sniffing

Comment 10

[Martijn Wargers (dead)]

Confirmed the crash and the ability to see where the user is heading with the first testcase, using current trunk build.

http://crash-stats.mozilla.com/report/index/17a352b5-7fca-11dc-a8ff-001a4bd43ed6
0  	xul.dll@0x695dd4  	
1 	nsDocShell::Destroy() 	mozilla/docshell/base/nsDocShell.cpp:3537
2 	nsFrameLoader::Destroy() 	mozilla/content/base/src/nsFrameLoader.cpp:265
3 	nsGenericHTMLFrameElement::UnbindFromTree(int, int) 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:3048
4 	nsGenericElement::UnbindFromTree(int, int) 	mozilla/content/base/src/nsGenericElement.cpp:2132
5 	nsGenericHTMLElement::UnbindFromTree(int, int) 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:1177
6 	nsHTMLBodyElement::UnbindFromTree(int, int) 	mozilla/content/html/content/src/nsHTMLBodyElement.cpp:477
7 	nsGenericElement::UnbindFromTree(int, int) 	mozilla/content/base/src/nsGenericElement.cpp:2132
8 	nsGenericHTMLElement::UnbindFromTree(int, int) 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:1177
9 	nsDocument::Destroy() 	mozilla/content/base/src/nsDocument.cpp:5588
10 	DocumentViewerImpl::Close(nsISHEntry*) 	mozilla/layout/base/nsDocumentViewer.cpp:1297

Comment 11

[David Bloom]

Here are some fun behaviors I've found while toying around with this bug:
1. Things that will crash Firefox in the attribute mutation event handler:
   /* (a)*/ document.open();
   /* (b)*/ document.write('foo');
   /* (c)*/ getSelection().selectAllChildren(document.body);
            getSelection().getRangeAt(0).surroundContents(window.document.body);
   // (c) doesn't seem to crash until after script execution ends.
2. Doing this in the event handler will cause Firefox to remove the last character from the HTTP request:
   location.hash = 'foo';
   location.hash = '';
   // (Doing this when going to the root path of a domain gives a 404 for the
   //  path "/1.1" :-D)
3. Even if Firebug is installed, console.log inside the event handler will throw an error that "console" is undefined.
4. Synchronous XMLHttpRequests work within the event handler. Even though the value of location.href is from the destination page, the same-origin policy still recognizes XMLHttpRequest as coming from the page launching the exploit. An attacker could still find this useful, however, to send the sniffed location.href value to the attacker's server.

(WARNING: This next paragraph consists of me talking about an area of computer security I know very little about...)
The crashers tend to be "Reason: KERN_INVALID_ADDRESS at address: 0x3286252a" (OS X Intel, Firefox 2.0.0.8). Sometimes it is a different memory address (and sometimes a different exception), but I haven't found out a way to consistently crash at the same, but non-0x3286252a, memory address. So, *as far as I can tell*, this vulnerability does not allow an attacker to execute code at an "arbitrary" memory address.
(/WARNING)

Comment 12

[Boris Zbarsky [:bzbarsky]]

smaug, aren't those resize handles native anonymous?  Why's this event propagating into user script?

Comment 13

[David Bloom]

The handles are indeed anonymous. The problem is that there is also a "_moz_resizing" attribute on the <IMG> that isn't anonymous.

So, I am not actually doing anything to any anonymous nodes (or even the attribute node). The vulnerability exists because the "_moz_resizing" attribute is removed when the user leaves the page, and location.href is updated before the attribute is removed. My exploit runs in a DOM attribute modified mutation event listener that is called. (Basically, it is after onunload, and after the browser has already completed some other parts of the navigation).

By the way, the location.href exposed is not the explicit URL entered into the address bar, it is actaully the location after any HTTP redirects. So, websites that HTTP redirect to a URL containing a session ID could be vulnerable to session hijacking as a result...

Comment 14

[David Bloom]

Sorry, I said resize handles in the original report. I was a little excited, I meant attribute :P

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Yeah, so we need to tear down editor stuff earlier in the page unload process.  Much earlier.  Right about when we fire the unload event, in fact, since that's the last place where we guarantee that:

1)  The URL is still the old URL.
2)  It's safe to execute script (e.g. the unload event script).

Comment 16

[Boris Zbarsky [:bzbarsky]]

It looks like the stack to editor teardown is the following:

#40 0xb1e062a3 in nsEditingSession::TearDownEditorOnWindow (this=0x8ab0170, 
    aWindow=0x899fb18, aStopEditing=1)
    at ../../../../mozilla/editor/composer/src/nsEditingSession.cpp:659
#41 0xb55b39f7 in nsDocShellEditorData::~nsDocShellEditorData ()
    at ../../../dist/include/xpcom/nsTArray.h:64
#42 0xb5578d39 in nsDocShell::Destroy (this=0x8a96558)
    at ../../../mozilla/docshell/base/nsDocShell.cpp:3537
#43 0xb4b5c982 in nsFrameLoader::Destroy (this=0x8b0b230)
    at ../../../../mozilla/content/base/src/nsFrameLoader.cpp:265
#44 0xb4c0709f in nsGenericHTMLFrameElement::UnbindFromTree (this=0x89d4370, aDeep=1, 
    aNullParent=0)
    at ../../../../../mozilla/content/html/content/src/nsGenericHTMLElement.cpp:3048
...
#50 0xb4b48c58 in nsDocument::Destroy (this=0x8a1fe88)
    at ../../../../mozilla/content/base/src/nsDocument.cpp:5591
#51 0xb489dde1 in DocumentViewerImpl::Close (this=0x8b17968, aSHEntry=0x0)
    at ../../../mozilla/layout/base/nsDocumentViewer.cpp:1297

I wonder whether it would make sense to do that release of the editor data off an event.  In this case it would hopefully mean that it would run when the location object not only has a different URI but also different permissions (such that you can't read the new location).  And it'd be safe to run scripts at that point.

The other option is to make sure that all editor teardowns happen when firing unload, as I said.

Peter, do you have a preference?

Comment 17

[Jesse Ruderman]

The crash looks exploitable based on the stack trace I get with a Mac trunk debug build.

Comment 18

[David Bloom]

I'm not an expert on Flash, but it may be possible to use this bug to trick the Flash plugin into thinking that it is running on a different domain.

See: http://my.opera.com/hallvors/blog/2007/10/22/a-malicious-thought-how-to-imagine-a-security-issue

Because the Flash plugin checks window.location to determine the URL, and this vulnerability causes window.location to be changed, it may be possible to load a Flash file inside the DOMAttrModified event handler, which would trick the Flash file into thinking it was running in the new window.location.

Comment 19

[Johnny Stenback (:jst)]

Boris, can you have a look at this one? If not, let me know...

Comment 20

[Boris Zbarsky [:bzbarsky]]

I can try, but probably not for about two weeks...  Got some Nov 15 deadlines here.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This patch makes sure to tear down the editor when we fire pagehide/unload.

Compared to what happens without this patch:

* When navigating in the docshell that's currently editable, we tear down the
  editor when we start the new document load (significantly before the code I'm
  adding).
* When navigating in an ancestor of the docshell that's currently editable
  there are two sub-cases:
    - We don't bfcache.  Then we'll Destroy() the subframe, which would trigger
      editor destruction anyway, but too late.
    - We bfcache.  Then we wouldn't destroy the editor for now, but would tear
      it down on restore (when we add the document request to the loadgroup,
      triggering the same code as the "navigate away" case).

So in all cases, this patch doesn't change whether the editor ends up torn down, only when.

If desired, I could tear down only on unload, not on pagehide.  That would make it easier to fix editor working when going back to a bfcached document, if we decide to fix that.  Peter, let me know, ok?

Comment 22

[Mike Schroepfer]

Peterv you got time to review?

Comment 23

[David Bloom]

Is [e-mail] the correct address to contact about the bug bounty program? I've tried emailing it twice but have had no response.

Comment 24

[Boris Zbarsky [:bzbarsky]]

dveditz, see comment 23?

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 289983 [details] [diff] [review]
Proposed fix

Let's go with this for now. We can always move from pagehide to unload when we need it.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fix checked in on trunk.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch merge

Comment 28

[Martijn Wargers (dead)]

Verified fixed with both testcases, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120405 Minefield/3.0b2pre

The first testcase doesn't crash anymore, and with the second testcase, it isn't possible anymore to sniff history.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 291144 [details] [diff] [review]
Branch merge

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 31

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

Verified in branch using testcase using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre. No capturing of URL, no crash.

Comment 32

[Alexander Sack]

blocking1.8.0.15 - distro patch.

Comment 33

[Alexander Sack]

Comment on attachment 291144 [details] [diff] [review]
Branch merge

caillon, we ship this in distros already. please sign off.

Comment 34

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 291144 [details] [diff] [review]
Branch merge

a=caillon for 1.8.0.15

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix committed to the 1.8.0 branch