The default bug view has changed. See this FAQ.

XSS using <script> element in unloaded document

VERIFIED FIXED in mozilla1.9

Status

()

Core
Security
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.8.1.15})

Trunk
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])

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
This is probably the same bug as bug 433003.  And, this seems to be a problem
with nsDocument::GetScriptGlobalObject like bug 428672.

A <script> element in an old document can be evaluated in a new document.  The
script itself is evaluated with the old document's principal, but, it's
possible to create a function that can run with the new document's principal.
Whiteboard: [sg:high xss]
Flags: wanted1.9.0.x?
Flags: blocking1.8.1.15?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15+
Version: unspecified → Trunk
(Assignee)

Comment 6

9 years ago
Created attachment 321642 [details] [diff] [review]
Proposed fix

This seems to whack the mole. I need to ensure that scripts still work in documents that have been navigated back to.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #321642 - Flags: superreview?(jst)
Attachment #321642 - Flags: review?(jst)
Comment on attachment 321642 [details] [diff] [review]
Proposed fix

Yeah, unfortunately this is probably the best we can do for now. r+sr=jst
Attachment #321642 - Flags: superreview?(jst)
Attachment #321642 - Flags: superreview+
Attachment #321642 - Flags: review?(jst)
Attachment #321642 - Flags: review+
(Assignee)

Comment 8

9 years ago
I just tested that scripts do still work after going back to a document.
Flags: in-testsuite?
(Assignee)

Comment 9

9 years ago
I'm not sure if this needs additional markings, but if someone can check this in before I get back a month from now, they should.
Keywords: checkin-needed
Looks like this got lost, may be too late for FF3 and have to wait for 3.0.1
Flags: blocking1.9?
Comment on attachment 321642 [details] [diff] [review]
Proposed fix

If we're asserting there, should we be defending in the code, too? Seems like we used to defend with an early return via NS_ENSURE_FALSE_HIDE_CONTROL_FLOW.

If that is not necessary, this patch has approval to land on CVS trunk _immediately_.
Attachment #321642 - Flags: approval1.9+
Instead of ensuring the result of the GetScriptGlobalObject() helper function  we're now doing it manually to make sure the window is an inner window. The window null check is equivalent to the previous check. From there if a non-null window isn't a nsIScriptGlobalObject we're very unhappy, an assertion is OK here.
Flags: blocking1.9? → blocking1.9+
mozilla/content/base/src/nsScriptLoader.cpp 	1.121 
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Johnny, are you able to back-port this patch to the 1.8 branch since Blake is out?
Created attachment 322975 [details] [diff] [review]
1.8.1 branch fix.

This compiles, but I don't have a way of testing this atm. Dan, or someone else, can you apply this and give it a spin? It's the same change, so I wouldn't expect any problems...
Attachment #322975 - Flags: review?(dveditz)
Flags: wanted1.9.0.x?
Whiteboard: [sg:high xss] → [sg:high xss] branch patch needs testing
I'm now able to build firefox 2, so I've tested that the above patch does indeed fix this bug.
Whiteboard: [sg:high xss] branch patch needs testing → [sg:high xss]
Whiteboard: [sg:high xss] → [sg:high xss][needs r dveditz]
Comment on attachment 322975 [details] [diff] [review]
1.8.1 branch fix.

r=dveditz

Approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #322975 - Flags: review?(dveditz) → review+
Attachment #322975 - Flags: approval1.8.1.15+
Fixed on the branch.
Keywords: fixed1.8.1.15
Whiteboard: [sg:high xss][needs r dveditz] → [sg:high xss]
Does the XUL script code need a similar change?
Verified the second testcase for 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. This looks good.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Verified for trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008061004 Firefox/3.0 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0 since I'm here anyway.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 22

9 years ago
(In reply to comment #19)
> Does the XUL script code need a similar change?

Er, does it not just go through the script loader?
No, it in fact does not.
(Assignee)

Comment 24

9 years ago
I looked through the XUL script code and it doesn't appear to support adding new XUL script elements (you can get the same effect by creating a new HTML script node, which goes through the script loader and should therefore be safe). Compilation of script nodes happens in the content sink (in OpenScript) and in the document (during ResumeWalk). Therefore, it seems to me that because XUL script nodes can only be compiled and run during parsing, we don't have to worry about this attack there. Boris, does that seem correct?
Hmm.  That does sound right, yes.  If the testcase depends on dynamically adding scripts, then we might be ok.

What if a new load is started partway through the parse?
(Assignee)

Comment 26

9 years ago
(In reply to comment #25)
> What if a new load is started partway through the parse?

That terminates the parser, which (as I understand things) terminates the current load.
(Reporter)

Comment 27

9 years ago
It seems to me that <xul:script> cannot be used for this attack.  This attack
depends on nsDocument::GetScriptGlobalObject().  But,
nsXULDocument::ExecuteScript() doesn't call nsDocument::GetScriptGlobalObject()
and instead accesses mScriptGlobalObject directly.  If
nsXULDocument::ExecuteScript() is called on an unloaded document somehow,
mScriptGlobalObject is null, and thus script is not executed.  Is this right?
Group: security

Updated

9 years ago
Depends on: 447579
(Assignee)

Updated

9 years ago
No longer depends on: 447579
You need to log in before you can comment on or make changes to this bug.