Last Comment Bug 400556 - [FIX]Vulnerability allows script to see where user is headed, sniff history, and crash [@ nsDocShell::Destroy()] the browser too
: [FIX]Vulnerability allows script to see where user is headed, sniff history, ...
: crash, fixed1.8.0.15, testcase, verified1.8.1.12
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
P1 critical with 1 vote (vote)
: mozilla1.9beta2
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Andrew Overholt [:overholt]
Depends on: 416751
  Show dependency treegraph
Reported: 2007-10-20 16:17 PDT by David Bloom
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
jst: blocking1.9+
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Testcase. (1.24 KB, text/html)
2007-10-20 16:18 PDT, David Bloom
no flags Details
Testcase (1.26 KB, text/html)
2007-10-20 18:12 PDT, David Bloom
no flags Details
Testcase 2: history sniffing (1.60 KB, text/html)
2007-10-21 04:10 PDT, David Bloom
no flags Details
Proposed fix (3.65 KB, patch)
2007-11-23 16:49 PST, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review
Branch merge (3.74 KB, patch)
2007-12-02 18:16 PST, Boris Zbarsky [:bz] (still a bit busy)
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review

Description User image David Bloom 2007-10-20 16:17:40 PDT
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: Gecko/20071008 Firefox/

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 User image David Bloom 2007-10-20 16:18:53 PDT
Created attachment 285619 [details]
Comment 2 User image David Bloom 2007-10-20 17:08:31 PDT
This can also be used to sniff history by using history.go(....).
Comment 3 User image :Gavin Sharp [email:] 2007-10-20 17:54:48 PDT
That testcase doesn't seem to work for me. I tested both a 1.8 branch and trunk build on Mac.
Comment 4 User image David Bloom 2007-10-20 17:56:15 PDT
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 User image :Gavin Sharp [email:] 2007-10-20 17:59:37 PDT
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 User image David Bloom 2007-10-20 18:06:55 PDT
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 User image David Bloom 2007-10-20 18:10:13 PDT
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 User image David Bloom 2007-10-20 18:12:54 PDT
Created attachment 285628 [details]

Disabled fast back/forward so the testcase will work.
Comment 9 User image David Bloom 2007-10-21 04:10:48 PDT
Created attachment 285651 [details]
Testcase 2: history sniffing
Comment 10 User image Martijn Wargers [:mwargers] 2007-10-21 05:18:32 PDT
Confirmed the crash and the ability to see where the user is heading with the first testcase, using current trunk build.
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 User image David Bloom 2007-10-21 07:29:53 PDT
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)*/;
   /* (b)*/ document.write('foo');
   /* (c)*/ getSelection().selectAllChildren(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 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.
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-21 11:47:38 PDT
smaug, aren't those resize handles native anonymous?  Why's this event propagating into user script?
Comment 13 User image David Bloom 2007-10-21 11:57:16 PDT
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 User image David Bloom 2007-10-21 12:05:01 PDT
Sorry, I said resize handles in the original report. I was a little excited, I meant attribute :P
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-21 12:10:22 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-21 13:27:50 PDT
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, 
    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 User image Jesse Ruderman 2007-10-21 15:11:29 PDT
The crash looks exploitable based on the stack trace I get with a Mac trunk debug build.
Comment 18 User image David Bloom 2007-10-22 08:06:09 PDT
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.


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 User image Johnny Stenback (:jst, 2007-10-30 17:25:36 PDT
Boris, can you have a look at this one? If not, let me know...
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-30 19:29:28 PDT
I can try, but probably not for about two weeks...  Got some Nov 15 deadlines here.
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2007-11-23 16:49:06 PST
Created attachment 289983 [details] [diff] [review]
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
* 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 User image Mike Schroepfer 2007-11-30 18:22:53 PST
Peterv you got time to review?
Comment 23 User image David Bloom 2007-11-30 18:32:44 PST
Is the correct address to contact about the bug bounty program? I've tried emailing it twice but have had no response.
Comment 24 User image Boris Zbarsky [:bz] (still a bit busy) 2007-11-30 18:35:21 PST
dveditz, see comment 23?
Comment 25 User image Peter Van der Beken [:peterv] 2007-12-02 15:51:54 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-12-02 18:07:24 PST
Fix checked in on trunk.
Comment 27 User image Boris Zbarsky [:bz] (still a bit busy) 2007-12-02 18:16:34 PST
Created attachment 291144 [details] [diff] [review]
Branch merge
Comment 28 User image Martijn Wargers [:mwargers] 2007-12-04 16:24:53 PST
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 User image Daniel Veditz [:dveditz] 2007-12-17 16:02:24 PST
Comment on attachment 291144 [details] [diff] [review]
Branch merge

approved for, a=dveditz for release-drivers
Comment 30 User image Boris Zbarsky [:bz] (still a bit busy) 2007-12-17 16:27:02 PST
Fixed on branch.
Comment 31 User image Al Billings [:abillings] 2008-01-18 12:45:28 PST
Verified in branch using testcase using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/2008011803 BonEcho/ No capturing of URL, no crash.
Comment 32 User image Alexander Sack 2008-03-12 08:44:08 PDT
blocking1.8.0.15 - distro patch.
Comment 33 User image Alexander Sack 2008-03-12 08:46:05 PDT
Comment on attachment 291144 [details] [diff] [review]
Branch merge

caillon, we ship this in distros already. please sign off.
Comment 34 User image Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-19 08:51:20 PDT
Comment on attachment 291144 [details] [diff] [review]
Branch merge

a=caillon for
Comment 35 User image Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 13:01:16 PDT
Fix committed to the 1.8.0 branch

Note You need to log in before you can comment on or make changes to this bug.