433328 - XSS using <script> element in unloaded document

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

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.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This seems to whack the mole. I need to ensure that scripts still work in documents that have been navigated back to.

Comment 7

[Johnny Stenback (: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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I just tested that scripts do still work after going back to a document.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 10

[Daniel Veditz [:dveditz]]

Looks like this got lost, may be too late for FF3 and have to wait for 3.0.1

Comment 11

[Mike Shaver (:shaver emeritus)]

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_.

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/content/base/src/nsScriptLoader.cpp 	1.121 

Comment 14

[Samuel Sidler (old account; do not CC)]

Johnny, are you able to back-port this patch to the 1.8 branch since Blake is out?

Comment 15

[Johnny Stenback (:jst)]

Attachment: 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...

Comment 16

[Johnny Stenback (:jst)]

I'm now able to build firefox 2, so I've tested that the above patch does indeed fix this bug.

Comment 17

[Daniel Veditz [: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

Comment 18

[Johnny Stenback (:jst)]

Fixed on the branch.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Does the XUL script code need a similar change?

Comment 20

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

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

Er, does it not just go through the script loader?

Comment 23

[Boris Zbarsky [:bzbarsky]]

No, it in fact does not.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 25

[Boris Zbarsky [:bzbarsky]]

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?

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 27

[moz_bug_r_a4]

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?