Closed Bug 432591 Opened 12 years ago Closed 12 years ago

Fix for bug 428672 can be circumvented by using XUL element

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: jst)

References

Details

(Keywords: verified1.8.1.15, Whiteboard: [sg:high])

Attachments

(3 files, 1 obsolete file)

Instead of <body> element, a XUL element can be used to attach an event handler
to an outer window.
Flags: wanted1.9.0.x?
Flags: wanted1.8.1.x?
Flags: blocking1.9?
Flags: blocking1.8.1.15?
Whiteboard: [sg:high]
Attached patch like so? (obsolete) — Splinter Review
Assignee: nobody → crowder
Status: NEW → ASSIGNED
Attachment #319837 - Flags: review?(jst)
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.
Assignee: crowder → jst
Attachment #319840 - Flags: superreview?(jonas)
Attachment #319840 - Flags: review?(jonas)
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.
Attachment #319837 - Attachment is obsolete: true
Attachment #319837 - Flags: review?(jst)
Attachment #319842 - Flags: review?(jst)
Attachment #319840 - Flags: superreview?(jonas)
Attachment #319840 - Flags: superreview+
Attachment #319840 - Flags: review?(jonas)
Attachment #319840 - Flags: review+
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.
Attachment #319842 - Flags: review?(jst)
Comment on attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).

a1.9+=damons
Attachment #319840 - Flags: approval1.9? → approval1.9+
Depends on: 431767
Flags: blocking1.9? → blocking1.9+
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.
Whiteboard: [sg:high] → [sg:high][has patch][has review][has approval]
Beltzner, yes, bug 431767 already on file.

Fix checked in.
Flags: wanted1.9.0.x?
OS: Windows XP → All
Hardware: PC → All
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Do we need a different patch for 1.8?
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15+
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.
Whiteboard: [sg:high][has patch][has review][has approval] → [sg:high][branch patch needs testing]
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.
Attachment #320617 - Flags: review?(dveditz)
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.
Attachment #320617 - Attachment description: 1.8 branch version (untested). → 1.8 branch version (tested, works).
Whiteboard: [sg:high][branch patch needs testing] → [sg:high]
Whiteboard: [sg:high] → [sg:high][needs r dveditz then approval]
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?
Attachment #320617 - Flags: review?(dveditz)
Attachment #320617 - Flags: review+
Attachment #320617 - Flags: approval1.8.1.15+
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.
Keywords: fixed1.8.1.15
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.
Whiteboard: [sg:high][needs r dveditz then approval] → [sg:high]
Group: security
You need to log in before you can comment on or make changes to this bug.