Closed Bug 1436808 Opened 4 years ago Closed 4 years ago

Apply Meta CSP to Content Privileged about: pages

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 3 obsolete files)

bug_1436808_meta_csp_about_pages.patch.patch
19.80 KB, patch
ckerschb
: review+
freddy
: review+
Details | Diff | Splinter Review 
No description provided.
Assignee: nobody → ckerschb
Blocks: 1430748
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Some of those pages need a little work so we can remove the 'unsafe-inline' parts of the CSP, but other than that, I think that should work.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66cf79db107bd4a44d84ef9460f329b01c0dabf0
Ulfr, just running this by you to check for you input. I think it should be fairly trivial to add a meta CSP to our packaged about: pages. Let me know if you have any suggestions, thoughts or opinion.
Flags: needinfo?(jvehent)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Ulfr, just running this by you to check for you input. I think it should be
> fairly trivial to add a meta CSP to our packaged about: pages. Let me know
> if you have any suggestions, thoughts or opinion.

As discussed on IRC; ulfr is fine with this; moving forward;
Gijs, ultimately we should apply a CSP to all about pages. Within this bug I want to get the low hanging fruit out of the way; eliminating all inline JS and CSS and applying a CSP of 'default-src chrome:' to the following about pages:

about:mozilla
==> toolkit/content/mozilla.xhtml

about:license
==> toolkit/content/license.html

about:buildconfig
==> toolkit/content/buildconfig.html

about:home
==> browser/base/content/abouthome/aboutHome.xhtml

about:robots
==> browser/base/content/aboutRobots.xhtml

about:reader
==> toolkit/components/reader/content/aboutReader.html

about:feeds
==> browser/components/feeds/content/subscribe.xhtml

about:tabcrashed
==> browser/base/content/aboutTabCrashed.xhtml

There are more content prvileged about pages, but those need a little more attention, so I think it makes sense to tackle them in separate bugs. Eventually I will follow up bugs for:
about:rights
about:certerror
about:blocked
about:neterror
about:checkerboard
Attachment #8949684 - Attachment is obsolete: true
Flags: needinfo?(jvehent)
Attachment #8950512 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8950512 [details] [diff] [review]
bug_1436808_meta_csp_about_pages.patch

Freddy, I think it makes sense for you to also take a look to make sure I am not missing anything - thanks!
Attachment #8950512 - Flags: review?(fbraun)
Comment on attachment 8950512 [details] [diff] [review]
bug_1436808_meta_csp_about_pages.patch

Review of attachment 8950512 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/feeds/content/subscribe.xhtml
@@ +21,5 @@
>  
>  <html id="feedHandler"
>        xmlns="http://www.w3.org/1999/xhtml">
>    <head>
> +    <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />

I don't think this will work because we use this to actually display web-based feeds. We should allow image sources, at least, and maybe video/media sources.

::: toolkit/components/reader/content/aboutReader.html
@@ +1,5 @@
>  <!DOCTYPE html>
>  <html>
>  
>  <head>
> +  <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />

This won't work because <video> and <img> in here are supposed to work with web-based sources.

::: toolkit/content/buildconfig.html
@@ +6,5 @@
>  #filter substitution
>  #include @TOPOBJDIR@/source-repo.h
>  <html>
>  <head>
> +  <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />

We can probably eliminate script and images/media entirely here too.

::: toolkit/content/jar.mn
@@ +49,5 @@
>  #endif
>     content/global/filepicker.properties
>     content/global/globalOverlay.js
>     content/global/mozilla.xhtml
> +   content/global/aboutmozilla.css

Nit: aboutMozilla (capital M).

::: toolkit/content/license.html
@@ +4,5 @@
>     - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
>  
>  <html lang="en">
>    <head>
> +    <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />

Can we eliminate script (and maybe images) entirely here and only allow css chrome: ?

::: toolkit/content/mozilla.xhtml
@@ +11,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>    <head>
> +    <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />

This should also eliminate script entirely (and maybe media/images?) leaving only css.
Attachment #8950512 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8950512 [details] [diff] [review]
bug_1436808_meta_csp_about_pages.patch

Review of attachment 8950512 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good so far!
Please flag again, once you have addressed Gijs' comments and a grenish try run.
Attachment #8950512 - Flags: review?(fbraun)
Thanks for the speedy turnaround!

(In reply to :Gijs from comment #8)
> ::: toolkit/components/reader/content/aboutReader.html
> > +  <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />
> 
> This won't work because <video> and <img> in here are supposed to work with
> web-based sources.

Would http/https be sufficient? e.g. 'default-src chrome:; img-src http: https:'?
Please see previous comment ^^
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Thanks for the speedy turnaround!
> 
> (In reply to :Gijs from comment #8)
> > ::: toolkit/components/reader/content/aboutReader.html
> > > +  <meta http-equiv="Content-Security-Policy" content="default-src chrome:" />
> > 
> > This won't work because <video> and <img> in here are supposed to work with
> > web-based sources.
> 
> Would http/https be sufficient? e.g. 'default-src chrome:; img-src http:
> https:'?

I guess we'll also need to support data: and maybe blob: . Not sure if blob: would work in practice. I don't think we care about ftp: or anything else.

In general the about:reader stuff needs to be fixed via bug 1204818.
Flags: needinfo?(gijskruitbosch+bugs)
Gijs, Freddy, I incorporated your suggestions. To be honest, for me preventing script execution is the most important here, hence I added img-src * instead of listing allowed sources explicitly in the cases of:
* browser/components/feeds/content/subscribe.xhtml
* toolkit/components/reader/content/aboutReader.html
If you feel different, please let me know and we can re-evaluate. 

Also, the reason we didn't have a green try run (see comment 7 for link) was browser_bookmarklet_windowOpen.js. This test relies on about:mozilla and we know that bookmarklets are affected by CSP [1]. I rewrote to use a dummy file instead of about:mozilla.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=866522
Attachment #8950512 - Attachment is obsolete: true
Attachment #8950556 - Flags: review?(gijskruitbosch+bugs)
Attachment #8950556 - Flags: review?(fbraun)
Comment on attachment 8950556 [details] [diff] [review]
bug_1436808_meta_csp_about_pages.patch

Review of attachment 8950556 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks good apart from the about:license thing and the test path nit below. Thanks!

::: browser/components/places/tests/browser/browser_bookmarklet_windowOpen.js
@@ +1,3 @@
>  "use strict";
>  
> +const BASE_URL = "http://example.com/browser/browser/components/places/tests/browser/";

Nit: please use gTestPath etc. instead of hardcoding this, so it's easier to move the test.

::: toolkit/content/license.html
@@ +4,5 @@
>     - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
>  
>  <html lang="en">
>    <head>
> +    <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src chrome:" />

Will this block the images that the style-src sources, given default-src is none?
Attachment #8950556 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #14)
> ::: toolkit/content/license.html
> > +    <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src chrome:" />
> 
> Will this block the images that the style-src sources, given default-src is
> none?

Good catch, I missed that. You are right, the css loads images which would have been blocked. I added img-src chrome: which allows to load those images again.
Attachment #8950556 - Attachment is obsolete: true
Attachment #8950556 - Flags: review?(fbraun)
Attachment #8950647 - Flags: review+
Attachment #8950647 - Flags: review?(fbraun)
Comment on attachment 8950647 [details] [diff] [review]
bug_1436808_meta_csp_about_pages.patch.patch

Review of attachment 8950647 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.
I have a small concern that there might be breakage that isn't covered in tests, given the age of the code and the visibility of those pages.
Can and should we address this with manual testing?
Attachment #8950647 - Flags: review?(fbraun) → review+
(In reply to Frederik Braun [:freddyb] from comment #17)
> I have a small concern that there might be breakage that isn't covered in
> tests, given the age of the code and the visibility of those pages.
> Can and should we address this with manual testing?

I manually tested all of them (at least to the extend that my time allowed). Worst case we have to open up some CSP for some time and file a follow up. Overall, it should work though. I think the tradeoff in risk VS waiting longer is that we should rather land that sooner and get more bake time in Nightly.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c47ba61ce442
Apply Meta CSP to content privileged about: pages. r=gijs,freddyb
https://hg.mozilla.org/mozilla-central/rev/c47ba61ce442
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1447384
See Also: → 1449872
No longer blocks: 1452604
Depends on: 1452604
No longer depends on: 1452604
Depends on: 1480934
Depends on: 1499354
You need to log in before you can comment on or make changes to this bug.