Closed Bug 1448359 Opened 2 years ago Closed Last year
Switch Activity-Stream CSP on, including about:welcome
52 bytes, text/x-github-pull-request
|Details | Review|
From https://github.com/mozilla/activity-stream/issues/3045 We added a CSP via meta tag in activity-stream.html, but right now it's set to Report-Only. It would be nice to get someone from security to review our current CSP It would also be helpful to know if there's a way to prevent us from being iframed from a tag CSP. frame-ancestors didn't appear to work when tried.
The recently landed bug 1449845 requires new about: pages to have CSP policies, and currently about:newtab and about:home are grandfathered into a whitelist. For about:welcome, therefore, we need to do something. @ewright figured out a smart solution for just doing that, but after some discussion, we think we'd rather just get all three birds knocked out with one stone, rather than a) entraining our (possibly non-trivial) CSP cleanup and fallout handling in an essentially unrelated patch which is already large or b) leaving the code & security story in an arbitrarily inconsistent state that may (or may not) be vulnerable to racing. So, bumping this up to 62.1 and making it block about:welcome.
Iteration: --- → 62.1 - May 21
Priority: P3 → --
Summary: Get review on our CSP → Switch Activity-Stream CSP on, including about:welcome
Current policy: script-src 'unsafe-inline'; img-src http: https: data: blob:; style-src 'unsafe-inline'; child-src 'none'; object-src 'none'; script-src: - Can the script loading at the bottom of about:welcome be done inside a script of it's own to remove the use of unsafe-inline scripts? - Don't you need chrome: and resource: here? img-src: ✔️ style-src: Don't you need chrome: and resource: here? child-src -> frame-src object-src: ✔️ Other comments: - We should likely add a default-src of 'none' to simplify and secure the CSP - Also is the reporting even working as it's delivered by meta tag? - Did the reporting get a data review? - My other concern is that the CSP is in each translation html file in central. Won't this increase the risk that it gets modified accidentally on a locale? Or are these files generated like the path suggests? I would suggest the following would be possible: <meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src chrome: resource:; img-src http: https: data: blob:; style-src chrome: resource: 'unsafe-inline';">
Thanks :jkt; A few more notes: * Ideally we should not make use of 'unsafe-inline' at all, can we change the code so we do not need 'unsafe-inline' in the CSP? * Do we need img-src http: as well? I thought we load everything over https anyway? * default-src 'none' would be great to have. * having object-src 'none' is great! Happy to discuss in a meeting if needed. Thanks for adding a CSP!
Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/8d599e1775483355b7e844e734faff55be33d525 feat(prerender): Enable CSP for Activity Stream (#4211) Fix Bug 1448359 - Switch Activity-Stream CSP on, including about:welcome
Hey Ed, it's great to see that we are moving from a report-only policy to a policy that is going to enforced. I have two quick questions: 1) Is the CSP shipped here going to be applied to all about:newtab as well as about:welcome pages? If so, that would allow us to remove about:newtab as well as about:welcome (potentially about:home) from the whitelist of the 'every content privileged about page has a CSP' here , right?  https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2507 2) I see that the CSP applied contains 'unsafe-inline' within script-src as well as style-src . I do understand that applying a strong CSP is not an easy undertaking. I am mainly curious if we have a plan to tighten that CSP further for upcoming releases? I am more than happy to chat if I/we can be of any help here - thanks!  <meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'unsafe-inline' resource: chrome:; connect-src https:; img-src https: data: blob:; style-src 'unsafe-inline';">
k88hudson can provide more details for comment 7 1) Yes, it's applied to all about:newtab about:home and about:welcome view-source:about:newtab view-source:about:home view-source:about:welcome (They all by default load the same file: FirefoxNightly.app/Contents/Resourcesfirstname.lastname@example.org!/chrome/content/prerendered/en-US/activity-stream.html) 2) I believe the unsafe-inline is to allow for existing snippets, which we're in the process of replacing with Activity Stream Router and templates
Flags: needinfo?(edilee) → needinfo?(khudson)
The reason for unsafe in-line in script and style is indeed because of the implementation of snippets; changing this requires a rewrite of the system. We do have a plan to replace this by 63, the metabug for which is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1432588
Hi, do you plan to request this patch to be uplifted to 62 beta or can I change the tracking flag for Firefox 62 from affected to wontfix? Thanks
If this didn't make it into 62, I think it's ok just to have it land with 63 (i.e. wontfix for 62) unless anyone else has any objections.
Flags: needinfo?(edilee) → needinfo?(jkt)
Flags: needinfo?(jkt) → needinfo?(edilee)
See Also: → 1513311
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.