1448359 - Switch Activity-Stream CSP on, including about:welcome

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P2)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 2

[Jonathan Kingston [:jkt] he/him]

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';">

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 4

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4211

Comment 5

[[github robot]]

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

Comment 6

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/9dcaad7a978f

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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 [1], right?

[1] 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 [2]. 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!

[2] <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';">

Comment 8

[Ed Lee :Mardak]

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/Resources/browser/features/[e-mail]!/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

Comment 9

[Kate Hudson :k88hudson]

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

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 11

[Kate Hudson :k88hudson]

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.

Comment 12

[Jonathan Kingston [:jkt] he/him]

Looking at this again, it looks like all of the issues are resolved.

The current policy I am seeing is:
default-src 'none'; object-src 'none'; script-src resource: chrome:; connect-src https:; img-src https: data: blob:; style-src 'unsafe-inline';

The only thing I see that isn't addressed is the unsafe inline styles. Shall we file that as a low priority follow up bug?

Comment 13

[Ed Lee :Mardak]

What are we revisiting or what's the question about "prerendered CSP"? The nightly CSP is set to

<meta http-equiv="Content-Security-Policy" content="default-src 'none'; object-src 'none'; script-src resource: chrome:; connect-src https:; img-src https: data: blob:; style-src 'unsafe-inline';">

Comment 14

[Jonathan Kingston [:jkt] he/him]

Sorry I updated my comment.
Shall I file a bug about the unsafe-inline styles and close this off? Everything else seems fine.

Comment 15

[Ed Lee :Mardak]

The style-src can't be removed now while we have bug 1513311 conditionally adding a dynamically generated stylesheet (where the server can optionally provide selectors and declarations that then get sanitized with CSSOM). This was added for rapid experimentation and dynamic layouts without requiring Firefox code changes for temporary style overrides.

https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/browser/components/newtab/content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx#48-87

If we want to remove that or change how it's implemented, we'll want a separate bug.