Last Comment Bug 433328 - XSS using <script> element in unloaded document
: XSS using <script> element in unloaded document
[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)
: David Keeler [:keeler] (use needinfo?)
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image Blake Kaplan (:mrbkap) 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 User image Johnny Stenback (:jst, 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 User image Blake Kaplan (:mrbkap) 2008-05-19 13:37:24 PDT
I just tested that scripts do still work after going back to a document.
Comment 9 User image Blake Kaplan (:mrbkap) 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 User image 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 User image 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 User image 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 User image :Gavin Sharp [email:] 2008-05-28 12:04:55 PDT
mozilla/content/base/src/nsScriptLoader.cpp 	1.121 
Comment 14 User image 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 User image Johnny Stenback (:jst, 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 User image Johnny Stenback (:jst, 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 User image Daniel Veditz [:dveditz] 2008-06-05 01:59:15 PDT
Comment on attachment 322975 [details] [diff] [review]
1.8.1 branch fix.


Approved for, a=dveditz for release-drivers
Comment 18 User image Johnny Stenback (:jst, 2008-06-05 16:50:57 PDT
Fixed on the branch.
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2008-06-05 16:57:14 PDT
Does the XUL script code need a similar change?
Comment 20 User image 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: Gecko/2008061005 BonEcho/ This looks good.
Comment 21 User image 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 User image Blake Kaplan (:mrbkap) 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 User image Boris Zbarsky [:bz] (still a bit busy) 2008-06-24 20:49:17 PDT
No, it in fact does not.
Comment 24 User image Blake Kaplan (:mrbkap) 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image Blake Kaplan (:mrbkap) 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 User image 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.