Closed Bug 180748 Opened 17 years ago Closed 17 years ago
There is no security check on XUL script src, are there others?
This bug is reported as the issue in the module review and jrgm asked me to make a bug out of it.
I'm taking this one; part of the CheckLoadURI overhaul.
Assignee: jaggernaut → mstoltz
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?
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.
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 on attachment 124395 [details] [diff] [review] Patch - add CheckLoadURI call to XUL script tag parsing Seeking reviews.
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)
Attachment #124395 - Flags: review?(heikki) → review+
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
Attachment #124395 - Flags: superreview?(brendan) → superreview+
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...
> 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
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.
Status: NEW → ASSIGNED
Comment on attachment 124395 [details] [diff] [review] Patch - add CheckLoadURI call to XUL script tag parsing a=blizzard for 1.4
Attachment #124395 - Flags: approval1.4? → approval1.4+
a=adt to landon the 1.4 branch.
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
And on the branch.
Adding keyword fixed1.4
mozilla1.4 shipped. unsetting blocking1.4 request.
Bugs published on the Known-vulnerabilities page long ago, removing confidential flag.
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.