Closed Bug 433328 Opened 13 years ago Closed 13 years ago

XSS using <script> element in unloaded document

Categories

(Core :: Security, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Details

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

Attachments

(2 files)

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
Attached patch Proposed fixSplinter Review
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+
I just tested that scripts do still work after going back to a document.
Flags: in-testsuite?
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
Closed: 13 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?
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.
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
(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.
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?
(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.
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
Depends on: CVE-2008-5015
No longer depends on: CVE-2008-5015
You need to log in before you can comment on or make changes to this bug.