Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
DOM
P1
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: David Bloom, Assigned: bz)

Tracking

(4 keywords)

Trunk
mozilla1.9beta2
crash, fixed1.8.0.15, testcase, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 285619 [details]
Testcase.
(Reporter)

Updated

10 years ago
OS: Mac OS X → All
(Reporter)

Comment 2

10 years ago
This can also be used to sniff history by using history.go(....).
(Reporter)

Updated

10 years ago
Summary: Vulnerability allows script to see where user is headed when they leave the page, and crash the browser too → Vulnerability allows script to see where user is headed, sniff history, and crash the browser too
Component: Security → DOM
Product: Firefox → Core
QA Contact: firefox → general
That testcase doesn't seem to work for me. I tested both a 1.8 branch and trunk build on Mac.
(Reporter)

Comment 4

10 years ago
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.
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.
(Reporter)

Comment 6

10 years ago
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. :)
(Reporter)

Comment 7

10 years ago
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.
(Reporter)

Comment 8

10 years ago
Created attachment 285628 [details]
Testcase 

Disabled fast back/forward so the testcase will work.
Attachment #285619 - Attachment is obsolete: true
(Reporter)

Comment 9

10 years ago
Created attachment 285651 [details]
Testcase 2: history sniffing
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
Flags: blocking1.8.1.9?
Keywords: crash, testcase
Summary: Vulnerability allows script to see where user is headed, sniff history, and crash the browser too → Vulnerability allows script to see where user is headed, sniff history, and crash [@ nsDocShell::Destroy()] the browser too
Version: unspecified → Trunk
(Reporter)

Comment 11

10 years ago
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)
(Assignee)

Comment 12

10 years ago
smaug, aren't those resize handles native anonymous?  Why's this event propagating into user script?
(Reporter)

Comment 13

10 years ago
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...
(Reporter)

Comment 14

10 years ago
Sorry, I said resize handles in the original report. I was a little excited, I meant attribute :P
(Assignee)

Comment 15

10 years ago
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).
(Assignee)

Comment 16

10 years ago
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

10 years ago
The crash looks exploitable based on the stack trace I get with a Mac trunk debug build.
Whiteboard: [sg:critical]
(Reporter)

Comment 18

10 years ago
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.
Boris, can you have a look at this one? If not, let me know...
Assignee: nobody → bzbarsky
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 20

10 years ago
I can try, but probably not for about two weeks...  Got some Nov 15 deadlines here.
(Assignee)

Updated

10 years ago
Priority: -- → P1
(Assignee)

Comment 21

10 years ago
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
  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?
Attachment #289983 - Flags: superreview?(peterv)
Attachment #289983 - Flags: review?(peterv)
(Assignee)

Updated

10 years ago
Summary: 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, and crash [@ nsDocShell::Destroy()] the browser too
Target Milestone: --- → mozilla1.9 M10

Comment 22

10 years ago
Peterv you got time to review?
(Reporter)

Comment 23

10 years ago
Is security@mozilla.org the correct address to contact about the bug bounty program? I've tried emailing it twice but have had no response.
(Assignee)

Comment 24

10 years ago
dveditz, see comment 23?
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.
Attachment #289983 - Flags: superreview?(peterv)
Attachment #289983 - Flags: superreview+
Attachment #289983 - Flags: review?(peterv)
Attachment #289983 - Flags: review+
(Assignee)

Comment 26

10 years ago
Fix checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
(Assignee)

Comment 27

10 years ago
Created attachment 291144 [details] [diff] [review]
Branch merge
Attachment #291144 - Flags: approval1.8.1.12?
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Whiteboard: [sg:critical] → [sg:critical?]
Comment on attachment 291144 [details] [diff] [review]
Branch merge

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #291144 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Comment 30

10 years ago
Fixed on branch.
Keywords: fixed1.8.1.12
Flags: wanted1.8.1.x+
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12
Group: security
Depends on: 416751

Comment 32

10 years ago
blocking1.8.0.15 - distro patch.
Flags: blocking1.8.0.15+

Comment 33

10 years ago
Comment on attachment 291144 [details] [diff] [review]
Branch merge

caillon, we ship this in distros already. please sign off.
Attachment #291144 - Flags: approval1.8.0.15?
Comment on attachment 291144 [details] [diff] [review]
Branch merge

a=caillon for 1.8.0.15
Attachment #291144 - Flags: approval1.8.0.15? → approval1.8.0.15+
Fix committed to the 1.8.0 branch
Keywords: fixed1.8.0.15
Crash Signature: [@ nsDocShell::Destroy()]
You need to log in before you can comment on or make changes to this bug.