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)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
(Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 3 obsolete files)
19.80 KB,
patch
|
ckerschb
:
review+
freddy
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → ckerschb
Blocks: 1430748
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
|
||
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)
Assignee | ||
Comment 7•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0dcaa6531404eb16f652c4e65849ea3bb0c0e3
Comment 8•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
Please see previous comment ^^
Flags: needinfo?(gijskruitbosch+bugs)
Comment 12•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
(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+
Assignee | ||
Updated•4 years ago
|
Attachment #8950647 -
Flags: review?(fbraun)
Assignee | ||
Comment 16•4 years ago
|
||
So that should do it now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81eb09d01a0580abca348367fbd2c5186ecce2a6
Comment 17•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c47ba61ce442
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•