Last Comment Bug 428672 - XSS using an event handler attached to the outer window
: XSS using an event handler attached to the outer window
[sg:high xss] fixed on branch by 432591
: verified1.8.1.15
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla1.9
Assigned To: Blake Kaplan (:mrbkap)
: David Keeler [:keeler] (use needinfo?)
Depends on:
  Show dependency treegraph
Reported: 2008-04-12 08:17 PDT by moz_bug_r_a4
Modified: 2008-08-17 17:02 PDT (History)
10 users (show)
mbeltzner: blocking1.9+
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+ in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed fix v1 (1.09 KB, patch)
2008-04-24 13:47 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Updated to comments (853 bytes, patch)
2008-04-24 16:46 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
mrbkap: superreview+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
stack (3.03 KB, text/plain)
2008-04-28 04:16 PDT, :Gavin Sharp [email:]
no flags Details
With 100% less crashing (904 bytes, patch)
2008-04-28 13:14 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
Now with 100% more bugs fixed (1017 bytes, patch)
2008-04-29 12:13 PDT, Blake Kaplan (:mrbkap)
jst: review-
jst: superreview-
Details | Diff | Splinter Review
More targeted fix (1020 bytes, patch)
2008-05-01 15:07 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
mtschrep: approval1.9+
Details | Diff | Splinter Review
Wrong diff. (7.29 KB, patch)
2008-06-03 17:45 PDT, Johnny Stenback (:jst,
no flags Details | Diff | Splinter Review

Description moz_bug_r_a4 2008-04-12 08:17:41 PDT
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.
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-14 15:11:26 PDT
Dan: should we be fixing this right away? We'll definitely take it if you're taking it for the next branch release ...
Comment 3 Daniel Veditz [:dveditz] 2008-04-20 10:15:26 PDT
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
Comment 4 Blake Kaplan (:mrbkap) 2008-04-24 13:47:24 PDT
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 5 Johnny Stenback (:jst, 2008-04-24 15:52:41 PDT
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.
Comment 6 Blake Kaplan (:mrbkap) 2008-04-24 16:46:54 PDT
Created attachment 317626 [details] [diff] [review]
Updated to comments

This fixes a security bug.
Comment 7 Johnny Stenback (:jst, 2008-04-24 16:47:07 PDT
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 8 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-24 21:07:28 PDT
Comment on attachment 317626 [details] [diff] [review]
Updated to comments

Comment 9 :Gavin Sharp [email:] 2008-04-28 03:13:00 PDT
mozilla/content/base/src/nsDocument.cpp 	3.825 
Comment 10 :Gavin Sharp [email:] 2008-04-28 04:16:09 PDT
This caused crashes in mochitest and the browser chrome test, consistently on 3 platforms, so I had to back it out.

Comment 11 :Gavin Sharp [email:] 2008-04-28 04:16:40 PDT
Created attachment 318125 [details]

I reproduced this crash locally, here's the stack.
Comment 12 :Gavin Sharp [email:] 2008-04-28 04:18:48 PDT
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
Comment 13 Blake Kaplan (:mrbkap) 2008-04-28 13:14:47 PDT
Created attachment 318219 [details] [diff] [review]
With 100% less crashing
Comment 14 Blake Kaplan (:mrbkap) 2008-04-28 23:18:52 PDT
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!
Comment 15 Blake Kaplan (:mrbkap) 2008-04-28 23:40:33 PDT
(I ran the latest patch through mochitests successfully)
Comment 16 moz_bug_r_a4 2008-04-29 02:12:28 PDT
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 18 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-29 07:04:38 PDT
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?
Comment 19 Blake Kaplan (:mrbkap) 2008-04-29 12:13:52 PDT
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?
Comment 20 Johnny Stenback (:jst, 2008-04-29 17:49:27 PDT
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?
Comment 21 Blake Kaplan (:mrbkap) 2008-04-29 19:19:58 PDT
The caller is nsGenericHTMLElement::GetEventListenerManagerForAttr. I can try pushing the check there.
Comment 22 Blake Kaplan (:mrbkap) 2008-05-01 15:07:42 PDT
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.
Comment 23 Brian Crowder 2008-05-03 21:29:31 PDT
nsGenericHTMLElement.cpp: 1.766
Comment 24 Samuel Sidler (old account; do not CC) 2008-06-02 21:32:26 PDT
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.
Comment 25 Johnny Stenback (:jst, 2008-06-03 15:33:41 PDT
This problem is fixed on the 1.8 branch by the fix for bug 432591.
Comment 26 Johnny Stenback (:jst, 2008-06-03 17:45:53 PDT
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.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2008-06-03 18:29:51 PDT
jst, that doesn't look like a patch for this bug (but for the subscript loader one).
Comment 28 Johnny Stenback (:jst, 2008-06-04 12:58:53 PDT
Comment on attachment 323646 [details] [diff] [review]
Wrong diff.

Yeah, duh.
Comment 29 Johnny Stenback (:jst, 2008-06-05 11:12:21 PDT
Fixed by the checkin for bug 432591.
Comment 30 Al Billings [:abillings] 2008-06-10 18:07:08 PDT
Verified testcase 1 (the only one that works in branch) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008061005 BonEcho/ for release.

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