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
Status: RESOLVED FIXED
[sg:high]
: verified1.8.1.15
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on: 431767
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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, jst@mozilla.com)
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, jst@mozilla.com)
dveditz: review+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description User image 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 User image Brian Crowder 2008-05-07 12:10:00 PDT
Created attachment 319837 [details] [diff] [review]
like so?
Comment 4 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-05-07 12:29:35 PDT
Created attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).
Comment 6 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 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 User image Damon Sicore (:damons) 2008-05-07 15:30:20 PDT
Comment on attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).

a1.9+=damons
Comment 9 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 2008-05-07 21:58:01 PDT
Beltzner, yes, bug 431767 already on file.

Fix checked in.
Comment 11 User image Daniel Veditz [:dveditz] 2008-05-09 11:37:21 PDT
Do we need a different patch for 1.8?
Comment 12 User image Johnny Stenback (:jst, jst@mozilla.com) 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 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 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 User image Daniel Veditz [:dveditz] 2008-06-05 02:23:36 PDT
Comment on attachment 320617 [details] [diff] [review]
1.8 branch version (tested, works).

r=dveditz

Approved for 1.8.1.15, 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 User image Johnny Stenback (:jst, jst@mozilla.com) 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 User image Al Billings [:abillings] 2008-06-10 17:24:11 PDT
Verified for branch 2.0.0.15 release with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre for testcase 1.

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