923920 - [meta] Tracking: Rewrite inline script/style in internal privileges pages (about:whatever)

Status: NEW

(Firefox :: General, defect)

Description

[Frederik Braun [:freddy]]

If we want to go with bug 923902, we will have to rewrite the privileged pages and ban all inline JavaScript.

Comment 1

[Frederik Braun [:freddy]]

This is a good first bug.

Comment 2

[Afshin Mehrabani]

Can I help?

Comment 3

[Frederik Braun [:freddy]]

If you think you're up to it, yes totally. It involves quite a bit of reading  & version control overhead, but I think it's worth it.

You might have to learn using Mercurial, our version control system, which is a bit complicated.
Generic Mercurial tips are here <https://developer.mozilla.org/en-US/docs/Mercurial> and a how the workflow for maintaining a patch on top of the Mozilla repo is explained here <https://developer.mozilla.org/en/docs/Mercurial_Queues#How_to_use_MQ_for_Mozilla_development>.

The whole process to submit a patch is outlined in <https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch>, but if you have more concrete questions, we can discuss out of band. I am usually available for chat in #security on irc.mozilla.org as freddyb.

Comment 4

[Frederik Braun [:freddy]]

One could also just find the HTML/JS pages and create a .diff file manually.
The (X)HTML filenames are mentioned in here, for example <http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js>.

Finding them is easy with our code search engines on http://mxr.mozilla.org/mozilla-central (and dxr.mozilla...).

Comment 5

[Afshin Mehrabani]

I think I don't have any problems with the source control. A clue about how to do it would be great, also I will ask more questions on IRC.

Can I be the fixer?

Comment 6

[Frederik Braun [:freddy]]

(In reply to Afshin Mehrabani from comment #5)
> I think I don't have any problems with the source control. A clue about how
> to do it would be great, also I will ask more questions on IRC.
> 
> Can I be the fixer?

We will use this bug to track the individual files.
You can take one of the above by looking at "assigned to" and clicking "take".

There are probably a lot more files for Social API, Sync and so on. I will track them down at a later point.
(See http://dxr.mozilla.org/mozilla-central/source/browser/base/jar.mn#7)

Comment 7

[Frederik Braun [:freddy]]

I just realized this is going to be a bit tricky for people who haven't heard of our XML-dialect XUL yet. Some inline JavaScript handlers are hidden behind XML attributes like "command", which would need to be rewritten too.

Comment 8

[Afshin Mehrabani]

Well, I put a comment on one of subsets. Is there any resource for getting information about XUL that you mentioned in the previous comment?

Comment 9

[Frederik Braun [:freddy]]

Yes! See https://developer.mozilla.org/en-US/docs/XUL

Comment 10

[desiradaniel2007]

I believe I also am more than good enough to fix this. I have already stared at the source code for about pages and XUL seems to be very similar to XHTML. Shall we work in pair? :-)

Comment 11

[Frederik Braun [:freddy]]

Yes, there's enough work to do for more than just you two :) just assign the bugs to yourself (but one by one, please).

Comment 12

[Kumar Rishabh]

hi can I help too here ?

Comment 13

[Frederik Braun [:freddy]]

Can we please try to keep the noise level down?
Just pick one of the bugs marked in "depends on" and assign them to yourself (if unassigned).

Comment 14

[Frederik Braun [:freddy]]

This tracking bug is not a mentored bug.

Comment 15

[Frederik Braun [:freddy]]

This wiki page explains the project

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We may need to look into finally fixing bug 371900 - I hit this when reviewing bug 948888, and had to shoe-horn in a temporary hack. Thankfully <key> elements aren't currently too common in in-content pages - but we'll want to be using them more and more.

Comment 17

[:Gijs (he/him)]

What are we doing to prevent "accidentally" introducing new inline style/script here? For instance, bug 1016300 cleaned up the in-content prefs, but since then new attributes have been added in. This feels, as the Dutch would say, like mopping the floor while the tap is open.

Comment 18

[Frederik Braun [:freddy]]

We could apply a Content Security Policy per-file. Even a relatively lax policy could disallow them.
We already do something like this for Firefox Hello, see https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2968

In the long run, I'm sure, we don't want a long if-statement that goes through a list of pages that get an extra CSP. Maybe the idea in bug 965637 helps.
Chris, do you have a better suggestion on how to apply CSP to internal pages?

Comment 19

[:Gijs (he/him)]

Seems like the about handling code ( http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp ) could take care of doing this on a per-about-page basis?

Comment 20

[:Gijs (he/him)]

Specifically, we could add an extra flag in the aboutredirector, and the code you pointed to could check the channel's originalURI. If it exists and is using the about: scheme, we can request the flags using nsIAboutModule's GetURIFlags method. This will basically be a simplification of the about:loop detecting code (because we won't need to hardcode URIs in there).

The downside is that we lose some flexibility, in that we can't easily have a different CSP list for every about module. If we wanted that, we'd need to extend nsIAboutModule to provide a separate method to return the CSP string on a per-page basis (that we'd have to then also store separately... although we could use a generic network.aboutcsp.<pagename> pref structure for that if all else fails).

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #18)
> We could apply a Content Security Policy per-file. Even a relatively lax
> policy could disallow them.
> We already do something like this for Firefox Hello, see
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2968
> 
> In the long run, I'm sure, we don't want a long if-statement that goes
> through a list of pages that get an extra CSP. Maybe the idea in bug 965637
> helps.
> Chris, do you have a better suggestion on how to apply CSP to internal pages?

So, there are several issues we need to consider, let me try to clarify:
* All about: pages share the same principal instance. So all about: pages would have to use the same CSP. In other words, even the discussed if-else approach to apply different CSPs wouldn't work here.

* Potentially we could move the CSP into nsIDocument (or some object that hangs off an nsIDocument). At the moment I am not sure what other implications such a move might have.

* Please note, that the term nsILoadInfo is overloaded in that context. Currently we use the nsILoadInfo that hangs off a *channel* not a document. It would *not* make sense to move the CSP into that nsILoadInfo, because we would need a CSP per page and not only for one request. I know you were talking about moving the CSP into nsIDocument, which makes more sense. Just trying to avoid any confusion because the refed Bug 965637 has nsILoadInfo in it's title.

* What do internal pages load? Would we really need a different CSP for all of them? Maybe we can assemble one CSP that fits all?

Comment 22

[Frederik Braun [:freddy]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> So, there are several issues we need to consider, let me try to clarify:
> * All about: pages share the same principal instance. So all about: pages
> would have to use the same CSP. In other words, even the discussed if-else
> approach to apply different CSPs wouldn't work here.
> 

Right. I thought this may be possible because the loop panel somehow gets a CSP: But I now realize that it's an unprivileged page, that has a different principal

> * Potentially we could move the CSP into nsIDocument (or some object that
> hangs off an nsIDocument). At the moment I am not sure what other
> implications such a move might have.
> 
> * Please note, that the term nsILoadInfo is overloaded in that context.
> Currently we use the nsILoadInfo that hangs off a *channel* not a document.
> It would *not* make sense to move the CSP into that nsILoadInfo, because we
> would need a CSP per page and not only for one request. I know you were
> talking about moving the CSP into nsIDocument, which makes more sense. Just
> trying to avoid any confusion because the refed Bug 965637 has nsILoadInfo
> in it's title.
> 

I'm not versed enough in the codebase to argue for or against either of the possibilities. :)

> * What do internal pages load? Would we really need a different CSP for all
> of them? Maybe we can assemble one CSP that fits all?

Rewriting a pages takes a lot of time and testing and rewrites also bitrot quite quickly, unless the CSP coding style (e.g. no inline scripts) is enforced. So it would be preferred to enable pages individually.

Alternatively, we could try to provide tests that prevent upcoming changes to introduce new (theoretical) CSP violations by counting inline scripts, on* attributes and so on...

Comment 23

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #22)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> > So, there are several issues we need to consider, let me try to clarify:
> > * All about: pages share the same principal instance. So all about: pages
> > would have to use the same CSP. In other words, even the discussed if-else
> > approach to apply different CSPs wouldn't work here.
> > 
> 
> Right. I thought this may be possible because the loop panel somehow gets a
> CSP: But I now realize that it's an unprivileged page, that has a different
> principal

This doesn't really make sense to me. Some about: pages are privileged, and some are not. See http://mxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#26 . Are we talking about different kinds of "principal"s ? Or not?