960728 - [meta] Remove inline scripts/styles in DevTools pages

Status: NEW

(DevTools :: General, task)

Description

[Brian Grinstead [:bgrins]]

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript and styles.  This bug will track the DevTools files that need updating.

Comment 1

[Brian Grinstead [:bgrins]]

Mark,
Just checking on a couple of files as to whether they need their scripts moved into external files or not:

1) https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor.xul.  It is a xul document with a bunch of event handlers added oncommand and onclick.
2) https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.xhtml#22.  Inside of the template there are some inline handlers registered, however, the template itself is being built from an external script file.

Comment 2

[Mark Goodwin [:mgoodwin]]

Thanks for filing this Brian.

Some context might be useful; we're doing this to pave the way for applying a CSP to prevent (primarily) DOM XSS issues in parts of the browser UI (we've had a few of these in the past in devtools) creating arbitrary code (chrome JS) execution issues.

Policies can be crafted which allow script in various attributes but doing so makes the control essentially useless.  For this reason we're trying to 
a) eliminate inline scripts and styles (including script valued attributes)
b) understand cases where this is impossible (or prohibitively hard), and
c) work out strategies for dealing with the impossible or hard cases (e.g. can we apply relaxed policies to only the documents that need them) 

(In reply to Brian Grinstead [:bgrins] from comment #1)
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/
> netmonitor.xul.  It is a xul document with a bunch of event handlers added
> oncommand and onclick.

Yes - this would need moving

> 2)
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.xhtml#22.  Inside of the template there are some inline handlers
> registered, however, the template itself is being built from an external
> script file.

This looks harder but, again, we'd want to be able to apply a CSP to this document too, if possible

Comment 3

[Brian Grinstead [:bgrins]]

> > 2)
> > https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> > markup-view.xhtml#22.  Inside of the template there are some inline handlers
> > registered, however, the template itself is being built from an external
> > script file.
> 
> This looks harder but, again, we'd want to be able to apply a CSP to this
> document too, if possible

OK, I've chatted with a couple people a bit more about what is happening here, and want to run something by you.

We are using domtemplate for this templating (https://github.com/joewalker/domtemplate).  Something I wasn't aware of when I first asked this is:

> Although it looks like we are using DOM level 0 event registration
> (i.e. element.onfoo = somefunc) we are actually using DOM level 2,
> by stripping off the 'on' prefix and then using addEventListener('foo', ...).

From https://github.com/joewalker/domtemplate#registration-of-event-handlers-onclickfunction.

So the library basically calls cloneNode on the elements inside <div id="templates" style="display:none"> as seen in the markup-view.xhtml, and replaces the inline onclick="" calls with addEventListener("click").

I hope I've explained well enough, but does this still sound like something that would need to change?

Comment 4

[Joe Walker [:jwalker]]

FWIW, the source is https://hg.mozilla.org/integration/fx-team/file/ff7083dc49b4/toolkit/devtools/gcli/Templater.jsm#l144

So we're doing DOM-L2 for:

    { value: function() { alert('hi'); } }

    <div onclick="${value}">

This is likely to be the majority case.

However I realize that there are 2 cases where we do use DOM-L0

    { value: "data" }

    <div onclick="alert('${value}')">

And

    { value: "alert('hi')" }

    <div onclick="${value}">

Comment 5

[Mark Goodwin [:mgoodwin]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> I hope I've explained well enough, but does this still sound like something
> that would need to change?

It doesn't sound like it needs to change insomuch as CSP will not prevent this from working; we do need to be careful that this doesn't provide a route for a partial CSP bypass though (if in something similar to Joe's DOM-L2 example in comment 4 the attacker can put <div onmouseover="${existing_destructive_function}"> in a dangerous place that could be bad - but not as bad as full XSS).

(In reply to Joe Walker [:jwalker] from comment #4)
> However I realize that there are 2 cases where we do use DOM-L0

The DOM-L0 cases would break under a strict-enough CSP.

Comment 6

[Joe Walker [:jwalker]]

(In reply to Mark Goodwin [:mgoodwin] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > I hope I've explained well enough, but does this still sound like something
> > that would need to change?
> 
> It doesn't sound like it needs to change insomuch as CSP will not prevent
> this from working; we do need to be careful that this doesn't provide a
> route for a partial CSP bypass though (if in something similar to Joe's
> DOM-L2 example in comment 4 the attacker can put <div
> onmouseover="${existing_destructive_function}"> in a dangerous place that
> could be bad - but not as bad as full XSS).

The good news is that 'existing_destructive_function' would have to be explicitly made available to the templater. So that's additional safety.

> (In reply to Joe Walker [:jwalker] from comment #4)
> > However I realize that there are 2 cases where we do use DOM-L0
> 
> The DOM-L0 cases would break under a strict-enough CSP.

I think that breaking DOM-L0 is probably ok. We could instrument domtemplate to tell us if it was used, and I don't think it's common anyway.


In addition: It should be true that domtemplate encourages or even mandates that templates are hard coded, which may make all of this mute?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Mark Goodwin [:mgoodwin] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > I hope I've explained well enough, but does this still sound like something
> > that would need to change?
> 
> It doesn't sound like it needs to change insomuch as CSP will not prevent
> this from working; we do need to be careful that this doesn't provide a
> route for a partial CSP bypass though (if in something similar to Joe's
> DOM-L2 example in comment 4 the attacker can put <div
> onmouseover="${existing_destructive_function}"> in a dangerous place that
> could be bad - but not as bad as full XSS).
> 

Is there a way to apply a CSP and see what breaks, as a way of quickly figuring out what pages still need inline scripts removed and testing that ones that we've updated to make sure that we didn't miss anything?

Comment 8

[Garrett Robinson [:grobinson]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Is there a way to apply a CSP and see what breaks, as a way of quickly
> figuring out what pages still need inline scripts removed and testing that
> ones that we've updated to make sure that we didn't miss anything?

Yes - you can use the CSP-Report-Only header. This will detect violations and trigger a report for each one, but will not enforce the CSP (so the page will continue to work). These reports will be logged to the Browser Console. You can also specify a report-uri for the browser to POST JSON-encoded reports to (for local testing, you might be able to specify localhost - not sure if that would work).

Comment 9

[Mark Goodwin [:mgoodwin]]

(In reply to Garrett Robinson [:grobinson] from comment #8)
> Yes - you can use the CSP-Report-Only header. This will detect violations
> and trigger a report for each one, but will not enforce the CSP (so the page
> will continue to work). These reports will be logged to the Browser Console.
> You can also specify a report-uri for the browser to POST JSON-encoded
> reports to (for local testing, you might be able to specify localhost - not
> sure if that would work).

This wouldn't work in chrome documents, would it? I was under the impression we were waiting on some other changes before we'd be able to test any of the changes we're making to facilitate this in Firefox.