Closed Bug 1448359 Opened 2 years ago Closed Last year

Switch Activity-Stream CSP on, including about:welcome

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Iteration:
62.4 - Jul 2
Tracking Status
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Mardak, Assigned: k88hudson)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main63-])

Attachments

(1 file)

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.
Blocks: 1448918
Iteration: --- → 62.1 - May 21
Priority: P3 → --
Summary: Get review on our CSP → Switch Activity-Stream CSP on, including about:welcome
Priority: -- → P3
Keywords: sec-want
Iteration: 62.1 - May 21 → 62.2 - Jun 4
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!
Iteration: 62.2 - Jun 4 → 62.3 - Jun 18
Assignee: nobody → khudson
Priority: P3 → P2
Iteration: 62.3 - Jun 18 → 62.4 - Jul 2
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
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Blocks: 1470588
https://hg.mozilla.org/mozilla-central/rev/9dcaad7a978f
Target Milestone: --- → Firefox 63
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';">
Flags: needinfo?(edilee)
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/activity-stream@mozilla.org.xpi!/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
Flags: needinfo?(khudson)
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
Flags: needinfo?(edilee)
Flags: needinfo?(edilee) → needinfo?(khudson)
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?(khudson)
Whiteboard: [adv-main63-]
See Also: → 1490213

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?

Flags: needinfo?(edilee)

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';">
Flags: needinfo?(edilee) → needinfo?(jkt)

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

Flags: needinfo?(jkt) → needinfo?(edilee)

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.

Flags: 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.