1.26 KB, text/html
1.60 KB, text/html
3.65 KB, patch
|Details | Diff | Splinter Review|
3.74 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail): approval1.8.0.next+
|Details | Diff | Splinter Review|
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:188.8.131.52) Gecko/20071008 Firefox/184.108.40.206 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.
This can also be used to sniff history by using history.go(....).
That testcase doesn't seem to work for me. I tested both a 1.8 branch and trunk build on Mac.
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.
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. :)
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.
Created attachment 285628 [details] Testcase Disabled fast back/forward so the testcase will work.
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
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 220.127.116.11). 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)
smaug, aren't those resize handles native anonymous? Why's this event propagating into user script?
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...
Sorry, I said resize handles in the original report. I was a little excited, I meant attribute :P
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).
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?
The crash looks exploitable based on the stack trace I get with a Mac trunk debug build.
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...
I can try, but probably not for about two weeks... Got some Nov 15 deadlines here.
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?
Peterv you got time to review?
Is firstname.lastname@example.org the correct address to contact about the bug bounty program? I've tried emailing it twice but have had no response.
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.
Fix checked in on trunk.
Created attachment 291144 [details] [diff] [review] Branch merge
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 on attachment 291144 [details] [diff] [review] Branch merge approved for 18.104.22.168, a=dveditz for release-drivers
Fixed on branch.
Verified in branch using testcase using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:22.214.171.124pre) Gecko/2008011803 BonEcho/126.96.36.199pre. No capturing of URL, no crash.
blocking188.8.131.52 - distro patch.
Comment on attachment 291144 [details] [diff] [review] Branch merge caillon, we ship this in distros already. please sign off.
Comment on attachment 291144 [details] [diff] [review] Branch merge a=caillon for 184.108.40.206
Fix committed to the 1.8.0 branch