It's unsafe to use mozIJSSubScriptLoader.loadSubScript() with non-chrome urls or chrome urls whose scheme/host part contain uppercase characters. Scripts that are loaded in this way do not use implicit XPCNativeWrappers when accessing content. loader.loadSubScript("chrome://test/..."); // safe loader.loadSubScript("chrome://TEST/..."); // unsafe loader.loadSubScript("file://..."); // unsafe Google Toolbar uses mozIJSSubScriptLoader.loadSubScript() with file: url, and allows an attacker to run arbitrary code with chrome privileges.

This testcase does not work on "https:"//bugzilla.mozilla.org/. Please set up this testcase on other server with "http:". This works on fx2.0.0.x with Google Toolbar. http://toolbar.google.com/

Steps to reproduce: 1. Install "test extension". 2. Click "test" button on the top of the browser's toolbox. Actual Results: chrome://lss-test/content/sub.js -> [object XPCNativeWrapper [object HTMLDocument]] / #document chrome://LSS-TEST/content/sub.js -> [object HTMLDocument] / xxx CHROME://lss-test/content/sub.js -> [object HTMLDocument] / xxx file://... -> [object HTMLDocument] / xxx data:text/plain,... -> [object HTMLDocument] / xxx

Hmm. The subscript loader always evaluates the script with the system principal? Note that we had a similar issue in the component loader; see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp&rev=1.157#1236 We could probably just copy this code, right? I'm not sure what the deal with case is; that's likely to be an issue for chrome in general, not just for the subscript loader. Benjamin might know what's going on there.

(In reply to comment #4) > Hmm. The subscript loader always evaluates the script with the system > principal? > Yes. I'll attach an updated version of test extension, which also shows whether the subscript is executed with the system principal. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/loader/mozJSSubScriptLoader.cpp&rev=1.25&mark=272,282-283#272

sub.js: function x(w) { var s; try { s = Components.stack; } catch (e) { s = e; } var d = w.content.document; return d.toString() + " / " + d.nodeName + "\n " + s; } Actual Results: chrome://lss-test/content/sub.js -> [object XPCNativeWrapper [object HTMLDocument]] / #document JS frame :: chrome://lss-test/content/sub.js :: x :: line 4 chrome://LSS-TEST/content/sub.js -> [object HTMLDocument] / xxx JS frame :: chrome://LSS-TEST/content/sub.js :: x :: line 4 CHROME://lss-test/content/sub.js -> [object HTMLDocument] / xxx JS frame :: CHROME://lss-test/content/sub.js :: x :: line 4 file://... -> [object HTMLDocument] / xxx JS frame :: file://.../sub.js :: x :: line 4 data:text/plain,.. -> [object HTMLDocument] / xxx JS frame :: data:text/plain,... :: x :: line 4

Not going to block for now as this isn't a regression, but that will change if we decide to block 1.8.0.13 for this.

Re-requesting trunk blocking. It may not be that common use in add-ons, but with Google Toolbar being vulnerable it's common enough. Blocking 1.8.1.13 is probably not realistic given the time remaining, but this is a branch blocker as soon as we can get a fix.

I don't really see a good way to fix this. The one way we *could* fix this is to make it so that subscripts loaded this way gets implicit wrapper automation if the caller of loadSubScript() is flagged to get implicit wrapper automation. The only sane way I see us being able to do that w/o some really major surgery to the code behind this is to lie and tell the JS engine that the filename of the script we're executing is the caller scripts filename, or at least a name that's prefixed with the callers script filename. I hacked that up, and it works, I made the script filename be "caller-script-filename -> loaded-subscript-uri". Too hacky? Other thoughts?

We could also always wrapper-automate things loaded via the subscript loader... Just like we do JS components.

I don't know that that's safe, that could leak XPCNativeWrappers out to untrusted code, which is bad. IOW, if an extension loads "http://evil.com/foo.js" (or file://, or data:...), and a webpage loads the same JS file, the script when loaded from the webpage will get implicit XPCNativeWrappers, and could then mutate them and fool chrome...

Oh, ugh. How about prohibiting the subscript loader from loading non-chrome scripts, then? That would make a lot more sense to me anyway; running anything like http://whatever as system script is beyond scary.

> How about prohibiting the subscript loader from loading non-chrome scripts, > then? Yes please, very much.

I used to use scope.eval to compile scripts into a specific scope, then fell back to using the subscript loader when eval was restricted. Now it appears I won't be able to use the subscript loader. What other methods are available for loading script source and compiling it into a specific scope?

Load it into an iframe and it gets a new scope. Or you can use the subscript loader as long as your script comes from chrome.

Just restricting loads to chrome:// here won't solve all the problems in this bug, we still need a way to deal with the package case mismatch issue as well, not sure what's the best way to deal with that yet. If we can assume package names are always lowercased, we could lowercase it here, but I don't know if that's a safe assumption.

Fix both issues. Using the canonicalized spec from the URI object means that whatever lowercasing the chrome registry does we pick up. I tested (using testcase 2) that this fixed the two uppercase tests. The check for a scheme of "chrome" fixes the other issue. Once I added that, the testcase just throws, of course. The rest of the changes are just making the error reporting work with the new setup and such. If desired, we could go back to using a naked char* for the buffer; I changed it because I was going to do other early returns in an earlier patch iteration, but I don't think that change is really needed anymore. We should certainly do the URI thing, no matter what we decide to do about non-chrome loads here.

Comment on attachment 310145 [details] [diff] [review] Fix for both issues Awesome. Bob, do you have alternatives that will work for you? Do we need to add a pref or something for testing harnesses or what not?

I'd also appreciate help with getting tests for this into a form that can be checked in.... And waiting for bc to respond before checking this in.

I think that I can work around it by inserting a script tag into the document and then appending the script text. Go for it. I'll deal.

Checked in. Would still love some help with the tests.

So who's going to tell the Google Toolbar people that we took away the feature they're using?

We should document these restrictions on MDC

This could be a painful bug to take on branch. It will break extensions and we have no Components.utils.import to offer extdevs. Currently, bz's patch only allows chrome://, but what about resource://? Components.utils.import accepts resource:// Branch has nsIResProtocolHandler, which could be used to create a bandaid for Fx2 extensions that are busted by the change.

Since this change appears to break extensions we may want to wait a bit for the branch release. Definitely want a trunk beta release under our belts to see how many we break and if the workarounds for those extensions are doable on the branch. I'd hate to have to kill all extensions by releasing a 2.0.1.x

If you take something that requires an active workaround on the part of extensions, you'll need to do a 2.0.1.x -- the set of broken extensions is simply not knowable (and can grow over time, if people are developing against 2.0.0.13 for a while and then ship their add-on with the reasonable 2.0.0.* maxVersion).

Yeah, I don't think we can, or should, take this on branch as written. Can we get some data on what extensions actually use this functionality with non-chrome URIs? Do these extensions actually depend on the resulting code being compiled with the system principal? Perhaps the right fix is to not give the system principal to code not loaded from chrome://? Frankly, using the channel principal instead of unconditionally using the system principal would make a heck of a lot more sense to me. If we did that, we could remove the "chrome:" check. That would break strictly less things, unless people are loading script from skin chrome packages. Thoughts?

I guess someone needs to file a bug to get an AMO scan done if we actually want that sort of data...

asking AMO to give us an idea on the impact by grepping addon source code (bug 423732)

mozIJSSubScriptLoader.idl comments need updated

So I was thinking about alternate ways to do this. Ideally, we would flag the JSScript itself instead of the filename, and then just always do wrapper automation here. Unfortunately, the JSScript* returned from JS_CompileScript is NOT the same JSScript* as the JSFunctions that got compiled point to. So flagging it is completely useless. If I could pass flags down into the JS_Compile* API, that would be nice, but we'd have to get them all the way down to js_EmitTree (which is where the "real" JSScript is created). I leave it to brendan and shaver to say what they think of that. The other bad news is that my impression from the people bitten by this so far who've gotten a hold of me (Operator and Calendar) is that they have the following constraints: 1) They're loading scripts from file:// 2) Their scripts really do need the system principal. So they're precisely in the "this code may be vulnerable" corner of things and really should get wrapper automation. Perhaps the right solution is that we should have a directory in which such scripts should be dumped and flag everything in that directory as system script? Or something along those lines? Alternately, we could just do what comment 9 suggests. It'll make the JS console a little more confusing for those scripts, and venkman/firebug will have to deal, but that's life. Reopening this and marking P1 for the time being; I don't think we can/should usefully ship the beta with the chrome check as it is. :( If nothing else, back out that one check before ship while we work on a better post-beta solution, I guess. But I'd love to hear brendan and shaver's thoughts here.

I suppose we could also require that anyone using this put the files in chrome://, but Operator at least uses this for files the user puts somewhere.... Calendar could probably change their stuff around, I would think. To be honest, we already do wrapper automation for some file:// URIs (component loader). Would doing that here increase risk? Note that we'd still want to do a protocol whitelist or something. No one should be loading http:// URIs through here, for sure.

This applies on top of the previous patch. The change is to ensure that the URI is either chrome or nsIFileURL (so chrome, file, resource) instead of just chrome, and to go ahead and flag it as a system filename. We could allow data: here if we wanted, but I don't see a reason to, to be honest. This reduces concerns in comment 11 to someone we don't trust loading file://, chrome://, or resource:// JS files, and given our same-directory security protections on file:// and resource://, I think that's not something we seriously need to worry about. But if you feel that it's a serious problem, then we should do your approach with prefixing the URI with the calling code's filename.

Comment on attachment 310413 [details] [diff] [review] I think this might be more viable Hmm, this does scare me. Doing this for files loaded through file:// URIs from a *known* location (i.e. the profile directory or what not) seems fine, but doing this for *any* file:// URI means that if an extension loads some random js file off disk through this API, then the user loads a HTML file that includes that same js file the HTML now has access to the implicit XPCNativeWrappers and could mutate them and make our chrome vulnerable in this case. Granted that's not an obvious nor trivial exploit to pull off, but unless we really need to, I'd rather not do this. I think I'd prefer a hack that simply fools the JS engine into flagging *this* particular load of this file as a system file, but not all loads of it. I could probably be convinced otherwise, but those are my thoughts at the moment. P.S. The comment in the patch above the call to FlagSystemFilenamePrefix() needs updated.

Calendar loads file:// URIs from the app install dir, I think. I guess they might be able to use resource:// for that if push came to shove. I think Operator loads from the profile. It wouldn't be that hard, I guess, to check that the file to be loaded is in a subdir of the profile or the install dir. That should cover all the cases we think are safe, right? For the "this particular load" thing we'd need to set the bits on the JSScript, which means flagging the prefix, compiling/executing, then unflagging the prefix. And having the js_NewScript code look up the bits and save them. In any case, it sounds like we should just do what you suggested in comment 9. I've more or less run out of time to work on this for the next few days. :(

(In reply to comment #33) > We could allow data: here if we wanted, but I don't see a reason to What about dynamically loading code snippets from prefs (or other configuration files)? At least that's what I've been doing, so far (and currently don't see an alternative to).

It's just too easy to shoot yourself in the foot with data:, in my opinion... What would really need to be vetted is the source of the data, and there's no way to do it from this code. If you trust the data enough, you can always write it to a file and execute that file.

I should also note that if you just have the JS string you can also evalInSandbox it, no?

(In reply to comment #38) > if you just have the JS string you can also evalInSandbox it, no? Right. Turns out that an evaluation context can even be passed to window.eval. Thanks for the hint!

So I'm not quite sure how to get the caller script here. jst, if you know, do you want to write it up? I'm not likely to be able to do the research needed to get it done before beta... On a separate note, bug 423732 did a scan of AMO for this, and there is only one use of data:, in what looks like a unit-testing extension.

This re-enables file:// URIs in the subscript loader, but it prepends the calling scripts filename followed by " -> " to the file:// URI so that whether the subscript gets XPCNativeWrappers or not will depend on the caller, not the file:// URI.

Comment on attachment 310893 [details] [diff] [review] Permit file:// URIs I presume this is going to require some fancy footwork in debuggers? I'm gonna want to see some tests with that, kthx. :)

I think you want to use the QI to nsIFileURL that I used in my patch. Otherwise you're not allowing through resource:// URIs, which I think we do want to allow. It should be possible to use most of my patch, minus the flagging, plus the script filename stuff.

This deals with file:// URIs as well as resource:// URIs, and for those prepends the caller script filename. As we don't have any kind of system in place to write tests for this, I won't be able to do that in time for beta 5, the tests would need to place a js file in the resource directory and then make sure it's loadable, and it would need to be able to load it from both a location where implicit XPCNativeWrappers are enabled and also from where they're not enabled.

Comment on attachment 310920 [details] [diff] [review] Updated fix. Looks good. Thank you for picking this up!

Fix checked in.

(In reply to comment #44) > Created an attachment (id=310920) [details] > Updated fix. > > This deals with file:// URIs as well as resource:// URIs, and for those > prepends the caller script filename. > > As we don't have any kind of system in place to write tests for this, I won't > be able to do that in time for beta 5 I'm surprised you can't test subscript loader's resulting privs with xpcshell tests, at least partially? (Can you at least file a bug describing what sort of system you'd need to write tests for it, so it's not just forgotten after b5?) > the tests would need to place a js file > in the resource directory and then make sure it's loadable, and it would need > to be able to load it from both a location where implicit XPCNativeWrappers are > enabled and also from where they're not enabled. I don't quite understand this part -- won't those tests pass on a build without this patch as well? Doesn't seem like they test that this bug is (still) fixed.

We should really test the following things for this bug: 1) resource:// URIs are loadable 2) file:// URIs are loadable 3) chrome:// URIs are loadable 4) Other URIs are not loadable. Especially, http:// URIs are not loadable. 5) Loading a loadable chrome URI does XPCNativeWrapper automation based on that chrome URI. 6) Loading a file:// or resource:// URI does XPCNativeWrapper automation based on the URI of the caller of loadSubscript. 7) Make sure to test various cases for chrome:// URIs. 8) Make sure that in all cases the loaded script is executed with the system principal. We should be able to test (4) with a mochitest. To test the rest, we need to place files that could be loadable at chrome://, file:// and resource:// URIs. file:// is doable with a mochitest: just create a file in /tmp as part of the test, write data to it, and try to loadSubscript() it, then remove it when done. It _might_ be possible to do the same with resource:// by using the app dir instead of /tmp? I'm not sure how to do chrome:// offhand. Ideally, we would have a basic system for placing things at such URIs, either via makefile (likely needed for chrome) or via some SimpleTest methods (just pass the string you want in the file, and it'll create the file for you, etc). So I think we can test 1, 2, 4, 8 (except the chrome:// case for 8) as things stand. To test 3, 5, 6, we need to figure out a way to run a test as part of a chrome package where we control wrapper automation and need a way to be loading a chrome:// file.

Filed bug 424483 and bug 424484 for some of the infrastructure improvements that could be useful here.

I spun off bug 424485 on writing the tests for this.

This broke at least the Google Notebook extension when it went in for Firefox 3, see bug 427902. An updated version of the Google Notebook extension exists now, but I don't know if that one works with Firefox 2, or what other extensions would break if we took this on the branch. If someone could test at least the Google Notebook extension, and that still works, I could port this to the branch. Shouldn't be too much work...

Al, can you confirm that the latest Google Notebook extension works on branch and isn't only trunk-compatible?

It installs and worked in the latest branch nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.15pre) Gecko/2008060303 BonEcho/2.0.0.15pre).

This is the same as the trunk version except for the change to NS_OpenURI() in nsNetUtils.h which was needed by this fix. (attaching to the right bug this time)

Comment on attachment 323747 [details] [diff] [review] 1.8 branch version. Looks good.

Comment on attachment 323747 [details] [diff] [review] 1.8 branch version. Approved for 1.8.1.15, a=dveditz for release-drivers

Fixed on the branch. QA: It is essential for this to get extra extension compatibility testing before 1.8.1.15 goes out the door!

Especially the JavaScript Debugger

Do you guys have a list so QA isn't simply guessing? The Google Notebook extension is obvious. Michael, which JavaScript Debugger? Perhaps it is obvious to others but not to me.

(In reply to comment #59) > Do you guys have a list so QA isn't simply guessing? Al, see http://starkravingfinkle.org/blog/2008/03/extension-developers-breaking-news-part-2/ comments for a list of extensions that needed to be fixed in FF3. I assume these will be affected in FF2 as well.

(In reply to comment #59) > Do you guys have a list so QA isn't simply guessing? > > The Google Notebook extension is obvious. > > Michael, which JavaScript Debugger? Perhaps it is obvious to others but not to > me. > Venkman as well as Firebug, I'd presume, though I would guess that mkaply was referring to the former.

Testcase 1 doesn't seem to do anything in either 2.0.0.14 or 2.0.0.15 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre). Testcase 2 is verified to no longer work in 2.0.0.15. The Google Notebook extension works without any issues. I verified Firebug and Venkman as well. In nightly 1.8 Thunderbird (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061003 Lightning/0.8 Thunderbird/2.0.0.15pre), I installed Lightning .8, since there were concerns about this completely breaking lightning as prevoiously written. Lightning ran without any noticeable errors (I imported an .ics calendar file, got alerts, looked at events). I also installed Chatzilla and verified that plugins for it are working (this was a problem previously reported). I asked Ctalbert, who has been working with MozRepl, to check to see if that still works. I asked StephenD, who has been working with Selenium, to check those as well.

I recorded and ran a Selenium IDE script against http://addons.mozilla.org using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre just fine.

Ran a simple automation test using MozRepl (http://hyperstruct.net/projects/mozlab) and it worked great on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.15pre) Gecko/2008061104 BonEcho/2.0.0.15pre

I'm marking this as verified for 1.8.1.15 then.

I am having (relatively new) issues with venkman loading scripts outside the components/ directory in lightning; some are hosted in js/. The scripts are properly loaded and listed (lightning works etc), but clicking them in venkman raises errors like: Error loading URL <file:/Users/dbo/profiles/thunderbird/mst/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calItemModule.js -> file:///Users/dbo/profiles/thunderbird/mst/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calAttendee.js>: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIChannel.asyncOpen]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://venkman/content/venkman-url-loader.js :: loadURLAsync :: line 79" data: no]. I am not sure whether this relates to this bug/change (and my updated thunderbird 2), though, but I can reproduce it with lightning 0.8, thus can exclude a recent change in lightning has caused it.

I'm pretty sure it is caused by this bug's change, as Venkman doesn't know what to make of the "URI1 -> URI2" syntax (file a bug if there isn't one, please, so Venkman can be fixed to load the correct URI). In the mean time, you should be able to use Pretty Print to debug these scripts.

Comment on attachment 323747 [details] [diff] [review] 1.8 branch version. a=asac for 1.8.0

Documentation updated.