The default bug view has changed. See this FAQ.

Fix for bug 428672 can be circumvented by using XUL element

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: jst)

Tracking

({verified1.8.1.15})

unspecified
verified1.8.1.15
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.15 +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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]

Comment 3

9 years ago
Created attachment 319837 [details] [diff] [review]
like so?
Assignee: nobody → crowder
Status: NEW → ASSIGNED

Updated

9 years ago
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)

Comment 5

9 years ago
Created attachment 319840 [details] [diff] [review]
Fix (same as for bug 428672).
Assignee: crowder → jst
Attachment #319840 - Flags: superreview?(jonas)
Attachment #319840 - Flags: review?(jonas)

Comment 6

9 years ago
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.
Attachment #319837 - Attachment is obsolete: true
Attachment #319837 - Flags: review?(jst)

Updated

9 years ago
Attachment #319842 - Flags: review?(jst)
Attachment #319840 - Flags: superreview?(jonas)
Attachment #319840 - Flags: superreview+
Attachment #319840 - Flags: review?(jonas)
Attachment #319840 - Flags: review+
Attachment #319840 - Flags: approval1.9?
(Assignee)

Comment 7

9 years ago
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+
(Assignee)

Updated

9 years ago
Depends on: 431767

Updated

9 years ago
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]
(Assignee)

Comment 10

9 years ago
Beltzner, yes, bug 431767 already on file.

Fix checked in.
Flags: wanted1.9.0.x?
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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+
(Assignee)

Comment 12

9 years ago
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.
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)
(Assignee)

Comment 14

9 years ago
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+
(Assignee)

Comment 16

9 years ago
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.
Keywords: fixed1.8.1.15 → verified1.8.1.15
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.