Open Bug 960728 Opened 6 years ago Updated 4 months ago

[meta] Remove inline scripts/styles in DevTools pages

Categories

(DevTools :: General, task)

task
Not set

Tracking

(Not tracked)

People

(Reporter: bgrins, Unassigned)

References

(Depends on 6 open bugs, Blocks 1 open bug)

Details

(Keywords: meta)

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.
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.
Flags: needinfo?(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
Flags: needinfo?(mgoodwin)
Blocks: 923920
No longer blocks: 923902
> > 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?
Flags: needinfo?(mgoodwin)
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}">
(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.
Flags: needinfo?(mgoodwin)
Depends on: 961165
(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?
(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?
Flags: needinfo?(mgoodwin)
(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).
Flags: needinfo?(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.
No longer depends on: 547189
Depends on: 371900
Depends on: 980331
Depends on: 980333
Depends on: 980335
Depends on: 980903
Depends on: 980910
Depends on: 980913
Depends on: 980915
Depends on: 980919
Depends on: 980951
Depends on: 980954
Depends on: 980956
Product: Firefox → DevTools
Type: defect → task
You need to log in before you can comment on or make changes to this bug.