Disabling JavaScript by any means prevents event handlers and timers from being executed, even if attached/initiated in Cu.evalInSandbox(), breaks dev tools and NoScript's usability

NEW
Unassigned

Status

()

Core
JavaScript Engine
4 years ago
4 years ago

People

(Reporter: mao, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
Steps to reproduce:
1) Disable JavaScript by any means (e.g. by toggling javascript.enabled to false in  about:config, or by docShell.allowJavascript, or by CAPS, or by Components.utils.blockScriptForGlobal())
2) Open a tab with any content page (e.g. http://mozilla.com)
3) Open a Scratchpad (shift+F4) and run the following script:

addEventListener("click", function() { alert("click") }, false);
setTimeout(function() alert("Timeout"), 1000);

Expected result: an alert box is shown after 1 second, and if you click on the web page you opened you get another alert box.

Actual result: nothing happens.

This is apparently new in Firefox 27, and I suspect it's intended as a "security feature" for some use cases (e.g. disabling JavaScript after the page has been loaded without reloading it), but it's disruptive for some other use cases (e.g. NoScript usability, for instance with bookmarklets, developer tools and probably user scripts) and deserves at least a work-around mechanism.

On a side note, this very problem maybe seen as an (accidental?) fix for bug 409737.
(Reporter)

Updated

4 years ago
Blocks: 409737
> and I suspect it's intended as a "security feature"

It's just the result of finally moving event listeners to WebIDL callbacks.  See bug 862627.

But yes, that fixed a long-standing bug where disabling script in a page would disable some scripts but not others.  WebIDL callbacks were designed such that they would fix that bug.

The current setup is that an event listener will run or not depending on whether script is enabled in the global the event listener comes from.  This means that as long as your global has script enabled you should be able to add event listeners.

You're right that this means you can't add event listeners from bookmarklets on script-disabled pages, but those are not really distinguishable from page script, unfortunately.

For the scratchpad case, it explicitly evaluates code in the page global, which has script disabled.  So yes, the listeners it adds won't work.

User scripts run in their own global so shouldn't be affected.

What's the noscript issue, exactly?
Blocks: 862627
No longer blocks: 409737
Component: JavaScript Engine → DOM
(Reporter)

Comment 2

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)

> What's the noscript issue, exactly?

NoScript for a long time has offered by default a bookmarklet execution "emulation" feature, which tries to mimic as accurately as possible bookmarklet execution also on pages which are not whitelisted (i.e. have JavaScript disabled).

This is a usability feature users expect, see http://forums.informaction.com/viewtopic.php?f=10&t=19191

NoScript also provide so called "Script Surrogates", which are similar to user scripts but run in the context of the page. Those which are designed to run on JavaScript-disabled pages (e.g. to enhance their usability when the original markup depended too much on scripting) are obviously crippled by this change (even though they could arguably work around the issue by moving to an approach more similar to user scripts').
> which tries to mimic as accurately as possible bookmarklet execution also on pages which
> are not whitelisted

OK.  Can you run those in a sandbox whose global has the window on the proto chain?
(Reporter)

Comment 4

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #3)
> > which tries to mimic as accurately as possible bookmarklet execution also on pages which
> > are not whitelisted
> 
> OK.  Can you run those in a sandbox whose global has the window on the proto
> chain?

I'm gonna try as soon as possible (I'm currently traveling), but IIRC there was a problem with assigning window.location (as many bookmarklets do) not working or working weird.
Hmm.  window.location should work fine.  Bareword location, however, might not have... but should now that we have WebIDL bindings.  As long as your sandbox doesn't ask for Xrays, of course.
(Reporter)

Comment 6

4 years ago
It seems to work, indeed.
So a work-around exists, and it could also be implemented by dev tools.
Should I open a followup bug for that, or just morph this one and relocate the component?
Component: DOM → JavaScript Engine
Jim, which would you prefer?

Note that a separate global has its own issues, starting with its own Array and whatnot...
Flags: needinfo?(jimb)

Comment 8

4 years ago
I don't mind morphing this bug (if that's what you're asking?).

(In reply to Boris Zbarsky [:bz] from comment #1)
> The current setup is that an event listener will run or not depending on
> whether script is enabled in the global the event listener comes from.  This
> means that as long as your global has script enabled you should be able to
> add event listeners.

So, in some well-defined place, we check the listener's global before running it?

Would it be feasible to loosen these rules slightly so that the listener would be permitted to run depending on whether script is enabled in the listener's global *or* in the listener's compilation unit?

That is, suppose add-ons like Scratchpad and NoScript, via a new chrome-only primitive, could set a new JS::CompileOptions boolean flag, 'overrideDisabledScript', say, when they evaluate code in the content global. The result would be functions that close over the content global on which script is disabled, but whose JSScripts' ScriptSource objects remember the 'overrideDisabledScript' flag set when they were compiled. Thus, they would run in the correct global, but be identifiable as having special permissions.

Thus, add-ons using this new magic chrome-only eval-like primitive could introduce JS code that would run even in script-disabled globals.

I don't know anything about how disabling script actually works, so perhaps this would be horrible to work around; I just thought I'd throw the idea out there --- we have the ability (if useful) to discriminate on a per-compilation-unit basis.

(I've never really understood what the "origin principals" are; could we use that instead of a new flag? Probably not...)
Flags: needinfo?(jimb)
> So, in some well-defined place, we check the listener's global before running it?

Yes, precisely.  It happens right here: http://hg.mozilla.org/mozilla-central/file/879038dcacb7/dom/bindings/CallbackObject.cpp#l135

> *or* in the listener's compilation unit?

I assume we're ignoring for the moment things like addEventListener("click", { handleEvent: function() {} })?  I guess that case is pretty rare anyway.

I would be fine with doing something like this; my only constraint is that this check needs to be reasonably fast.  I guess we'd only do the check if !ScriptAllowed(), which would automatically make it a somewhat rare case....

> I don't know anything about how disabling script actually works

Basically we have a global, we get its compartment, we get the compartment private, and then we have some booleans and integers in there that we read.

Disabling script will twiddle those booleans and integers, depending on how you do it.

In our case we'd just do the check in CallSetup there, so it wouldn't be affected by how ScriptAllowed works.

> I've never really understood what the "origin principals" are

They're a way of not leaking cross-site information via window.onerror.  If a page on host A uses <script src="some-URI"> to load a non-script thing, typically we will end up with a SyntaxError and report it via window.onerror, including the line of text the error was on.  But if some-URI was cross-origin, we don't want to leak the text.  So we track where the script was loaded from via originPrincipals (unlike principals, which have to be those of the page) and use that when handling the SyntaxError to decide whether to tell onerror the source line.

Anyway, ccing bholley, since I bet he's very interested in this.

Comment 10

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #9)
> I would be fine with doing something like this; my only constraint is that
> this check needs to be reasonably fast.  I guess we'd only do the check if
> !ScriptAllowed(), which would automatically make it a somewhat rare case....

The compilation unit check would be, in effect:

  fun->as<JSFunction>().nonLazyScript()->scriptSource().overrideDisabledScript()

with an 'if' with a similar-looking case to handle lazy functions. (Lazy functions would not need to be de-lazified; LazyScript knows its ScriptSource, too - as it obviously must.) Obviously, we'd wrap all that up in a JSAPI call.

So that's several steps of pointer chasing, and two branches (the script-to-source step may need to dereference a CCW in rare cases), but no shape lookups or hash table lookups or anything like that. And as you say, it would only occur when !ScriptAllowed().


> So we track
> where the script was loaded from via originPrincipals (unlike principals,
> which have to be those of the page) and use that when handling the
> SyntaxError to decide whether to tell onerror the source line.

Okay, so it does reflect the origin of the code, independent of the global it's running in. That's similar to what we want. I guess we could always run code whose origin principals were trusted; but it seems to me that the originPrincipals are meant to serve a very specific purpose, whose resemblance to what we want here is mostly coincidental. That sounds like something we don't want to drag into this. I was just curious.
Yeah, let's not overload originPrincipal here.

Bobby, how does Jim's proposal for an explicit flag on the JSScript and special-casing function objects here sound?
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] from comment #11)
> Yeah, let's not overload originPrincipal here.
> 
> Bobby, how does Jim's proposal for an explicit flag on the JSScript and
> special-casing function objects here sound?

It sounds mostly good, I think.

There are a few cases where we want to block script even for privileged code:
http://dxr.mozilla.org/mozilla-central/source/js/jsd/jsd_xpc.cpp#1768
http://dxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#3428

What do we want to do about those here?
Flags: needinfo?(bobbyholley)
Hmm.  So by default privileged code would _not_ get the overrideDisabledScript flag.  You'd have to explicitly opt in to that, right?

I guess if we have cases when we want to block script even for the overrideDisabledScript case then we need to track those on the compartment private and the "is script enabled" check would allow those to override even the override flag.
(In reply to Boris Zbarsky [:bz] from comment #13)
> Hmm.  So by default privileged code would _not_ get the
> overrideDisabledScript flag.  You'd have to explicitly opt in to that, right?

Right.

> I guess if we have cases when we want to block script even for the
> overrideDisabledScript case then we need to track those on the compartment
> private and the "is script enabled" check would allow those to override even
> the override flag.

I think we can just expose mScriptBlocks separately from Allowed(), and make that unoverrideable.
You need to log in before you can comment on or make changes to this bug.