409737 - javascript.enabled and docShell.allowJavascript do not disable all event handlers

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Mike Perry]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071222 Remi/2.0.0.11-1 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071222 Remi/2.0.0.11-1 Firefox/2.0.0.11

Javascript event handlers registered with addEventListener are not disabled when either javascript.enabled or docShell.allowJavascript are toggled. Other types of event handlers, such as those registered via the DOM event handler attributes are properly canceled.

Reproducible: Always

Steps to Reproduce:
1. open about:config
2. Visit http://fscked.org/transient/firefoxjsbug.html
3. Click on the page
4. Toggle javascript.enabled to false
5. Click on the firefox bug page again. Observe that some event handlers have been disabled, others have not
Actual Results:  
While some types of event listeners are blocked, other event listeners still fire.

Expected Results:  
No client-side javascript code should execute at all.

This bug is critical to Tor security. The Torbutton extension (https://torbutton.torproject.org/dev/) relies on the ability to prevent pages from executing Javascript and other dynamic content once Tor has been disabled, to prevent pages from performing network activity after the user has changed Tor state.

Comment 1

[Daniel Veditz [:dveditz]]

Confirming bug.

Have you considered adapting the "session restore" functionality and simply closing existing pages, then resurrecting the user's state when they turn off the Tor button? Even if you kill off the remaining event listeners for existing pages I'd still be nervous about letting Tor users interact with them.

Comment 2

[Boris Zbarsky [:bzbarsky]]

What we could probably do is make AutoScriptEvaluate::StartEvaluating return an error if script can't be executed on that cx with the relevant principal.  That somewhat relies on the right cx being used, but we certainly do that with the cx pusher for event listeners.

Unfortunately, other callbacks into JS may not be so lucky (XMLHttpRequest readystate handlers come to mind, though there could also be issues in any DOM API that allows passing in page-implemented callbacks and might call them async... Or even that doesn't return for a long time (e.g. one of the callbacks does sync XMLHttpRequests or some such).  With this last, for example, treewalker's nodefilters could be attacked.  Or the namespace resolver from the proposed DOM API for Selectors.  Or any of a number of other callbacks.  I guess in the long-running situation the calling cx will still be on the stack, so things might be ok and we only need to worry about propagating the right cx into async callbacks...

That would only affect the docshell setting and per-site settings, though.  If we fail to get the right cx but are really async, we'll get the safe context and that's affected by the general "turn off JS" pref.  So at least as a first approximation doing the checks in StartEvaluating would work.

That said, nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject does an OBJ_GET_PROPERTY before calling StartEvaluating.  That doesn't seem safe...  brendan, am I missing something there?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Requesting blocking, but I'm not completely sure it should, to be honest...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not blocking as this is how we've always behaved and it's late in the release cycle. But wanted as it would be nice to have if someone came up with a safe fix.

Comment 5

[Mike Perry]

Would like to point out that many people do depend on this functionality outside of Tor users. Users of Javascript toggling extensions such as QuickJava come to mind as well.

Comment 6

[Brandon Sterne (:bsterne)]

BZ or Jonas, do you have anyone in mind who could work on this?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Smaug might be a good candidate.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Or jst maybe. Jst: is there any way we can disable this on a javascript level? So that no matter which entry points we miss (what about timers) it'll still be blocked?

Comment 9

[Tomas]

Any progress?

Comment 10

[Levente Farkas]

any news during the last few years?

Comment 11

[Boris Zbarsky [:bzbarsky]]

It's a pretty low priority.  I suspect that in the end someone who actually cares about dynamically enabling/disabling JS while a site is live will need to step up and fix this.  I'm happy to point to relevant code as needed.

Comment 12

[Christopher Finke]

docShell.allowJavascript = false;

doesn't prevent JavaScript in the onload="" attribute of images either.  This doesn't just affect websites, but XUL <editor>s as well.  (ScribeFire, in particular.)

I've filed a separate bug for this, but it might be better included in this bug.  I hope someone will fix it, because it greatly impacts major features of the <editor> element, including image drag and drop.

https://bugzilla.mozilla.org/show_bug.cgi?id=524329

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, that's different from this bug, since comment 0 explicitly says that attribute on* handlers _are_ prevented in the case this bug is filed about.

Comment 14

[John J. Barton]

It seems that jsdIStackFrame.executionContext.scriptsEnabled may also be a victim.

Comment 15

[John J. Barton]

We added this code to Firebug 1.5:
  context.eventSuppressor = context.window.getInterface(Ci.nsIDOMWindowUtils);
  if (context.eventSuppressor)
      context.eventSuppressor.suppressEventHandling(true);
in addition to 
  executionContext.scriptsEnabled = false;
when we stop on a breakpoint. 

This immediately solved one bug and we've not seen bad side effects. The only funny business was puzzing: if we call context.window.getInterface() after returning from the debuggers nested event loop, it fails. That is why we save the interface ref in context.eventSuppressor so we can use that to free the window after we return.

Comment 16

[Mike Perry]

(In reply to comment #15)
> We added this code to Firebug 1.5:
>   context.eventSuppressor = context.window.getInterface(Ci.nsIDOMWindowUtils);
>   if (context.eventSuppressor)
>       context.eventSuppressor.suppressEventHandling(true);

John, I've tried doing this call on the contentWindow attribute of the relevant tabbrowser.docShell attribute, but it is disabling all hotkeys in the entire browser window. For example, control-t will not cause that browser window to open a new tab if the tab itself is the currently selected widget (as opposed to the URL bar, for example).

Did you notice this side effect as well? If not, which window are you actually calling suppressEventHandling on?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> ...but it is disabling all hotkeys in the entire
> browser window.
...
> Did you notice this side effect as well?

(Not that this is related to this bug, but) preventing all hotkeys if the focused windows has events suppressed is by design, not a side effect.

Comment 18

[John J. Barton]

Mike, mozilla.dev.platform is a great place to ask such questions. And I'll confirm what Olli said as what we also see.

Comment 19

[Mike Perry]

My apologies. I was just trying to keep some historical record for other people who hit this bug and try the workaround, since it is going on 3 years old, and I've long since lost any hope for an actual fix :)

I will try to investigate more and if needed continue workaround discussion somewhere else...

Comment 20

[obsolete.fax]

Status needs to be changed to Confirmed.

Comment 21

[Johnny Stenback (:jst)]

Not blocking 1.9.3 on this bug.

Comment 22

[shawn.sumin]

The Tor Project / Electronic Frontier Foundation is paying to have this bug fixed.

"If you know C++ and/or Firefox internals, we should be able to pay you for your time to address these issues and shepherd the relevant patches through Mozilla's review process."

Source: https://blog.torproject.org/blog/web-developers-and-firefox-hackers-help-us-firefox-4

Comment 23

[John J. Barton]

The original test case no longer works. But if it did we could see if Firebug 1.7 can block the events. I guess yes, we use nsIDOMWindowUtils suppressEventHandling(true).

Comment 24

[Champignon]

Attachment: Minimal testcase

Since the original test case has disappeared, I took the liberty of making a new one.

Note that this isn't just a privacy issue, it's also a performance issue: sometimes you need to disable Javascript to prevent the browser from slowing to a crawl. The current bug means that it's never possible for the user to completely disable Javascript when it was previously enabled, unless he reloads ALL open pages.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fixed by bug 862627.