Closed Bug 293394 Opened 19 years ago Closed 8 years ago

javascript: should never execute with chrome privs

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: ian, Assigned: franziskus)

Details

(Keywords: sec-want, Whiteboard: [sg:want?][necko-backlog][adv-main48-])

Attachments

(1 file)

MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz
58 bytes, text/x-review-board-request
franziskus
: review+
Details 
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.
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
Whiteboard: [sg:investigate]
Assignee: darin → nobody
QA Contact: benc → networking
Whiteboard: [sg:investigate] → [sg:want?]
anything to do here?
Flags: needinfo?(mozilla)
Whiteboard: [sg:want?] → [sg:want?][necko-backlog]
(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!
Flags: needinfo?(mozilla) → needinfo?(jonas)
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
Flags: needinfo?(jonas)
want to take that?
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla) → needinfo?(franziskuskiefer)
I'm not sure if the triggeringPrincipal is the right one here, but that's what used in EvaluateScript.
Assignee: nobody → franziskuskiefer
Flags: needinfo?(franziskuskiefer)
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.
Attachment #8724673 - Flags: review?(jonas)
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/
Attachment #8724673 - Flags: review?(jonas)
> 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
Attachment #8724673 - Flags: review?(jonas) → review-
Attachment #8724673 - Flags: review- → review?(jonas)
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"
Attachment #8724673 - Flags: review?(jonas)
Attachment #8724673 - Flags: review?(bzbarsky)
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.
Attachment #8724673 - Flags: review?(bzbarsky)
Comment on attachment 8724673 [details]
MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz

r=me, I said.  Stupid mozreview.
Attachment #8724673 - Flags: review+
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/
Attachment #8724673 - Attachment description: MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r?sicking → MozReview Request: Bug 293394 - javascript: should never execute with chrome privs, r=bz
Attachment #8724673 - Flags: review+ → review?(bzbarsky)
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.
Attachment #8724673 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1108512d5ba1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Whiteboard: [sg:want?][necko-backlog] → [sg:want?][necko-backlog][adv-main48-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: