Last Comment Bug 180748 - There is no security check on XUL script src, are there others?
: There is no security check on XUL script src, are there others?
Status: RESOLVED FIXED
: fixed1.4
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows 2000
-- normal (vote)
: ---
Assigned To: Mitchell Stoltz (not reading bugmail)
:
: Neil Deakin
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-11-18 10:51 PST by bsharma
Modified: 2008-07-31 03:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch - add CheckLoadURI call to XUL script tag parsing (1.57 KB, patch)
2003-05-28 15:53 PDT, Mitchell Stoltz (not reading bugmail)
hjtoi-bugzilla: review+
brendan: superreview+
blizzard: approval1.4+
Details | Diff | Splinter Review

Description User image bsharma 2002-11-18 10:51:36 PST
This bug is reported as the issue in the module review and jrgm asked me to make
a bug out of it.
Comment 1 User image Mitchell Stoltz (not reading bugmail) 2003-02-27 14:10:48 PST
I'm taking this one; part of the CheckLoadURI overhaul.
Comment 2 User image Mitchell Stoltz (not reading bugmail) 2003-05-15 17:05:31 PDT
I want to add a call to CheckLoadURI for <script src="..."/> tags in XUL, to
prevent XUL pages loaded from http:// from using JS files from file:// (they
will still be able to use script files from chrome). I'm not sure where to put
the call - my guess is nsXULContentSink, but I'm not sure where the best spot
is; I don't want to mess up the Fastload stuff. Can you offer any advice?
Comment 3 User image Heikki Toivonen (remove -bugzilla when emailing directly) 2003-05-19 12:20:39 PDT
I don't think the sink is enough, since you can create a script element
dynamically. It should go into the script element or the script loader, I think.
Comment 4 User image Mitchell Stoltz (not reading bugmail) 2003-05-27 16:17:37 PDT
from Brendan:

There's nothing to worry about with FastLoad.  Just check in
nsXULContentSink::OpenScript, near the NS_NewURI call.  That call's failure case
is handled by a delete script; return rv block. 
Comment 5 User image Mitchell Stoltz (not reading bugmail) 2003-05-28 15:53:23 PDT
Created attachment 124395 [details] [diff] [review]
Patch - add CheckLoadURI call to XUL script tag parsing
Comment 6 User image Mitchell Stoltz (not reading bugmail) 2003-05-28 15:54:09 PDT
Comment on attachment 124395 [details] [diff] [review]
Patch - add CheckLoadURI call to XUL script tag parsing

Seeking reviews.
Comment 7 User image Heikki Toivonen (remove -bugzilla when emailing directly) 2003-05-28 16:32:04 PDT
Comment on attachment 124395 [details] [diff] [review]
Patch - add CheckLoadURI call to XUL script tag parsing

r=heikki if you add a comment saying that if/when XUL scripts can be
dynamically created and they can load external source, then the security check
should move there (currently our understanding is that they cannot be created
dynamically or at least they won't work if they are created)
Comment 8 User image Brendan Eich [:brendan] 2003-05-28 16:47:22 PDT
Comment on attachment 124395 [details] [diff] [review]
Patch - add CheckLoadURI call to XUL script tag parsing

Looks ok to me -- where is the code that prevents a script from making a script
node and setting its src attribute?

/be
Comment 9 User image Heikki Toivonen (remove -bugzilla when emailing directly) 2003-05-28 17:20:52 PDT
I don't think there is any code to trigger loading the src when the script
element is created dynamically. I tried some tests to create the script
dynamically and set the src attribute, but the external script was never executed.

Also, in nsXULElement.cpp line 5216 there seems to be code that prevents cached
XUL (fastload?) from loading non-chrome JS.

Mitch, could you also check out nsXULDocument::LoadScript() and make sure that
it cannot veto over the sink? Just in case...

Comment 10 User image Brendan Eich [:brendan] 2003-05-28 19:48:24 PDT
> Also, in nsXULElement.cpp line 5216 there seems to be code that prevents cached
> XUL (fastload?) from loading non-chrome JS.

No, that check is deciding whether to cache the compiled script in memory.  It
has nothing to do with FastLoad per se, although related XUL content code
requires a chrome script to be cached after being compiled, so that it will
never be reloaded from disk (and therefore mux'd to the FastLoad file twice).

/be
Comment 11 User image Mitchell Stoltz (not reading bugmail) 2003-05-30 12:40:34 PDT
I can't find any code path that bypasses the sink, so I think we're OK. Iv'e
checked this in on the trunk, waiting for 1.4 branch approval.
Comment 12 User image Christopher Blizzard (:blizzard) 2003-05-30 14:35:05 PDT
Comment on attachment 124395 [details] [diff] [review]
Patch - add CheckLoadURI call to XUL script tag parsing

a=blizzard for 1.4
Comment 13 User image Samir Gehani 2003-05-30 15:06:08 PDT
a=adt to landon the 1.4 branch.
Comment 14 User image Mitchell Stoltz (not reading bugmail) 2003-06-02 19:38:52 PDT
Checked in on trunk.
Comment 15 User image Mitchell Stoltz (not reading bugmail) 2003-06-02 19:39:28 PDT
And on the branch.
Comment 16 User image Paul Wyskoczka 2003-06-03 07:51:33 PDT
Adding keyword fixed1.4
Comment 17 User image Asa Dotzler [:asa] 2003-07-27 19:12:11 PDT
mozilla1.4 shipped. unsetting blocking1.4 request.
Comment 18 User image Daniel Veditz [:dveditz] 2004-07-20 04:45:20 PDT
Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.

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