Apply Meta CSP to Content Privileged about: pages

RESOLVED FIXED in Firefox 60

Status

()

Product:
Core
Component:
DOM: Security
Type:
enhancement
Importance:
P2
normal
Status:
RESOLVED FIXED
Opened:
Last year
Updated:
8 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Version:
unspecified
Target:
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 3 obsolete attachments)

bug_1436808_meta_csp_about_pages.patch.patch
19.80 KB, patch
ckerschb
: review+
freddyb
: review+
Details | Diff | Splinter Review
Assignee

Description

•
Last year 
No description provided.
Assignee: nobody → ckerschb
Blocks: 1430748
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
 Assignee

Comment 2

•
Last year 
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
 Assignee

Comment 3

•
Last year 
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)
 Assignee

Comment 4

•
Last year 
(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;
 Assignee

Comment 5

•
Last year 
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)
 Assignee

Comment 6

•
Last year 
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 8

•
Last year 
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 9

•
Last year 
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)
 Assignee

Comment 10

•
Last year 
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:'?
 Assignee

Comment 11

•
Last year 
Please see previous comment ^^
Flags: needinfo?(gijskruitbosch+bugs)

Comment 12

•
Last year 
(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)
 Assignee

Comment 13

•
Last year 
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 14

•
Last year 
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+
 Assignee

Comment 15

•
Last year 
(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 17

•
Last year 
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+
 Assignee

Comment 18

•
Last year 
(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.

Comment 19

•
Last year 
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

Comment 20

•
Last year
bugherder 
https://hg.mozilla.org/mozilla-central/rev/c47ba61ce442
Status: ASSIGNED → RESOLVED
Closed: Last year
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Updated

•
Last year
Depends on: 1447384
See Also: → 1449872
No longer blocks: 1452604
Depends on: 1452604
No longer depends on: 1452604

Updated

•
11 months ago
Depends on: 1480934
Depends on: 1499354
You need to log in before you can comment on or make changes to this bug.