The default bug view has changed. See this FAQ.

XSS using an event handler attached to the outer window

RESOLVED FIXED in mozilla1.9

Status

()

Core
Security
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.8.1.15})

unspecified
mozilla1.9
x86
Windows XP
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 xss] fixed on branch by 432591)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Flags: wanted1.8.1.x?
Flags: blocking1.9?
Whiteboard: [sg:high xss]
Dan: should we be fixing this right away? We'll definitely take it if you're taking it for the next branch release ...
Flags: wanted1.9.0.x?
Flags: blocking1.9?
Flags: blocking1.9-
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 1.9.0.1
Assignee: dveditz → jst
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15? → blocking1.8.1.15+
(Assignee)

Comment 4

9 years ago
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.
Assignee: jst → mrbkap
Status: NEW → ASSIGNED
Attachment #317603 - Flags: superreview?(jst)
Attachment #317603 - Flags: review?(jst)
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.
Attachment #317603 - Flags: superreview?(jst)
Attachment #317603 - Flags: superreview+
Attachment #317603 - Flags: review?(jst)
Attachment #317603 - Flags: review+
(Assignee)

Comment 6

9 years ago
Created attachment 317626 [details] [diff] [review]
Updated to comments

This fixes a security bug.
Attachment #317603 - Attachment is obsolete: true
Attachment #317626 - Flags: superreview+
Attachment #317626 - Flags: review+
Attachment #317626 - Flags: approval1.9?
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...
Attachment #317603 - Attachment is obsolete: false
Attachment #317603 - Flags: approval1.9?
(Assignee)

Updated

9 years ago
Attachment #317603 - Attachment is obsolete: true
Attachment #317603 - Flags: approval1.9?
Flags: blocking1.9- → blocking1.9+
Comment on attachment 317626 [details] [diff] [review]
Updated to comments

a1.9=beltzner
Attachment #317626 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
mozilla/content/base/src/nsDocument.cpp 	3.825 
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
This caused crashes in mochitest and the browser chrome test, consistently on 3 platforms, so I had to back it out.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 318125 [details]
stack

I reproduced this crash locally, here's the stack.
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
(Assignee)

Comment 13

9 years ago
Created attachment 318219 [details] [diff] [review]
With 100% less crashing
Attachment #317626 - Attachment is obsolete: true
Attachment #318219 - Flags: superreview?(jst)
Attachment #318219 - Flags: review?(jst)

Updated

9 years ago
Attachment #318219 - Flags: superreview?(jst)
Attachment #318219 - Flags: superreview+
Attachment #318219 - Flags: review?(jst)
Attachment #318219 - Flags: review+
(Assignee)

Comment 14

9 years ago
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!
Attachment #318219 - Flags: approval1.9?
(Assignee)

Comment 15

9 years ago
(I ran the latest patch through mochitests successfully)
(Reporter)

Comment 16

9 years ago
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 on attachment 318219 [details] [diff] [review]
With 100% less crashing

a=beltzner, can we make that testcase a test and include that as well?
Attachment #318219 - Flags: approval1.9? → approval1.9+
Whiteboard: [sg:high xss] → [sg:high xss][needs new patch][needs to block?]

Updated

9 years ago
Assignee: mrbkap → jst
Status: REOPENED → NEW
(Assignee)

Comment 19

9 years ago
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?
Assignee: jst → mrbkap
Attachment #318219 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #318447 - Flags: superreview?(jst)
Attachment #318447 - Flags: review?(jst)
Whiteboard: [sg:high xss][needs new patch][needs to block?] → [sg:high xss][needs new patch]
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?

Updated

9 years ago
Attachment #318447 - Flags: superreview?(jst)
Attachment #318447 - Flags: superreview-
Attachment #318447 - Flags: review?(jst)
Attachment #318447 - Flags: review-
(Assignee)

Comment 21

9 years ago
The caller is nsGenericHTMLElement::GetEventListenerManagerForAttr. I can try pushing the check there.
(Assignee)

Comment 22

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

Updated

9 years ago
Attachment #318906 - Flags: superreview?(jst)
Attachment #318906 - Flags: superreview+
Attachment #318906 - Flags: review?(jst)
Attachment #318906 - Flags: review+

Updated

9 years ago
Whiteboard: [sg:high xss][needs new patch] → [sg:high xss]

Updated

9 years ago
Attachment #318906 - Flags: approval1.9?

Updated

9 years ago
Attachment #318906 - Flags: approval1.9? → approval1.9+
Whiteboard: [sg:high xss] → [has patch][has review][has approval][sg:high xss]

Comment 23

9 years ago
nsGenericHTMLElement.cpp: 1.766
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Whiteboard: [has patch][has review][has approval][sg:high xss] → [sg:high xss][needs branch patch]
This problem is fixed on the 1.8 branch by the fix for bug 432591.
Whiteboard: [sg:high xss][needs branch patch] → [sg:high xss] fixed on branch by 432591
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.
Attachment #323646 - Flags: superreview?(bzbarsky)
Attachment #323646 - Flags: review?(bzbarsky)
jst, that doesn't look like a patch for this bug (but for the subscript loader one).
Comment on attachment 323646 [details] [diff] [review]
Wrong diff.

Yeah, duh.
Attachment #323646 - Attachment description: 1.8 branch version. → Wrong diff.
Attachment #323646 - Attachment is obsolete: true
Attachment #323646 - Flags: superreview?(bzbarsky)
Attachment #323646 - Flags: review?(bzbarsky)
Fixed by the checkin for bug 432591.
Keywords: fixed1.8.1.15
Verified testcase 1 (the only one that works in branch) 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 2.0.0.15 release.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Group: security
Flags: wanted1.9.0.x?
You need to log in before you can comment on or make changes to this bug.