Last Comment Bug 432591 - Fix for bug 428672 can be circumvented by using XUL element
: Fix for bug 428672 can be circumvented by using XUL element
: verified1.8.1.15
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst,
: David Keeler [:keeler] (use needinfo?)
Depends on: 431767
  Show dependency treegraph
Reported: 2008-05-07 03:00 PDT by moz_bug_r_a4
Modified: 2008-07-02 02:00 PDT (History)
9 users (show)
mtschrep: blocking1.9+
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

like so? (1.11 KB, patch)
2008-05-07 12:10 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
Fix (same as for bug 428672). (845 bytes, patch)
2008-05-07 12:29 PDT, Johnny Stenback (:jst,
jonas: review+
jonas: superreview+
dsicore: approval1.9+
Details | Diff | Splinter Review
less mole-whacking (723 bytes, patch)
2008-05-07 12:32 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
1.8 branch version (tested, works). (1.81 KB, patch)
2008-05-12 16:14 PDT, Johnny Stenback (:jst,
dveditz: review+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2008-05-07 03:00:48 PDT
Instead of <body> element, a XUL element can be used to attach an event handler
to an outer window.
Comment 3 Brian Crowder 2008-05-07 12:10:00 PDT
Created attachment 319837 [details] [diff] [review]
like so?
Comment 4 Blake Kaplan (:mrbkap) 2008-05-07 12:18:50 PDT
This seems like a route to whack-mole. Can we fix nsDocument::GetInnerWindow to return null instead of an outer window? I started looking through callers (of which there are a lot fewer than nsDocment::GetScriptGlobalObject) and it seems like all of them would be happy with such a change.
Comment 5 Johnny Stenback (:jst, 2008-05-07 12:29:35 PDT
Created attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).
Comment 6 Brian Crowder 2008-05-07 12:32:14 PDT
Created attachment 319842 [details] [diff] [review]
less mole-whacking

This does fix the bug...  we can fix null-derefs whack-a-mole style (and easily), and at least it's not an XSS vulnerability.
Comment 7 Johnny Stenback (:jst, 2008-05-07 14:02:10 PDT
Comment on attachment 319842 [details] [diff] [review]
less mole-whacking

Sorry, didn't see your original patch before I attached mine :(

We do want to do this, but not now IMO. The regression risk from this is unfortunately pretty high (as we've witnessed before), so we should do this as part of 431767 early in a release instead of now.
Comment 8 Damon Sicore (:damons) 2008-05-07 15:30:20 PDT
Comment on attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).

Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2008-05-07 18:18:42 PDT
jst, so I understand, you're advocating for your patch now, and crowder's patch as a 1.9.0.x thing? If so, can we get a new bug filed on that with crowder's patch attached so we can continue to track that? thx.
Comment 10 Johnny Stenback (:jst, 2008-05-07 21:58:01 PDT
Beltzner, yes, bug 431767 already on file.

Fix checked in.
Comment 11 Daniel Veditz [:dveditz] 2008-05-09 11:37:21 PDT
Do we need a different patch for 1.8?
Comment 12 Johnny Stenback (:jst, 2008-05-12 16:14:36 PDT
Created attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

This is the same patch for 1.8 (for this bug and for bug 428672 as well), though it's untested (I'm unable to build 1.8 due to cairo version dependency issues it seems). If someone is able to test this I'd appreciate it.
Comment 13 Samuel Sidler (old account; do not CC) 2008-06-02 21:34:23 PDT
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

Flagging dveditz for review of this so he can test. bsterne might also be able to.
Comment 14 Johnny Stenback (:jst, 2008-06-03 15:31:45 PDT
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

I just tested this fix, and it fixes the problem reported in this bug.
Comment 15 Daniel Veditz [:dveditz] 2008-06-05 02:23:36 PDT
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).


Approved for, a=dveditz for release-drivers

That's two pretty similar bugs -- do we need to audit all the calls to GetScriptGlobalObject() and GetInnerWindow() and make sure they don't have this problem?
Comment 16 Johnny Stenback (:jst, 2008-06-05 11:11:49 PDT
Fix checked in. Dan, yes, we're spot fixing here. There's a bug on file already to make GetInnerWindow() (on trunk) always really only return an inner window, but that's not something I'd be willing to land on the branch until it's gotten a fair bit of trunk testing.
Comment 17 Al Billings [:abillings] 2008-06-10 17:24:11 PDT
Verified for branch release with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008061005 BonEcho/ for testcase 1.

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