We should make javascript: URIs refuse point blank to do anything if they are running in a privileged context. Please who want to do something in a privileged context should use something other than URIs to do it.

Just a heads-up, this will remove all chrome permissions from the JS Console. Whether that is an ok side-effect or not is not my call, but I know a number of people will not like that change. :)

The JS Console doesn't _have_ to use javascript: URIs to do what it does.

See bug 293363 comment 10 and bug 293363 comment 11. /be

I'm not talking about adding more DISALLOW_SCRIPT flags to the code, but rather making the code that compiles the js-uri never ever compile it using the system principal. In fact, this bug should remove the need for using DISALLOW_SCRIPT flags in a bunch of places I suspect (though i'm not sure on that)

sicking: yeah, that's what I thought you meant. Besides the javascript console, I bet that would break a bunch of extensions, enough that we don't want to do that. We've built a powerful platform. If in our haste to secure it, we start swinging the axe and taking out whole piers, developers whom we break will fall off and never get back on. There's nothing wrong with using javascript: URLs in chrome. What's good for content is good for chrome, often enough. What is content today may be chrome tomorrow, with the necessary changes, and the fewer the better. /be

(In reply to comment #2) >The JS Console doesn't _have_ to use javascript: URIs to do what it does. That depends somewhat on how you define "what it does"... obviously we want to compile and evaluate some script as chrome, but we don't want to accidentally clobber variables declared by console.js, nor do we want to generate spurious strict JavaScript warnings, nor do we want any errors haaving a source of console.js, plus of course we want the result of the evaluation. Now if anyone knows an alternative method of achieving this I'm all ears.

Does javascript: execute JS in a new context? I thought it executed it in the calling context. If it does execute in a new context, do we not have any way of executing JS in a clean context without using javascript:? It seems like it would be a lot neater just to have a simple evalInNewContext() call rather than having to use a _URI_. The reason to fix this bug is that regardless of what we do, we might still have bugs that allow untainted URI injection into elevated-priv environments. It's a defence-in-depth security feature, not a fix for a known problem.

(In reply to comment #7) >Does javascript: execute JS in a new context? The JS console creates a blank frame in which to load the javascript: URL.

Can't we do something equivalent with data: URIs? e.g. data:text/html,<script src="data:text/javascript,%s"> ...? (with suitable levels of escaping at each level of course)

(In reply to comment #9) > Can't we do something equivalent with data: URIs? e.g. > > data:text/html,<script src="data:text/javascript,%s"> As far as I can tell this loses the result of the script.

Good point.

(In reply to comment #8) > >Does javascript: execute JS in a new context? > The JS console creates a blank frame in which to load the javascript: URL. Couldn't you just eval() the javascript in this blank frame? Or insert a <script> with the code?

This is getting ridiculous. Couldn't you do "X", "Y", or "Z"? Couldn't you use C++ or Java? There is no need to ban chrome use of javascript: URLs if they are secure for content to use in the face of cross-site scripting threats. /be

javascript: is a rather dubious feature, in line with document.write(), as far as good programming practices are concerned. It also happens to be one of the most frequently used components in privilege-elevation attacks. By preventing any possibility of javascript: having chrome privs we have defense in depth at a low cost. Personally I think this is one case where it is worth it. But if you disagree, feel free to WONTFIX it.

I'm not interested in opinions on programming purity -- keep that hairshirt covered up, please. It's no more relevant to bring up document.write as a sin to repent, either, since as you note in bug 293363 comment 1, we can't afford to do away with that in the real world, either. The plain fact is that javascript: URLs have been supported for almost ten years now by the majority of all web browsers. We should think twice before removing them from the "chrome" part of our browser platform. I am not about to WONTFIX this bug, but "defense in depth" does not trump all other goods, and we have done a crappy job lately of patching symptoms and breaking our platform, yet not fixing underlying security holes. I think we should slow down and stop the rushing, and the platform gutting. /be

> It's no more relevant to bring up document.write as a sin to repent, either, > since as you note in bug 293363 comment 1, we can't afford to do away with that > in the real world, either. I meant to write "as you note about javascript: URLs on the web", and of course, you are right that we don't support document.write in XUL. People have filed bugs asking for it over the years, though -- something about the heavily procedural DOM APIs is painful to use, and innerHTML and document.write win in the real world. Again, programming purity opinions don't help us with backward compatibility and platform consistency. If it were as simple as saying "defense in depth, any bug recurrence means we turn off the contentious feature", we wouldn't have much of a platform left. We see bugs from our foundations creeping up to the surface floor of our house, and instead of gassing or freezing their nest, we start swinging the axe at the floor boards. I've done this, I'm not pointing a finger at anyone else. It is a product of rushing to patch code not fully understood, and it's bad business. If we have a 0day forcing our hand, maybe it's unavoidable to hack without full understanding, but we have done it blindly too much in Mozilla's history. Firefox 1.0.3 broke a ton of DHTML out there, on major sites. That was a different bug and symptom-patching fix, but the same principle applies. I personally won't take that kind of chance again just to "feel safe". /be

One thing to keep in mind: Mozilla supports pluggable protocol handlers. Any extension can provide a new URL scheme that the browser and the web can use. What we lack is a way for that extension author to fine tune the restrictions placed on that new URL scheme. What I'm getting at is the following: just because we blacklist javascript: URLs in some contexts, that doesn't mean that we have really solved the problem. Other URL schemes could be developed that are just as dangerous. As soon as we find ourselves blacklisting URL schemes, we should stop and think about how this works when some new protocol is introduced -- usually, blacklisting doesn't scale in these cases.

brendan: I wasn't suggesting rushing, I merely suggested this as a possibility. I do think we should do it, and I don't think there is much of an excuse for using javascript: in chrome, but even if that wasn't the case, I'd rather file bugs with ideas like this than just never raise the ideas in the first place. If you disagree that's fine, that's why we have a review process in place. There's no need to be all defensive and abrasive about it. We indeed don't support document.write() in XUL, nor do I think we should. But then I also don't think we should encourage people to use <font> or style="", and for much the same reasons. I understand your argument that there are times where we should be giving authors as much rope as they want, but I also think there are times where we should say "sorry, but this particular suicide method can end up damaging a lot more than just you". And I think the javascript: scheme is such a case. darin: My suggestion would be more along the lines of having protocol handlers be responsible for deciding how elevated they should let their privileges get (and making the javascript: URI say "ooh no, I don't want to have chrome privileges!"). I wouldn't want to hard-code the javascript: scheme into a black-list anywhere. Anyone could write a plugin along the lines of evil:foo where "foo" is hex giving arbitrary code to pass to the processor and execute, with no security checks whatsoever. Or a file:-analogue that always returned file system data, with an origin the same as the referencing document. We have to let such extensions be able to decide how to implement their security.

as i said in bug 293363 comment 7, this *will* break _content in chrome, sicking should be able to figure out why.

Timeless: could you for once explain yourself fully? Hixie: we've had lots of exploits, both XSS and chrome/full-user-privs. Only some of 'em have involved javascript: URLs. Many have involved windows -- should we get rid of windows in the DOM level 0 (they're not in the w3 standardized DOM)? Of course not. I don't see why javascript: URLs are being singled out, either for removal from the chrome side of the platform, or for disproportionate blame for past exploits. It's hard to say what is "proportionate" blame, though, and I have not gone through all past security bugs and laid blame for each. I'm just going by aggregate memory. /be

(I have plans for window.open() which would dramatically reduce the potential for further security holes via windows, not to mention reducing the annoyance caused by those windows, but that's for another bug.) I'm not "singling out" javascript: -- but we like to track one issue per bug so that's why this bug is just about javascript:. There are other things we can do to reduce the overall likelihood of people finding attack vectors, for example tracking the origins of URIs more thoroughly, or not supporting ActiveX, or not allowing people to execute files straight from a download prompt. As for why neutralising javascript: might be a good idea, my opinion, as mentioned, is that the feature is not of great programming value, yet is easy to abuse whenever a string is passed from a potentially hostile origin to chrome and then used as a URI. Passing a URI in this fashion often looks quite innocent, for example used as an image source, and thus it is quite likely for people to write such code without really thinking about it.

(I wasn't talking about window.open, but windows -- including frames and iframes -- in the DOM level 0.) /be

_content is implemented using javascript:this.content. if you prevent javascript: urls from having chrome privs, that's going to fail miserably.

timeless: wrong, see http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#5187 Spurious arguments don't help here. /be

So here's my feelings on this bug: First off we should fix bug 293363 appropriatly since even if we were to fix this bug we wouldn't cover XSS attacks. Then we should evaluate what the value of supporting chrome-js-uris is. If there really are places where it is usefull and couldn't be replaced with a simple eval() or similar then we should keep it. I'm not suggesting we axe a genuinely usefull feature unless there is no other way out (which there is in this case). *If* we do think that we could do this (i.e. if there are good replacements and little use in our code), but doing so would break a lot of extensions then of course we should reevaluate. Then maybe we could try to get the extensions to migrate over to whatever replacement there is and do this for FF 1.5 or some such. So most importantly, lets fix bug 293363 first and reeval this one after that.

Note that var result = Evaluator.eval(code); throws an exception nowadays: >function eval must be called directly, and not by way of a function of another name While var result = Evaluator.Script(code).exec(Evaluator); works around that issue, console.js still gets blamed for any JS warnings and/or execptions.

(In reply to comment #26) > Note that var result = Evaluator.eval(code); throws an exception nowadays: > > > function eval must be called directly, and not by way of a function of > > another name > > While var result = Evaluator.Script(code).exec(Evaluator); works around > that issue, console.js still gets blamed for any JS warnings and/or > execptions. Neil, did you mean to comment in bug 293933? I'll comment over there. /be

anything to do here?

(In reply to Patrick McManus [:mcmanus] from comment #28) > anything to do here? I don't think there is (also predates my time at Mozilla quite significantly). Most likely we should mark that as a WONTFIX to get it off the backlog. Anyway, Jonas is in a better position to make that decision!

We should at [1] make sure that if the window we're about to run JS in uses the system principal, that we immediately bail. [1] http://mxr.mozilla.org/mozilla-central/source/dom/jsurl/nsJSProtocolHandler.cpp#577

want to take that?

Franziskus, can you take a look at this one when you get time, please? Looks like it should be a quick fix, but it if ends up being more complex, we should come back to it next quarter.

Review commit: https://reviewboard.mozilla.org/r/37111/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37111/

I'm not sure if the triggeringPrincipal is the right one here, but that's what used in EvaluateScript.

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz https://reviewboard.mozilla.org/r/37111/#review33663 ::: dom/jsurl/nsJSProtocolHandler.cpp:583 (Diff revision 1) > + if (loadInfo) { > + nsIPrincipal* triggeringPrincipal = loadInfo->TriggeringPrincipal(); > + if (nsContentUtils::IsSystemPrincipal(triggeringPrincipal)) { I'd rather say that you should use the principal of the global object (i.e. window) that we're about to execute the script in. That should generally be the same principal as the triggeringPrincipal, but the window matters just as much I think. Actually, the safest thing is likely to check both the triggeringPrincipal *and* the principal of the window.

The other thing I should add is that I have *no* idea if this will break any Firefox functionality, or any addons.

(In reply to Jonas Sicking (:sicking) from comment #36) > The other thing I should add is that I have *no* idea if this will break any > Firefox functionality, or any addons. (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #37) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cf2300824ba ok, let's see what try says. not sure how we can check if it breaks addons > Actually, the safest thing is likely to check both the triggeringPrincipal *and* the principal of the window. done

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37111/diff/1-2/

> Actually, the safest thing is likely to check both the triggeringPrincipal *and* the principal of the window. That's "safest", but produces the wrong behavior. javascript: only executes if the principal trying to do the load subsumes the principal of the global the script will run in. See nsJSThunk::EvaluateScript. Therefore, it's enough to check that the principal of the global is not system. The principal of the global is also the principal that the script runs with, so that's what we want to be checking anyway. The patch as proposed would break, for example, bookmarklets, because it would disallow running javascript: triggered by chrome in a content window. And this check should be in nsJSThunk::EvaluateScript, where the existing principal checks are done, for at least two reasons: 1) Throwing exceptions from AsyncOpen is generally a bad idea. 2) nsJSThunk::EvaluateScript is where we know the actual principal we would run with (the "objectPrincipal") so can make a sane decision about whether to run.

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz based on bz's comment

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37111/diff/2-3/

thanks for the comments. I'm checking objectPrincipal now in nsJSThunk::EvaluateScript.

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz https://reviewboard.mozilla.org/r/37111/#review35157 Probably better have bz review this. But looks good to me. ::: dom/jsurl/nsJSProtocolHandler.cpp:266 (Diff revision 3) > + // fail if someone tries to load with a system principal Maybe "Fail if someone tries to execute in a global with system principal"

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz https://reviewboard.mozilla.org/r/37111/#review35407 ::: dom/jsurl/nsJSProtocolHandler.cpp:266 (Diff revision 3) > + // fail if someone tries to load with a system principal Capital 'f' at start, period at end, please. r=me with that fixed.

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz r=me, I said. Stupid mozreview.

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37111/diff/3-4/

Comment on attachment 8724673 [details] MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz something went wrong here with the r=bz.