Closed Bug 428672 Opened 17 years ago Closed 17 years ago

XSS using an event handler attached to the outer window

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Details

(Keywords: verified1.8.1.15, Whiteboard: [sg:high xss] fixed on branch by 432591)

Attachments

(2 files, 5 obsolete files)

stack
(deleted), text/plain
Details
More targeted fix
(deleted), patch
jst
: review+
jst
: superreview+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
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+
Attached patch Proposed fix v1 (obsolete) (deleted) — — Splinter Review
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+
Attached patch Updated to comments (obsolete) (deleted) — — Splinter Review
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?
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+
Keywords: checkin-needed
mozilla/content/base/src/nsDocument.cpp 3.825
Status: ASSIGNED → RESOLVED
Closed: 17 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 → ---
Attached file stack (deleted) —
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
Attached patch With 100% less crashing (obsolete) (deleted) — — Splinter Review
Attachment #317626 - Attachment is obsolete: true
Attachment #318219 - Flags: superreview?(jst)
Attachment #318219 - Flags: review?(jst)
Attachment #318219 - Flags: superreview?(jst)
Attachment #318219 - Flags: superreview+
Attachment #318219 - Flags: review?(jst)
Attachment #318219 - Flags: review+
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?
(I ran the latest patch through mochitests successfully)
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?]
Assignee: mrbkap → jst
Status: REOPENED → NEW
Attached patch Now with 100% more bugs fixed (obsolete) (deleted) — — Splinter Review
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?
Attachment #318447 - Flags: superreview?(jst)
Attachment #318447 - Flags: superreview-
Attachment #318447 - Flags: review?(jst)
Attachment #318447 - Flags: review-
The caller is nsGenericHTMLElement::GetEventListenerManagerForAttr. I can try pushing the check there.
Attached patch More targeted fix (deleted) — — Splinter Review
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)
Attachment #318906 - Flags: superreview?(jst)
Attachment #318906 - Flags: superreview+
Attachment #318906 - Flags: review?(jst)
Attachment #318906 - Flags: review+
Whiteboard: [sg:high xss][needs new patch] → [sg:high xss]
Attachment #318906 - Flags: approval1.9?
Attachment #318906 - Flags: approval1.9? → approval1.9+
Whiteboard: [sg:high xss] → [has patch][has review][has approval][sg:high xss]
nsGenericHTMLElement.cpp: 1.766
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 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
Attached patch Wrong diff. (obsolete) (deleted) — — Splinter Review
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
Attachment #315281 - Attachment is private: true
Attachment #318347 - Attachment is private: true
Group: security
Flags: wanted1.9.0.x?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: