Open Bug 923920 Opened 7 years ago Updated 1 year ago

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

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

People

(Reporter: freddy, Unassigned)

References

(Depends on 7 open bugs, )

Details

(Keywords: meta)

If we want to go with bug 923902, we will have to rewrite the privileged pages and ban all inline JavaScript.
This is a good first bug.
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js]
Can I help?
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.
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...).
Summary: Rewrite inline script in internal privileges pages (about:whatever) → Rewrite inline script/style in internal privileges pages (about:whatever)
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?
Depends on: 948879
Depends on: 948880
Depends on: 948881
Depends on: 948882
Depends on: 948883
Depends on: 948884
Depends on: 948886
Depends on: 948888
Depends on: 948889
Depends on: 948892
Depends on: 948894
Depends on: 948896
Depends on: 948897
Depends on: 948898
Depends on: 948899
Depends on: 948900
(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)
Assignee: nobody → fbraun
Keywords: meta
Summary: Rewrite inline script/style in internal privileges pages (about:whatever) → Tracking: Rewrite inline script/style in internal privileges pages (about:whatever)
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.
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?
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? :-)
Yes, there's enough work to do for more than just you two :) just assign the bugs to yourself (but one by one, please).
hi can I help too here ?
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).
Depends on: 956482, 956484
Depends on: 960566
This tracking bug is not a mentored bug.
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js]
Depends on: 960728
Depends on: 970359
Depends on: 972316
Depends on: 975855
Depends on: 975856
Depends on: 975857
This wiki page explains the project
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.
Depends on: 371900
Depends on: 978115
Depends on: 980911
Depends on: 1016300
Depends on: 1063522
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.
Flags: needinfo?(fbraun)
Depends on: 1119229
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?
Flags: needinfo?(fbraun) → needinfo?(mozilla)
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?
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).
(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?
Flags: needinfo?(mozilla)
(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...
(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?
Assignee: fbraun → nobody
Summary: Tracking: Rewrite inline script/style in internal privileges pages (about:whatever) → [meta] Tracking: Rewrite inline script/style in internal privileges pages (about:whatever)
You need to log in before you can comment on or make changes to this bug.