Last Comment Bug 433328 - XSS using <script> element in unloaded document
: XSS using <script> element in unloaded document
Status: VERIFIED FIXED
[sg:high xss]
: verified1.8.1.15
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-12 07:31 PDT by moz_bug_r_a4
Modified: 2008-07-23 08:38 PDT (History)
8 users (show)
mbeltzner: blocking1.9+
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (858 bytes, patch)
2008-05-19 13:06 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
jst: superreview+
shaver: approval1.9+
Details | Diff | Review
1.8.1 branch fix. (970 bytes, patch)
2008-05-29 13:00 PDT, Johnny Stenback (:jst, jst@mozilla.com)
dveditz: review+
dveditz: approval1.8.1.15+
Details | Diff | Review

Description moz_bug_r_a4 2008-05-12 07:31:59 PDT
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) (please use needinfo!) 2008-05-19 13:06:34 PDT
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.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2008-05-19 13:16:38 PDT
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) (please use needinfo!) 2008-05-19 13:37:24 PDT
I just tested that scripts do still work after going back to a document.
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-05-20 22:17:14 PDT
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] 2008-05-28 11:00:24 PDT
Looks like this got lost, may be too late for FF3 and have to wait for 3.0.1
Comment 11 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-05-28 11:21:20 PDT
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] 2008-05-28 11:57:30 PDT
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: gavin@gavinsharp.com] 2008-05-28 12:04:55 PDT
mozilla/content/base/src/nsScriptLoader.cpp 	1.121 
Comment 14 Samuel Sidler (old account; do not CC) 2008-05-28 13:53:39 PDT
Johnny, are you able to back-port this patch to the 1.8 branch since Blake is out?
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2008-05-29 13:00:58 PDT
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...
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-03 15:44:13 PDT
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] 2008-06-05 01:59:15 PDT
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, jst@mozilla.com) 2008-06-05 16:50:57 PDT
Fixed on the branch.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-06-05 16:57:14 PDT
Does the XUL script code need a similar change?
Comment 20 Al Billings [:abillings] 2008-06-10 18:09:23 PDT
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] 2008-06-10 18:13:22 PDT
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) (please use needinfo!) 2008-06-24 07:43:12 PDT
(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 [:bz] (Out June 25-July 6) 2008-06-24 20:49:17 PDT
No, it in fact does not.
Comment 24 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-25 05:07:54 PDT
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 [:bz] (Out June 25-July 6) 2008-06-25 06:49:45 PDT
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) (please use needinfo!) 2008-06-25 14:04:14 PDT
(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 2008-06-26 01:40:45 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.