If an event handler attribute is set on <body> element in a document that has already been unloaded, an event handler function is attached to the outer window. This allows an attacker to perform an XSS attack.
Dan: should we be fixing this right away? We'll definitely take it if you're taking it for the next branch release ...
Would be better to get this into the FF3 release than have it be the first FF3.0.1 advisory, but if it's tricky/risky then we can live with 126.96.36.199
Created attachment 317603 [details] [diff] [review] Proposed fix v1 The underlying problem here is that the docshell's idea of what a script global object should be is an outer window where the document wants an inner window. I'm not sure if there's a better way to go from an outer nsIScriptGlobalObject -> inner nsIScriptGlobalObject (without the 2 QIs) or if I should be using "GetCurrentInnerWindow" instead of ensuring one.
Comment on attachment 317603 [details] [diff] [review] Proposed fix v1 Could we get away with calling GetCurrentInnerWindow() here instead to avoid potentially creating a new inner window for a closed window or what not? r+sr=jst either way.
Created attachment 317626 [details] [diff] [review] Updated to comments This fixes a security bug.
Comment on attachment 317603 [details] [diff] [review] Proposed fix v1 Given that this is a security bug I think we should try to get this in for RC1...
Comment on attachment 317626 [details] [diff] [review] Updated to comments a1.9=beltzner
This caused crashes in mochitest and the browser chrome test, consistently on 3 platforms, so I had to back it out.
And I did hit the assertion that was added: ###!!! ASSERTION: What kind of global object do we have?: 'pwin', file /Users/gavin/moz/mozilla/content/base/src/nsDocument.cpp, line 2569 ###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../dist/include/xpcom/nsCOMPtr.h, line 868
Created attachment 318219 [details] [diff] [review] With 100% less crashing
Comment on attachment 318219 [details] [diff] [review] With 100% less crashing Let's try this again. This time, there are no null pointer dereferences lurking!
(I ran the latest patch through mochitests successfully)
I tested with the latest patch. It's still exploitable. On trunk, it's possible to attach an event handler to a cross-origin window.
Comment on attachment 318219 [details] [diff] [review] With 100% less crashing a=beltzner, can we make that testcase a test and include that as well?
Created attachment 318447 [details] [diff] [review] Now with 100% more bugs fixed Stealing this back. I was worried about this when I attached the first patch. This should fix all variants of the bug, but now I find myself wondering why we bother looking at the docshell at all in nsDocument::GetScriptGlobalObject. Once ours has been cleared out, we can only ever return an outer window or the *next* inner window in the succession, which is likely to be wrong. jst, do you remember why we do this?
I don't remember details, but I know it's not pretty and not something we can change this late in the game. This code came in as part of bug 141295, and I know I've removed it in the past (never got checked in, broke stuff). What's the actual caller of nsDocument::GetScriptGlobalObject() that matters in this case? Can we move the document check there until we have a chance to clean this up?
The caller is nsGenericHTMLElement::GetEventListenerManagerForAttr. I can try pushing the check there.
Created attachment 318906 [details] [diff] [review] More targeted fix I filed bug 431767 on figuring out what nsDocument::GetScriptGlobalObject should do after the document has been removed from its docshell.
Johnny, do you have time to make a branch patch for this bug since Blake is out? The patch in comment 22 seems pretty simple, but I'm not sure if it applies to the branch.
This problem is fixed on the 1.8 branch by the fix for bug 432591.
Created attachment 323646 [details] [diff] [review] Wrong diff. This is the same as the trunk version except for the change to NS_OpenURI() in nsNetUtils.h which was needed by this fix.
jst, that doesn't look like a patch for this bug (but for the subscript loader one).
Comment on attachment 323646 [details] [diff] [review] Wrong diff. Yeah, duh.
Fixed by the checkin for bug 432591.
Verified testcase 1 (the only one that works in branch) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) Gecko/2008061005 BonEcho/184.108.40.206pre for 220.127.116.11 release.