-
Notifications
You must be signed in to change notification settings - Fork 918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Explore more CSP tightening (report-only) #14897
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14897 +/- ##
=======================================
Coverage 78.64% 78.64%
=======================================
Files 156 156
Lines 8171 8173 +2
=======================================
+ Hits 6426 6428 +2
Misses 1745 1745 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
1029f7c
to
ab3e5b2
Compare
@@ -28,16 +28,13 @@ | |||
] | |||
_csp_img_src = [ | |||
"data:", | |||
"mozilla.org", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 'self'
covers this already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically included in "img-src": list(set(_csp_default_src + _csp_img_src))
with _csp_default_src
still including csp.constants.SELF, "*.mozilla.net", "*.mozilla.org", "*.mozilla.com"
(I haven't changed that bit, the defaults still include all the hosts and are appended to all the rules. I'm only trimming the default for report-only
after all of this gets set.)
That's perhaps another thing to RO-testdrive, restrict the default img src hosts to just self (+GA) for images, to see if all are contained internally?
(Not sure if all img src refs are always locally to bedrock, from /media root of the instance even for CMS content in all the envs, demos, staging/integration hosts etc. — e.g. *stage.gcp.moz.works definitely links images from allizom etc., will have to look how the dev extras are being added, so we don't axe the bit that appends those for non-prod use…)
@@ -125,6 +122,8 @@ | |||
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["report-uri"] = csp_ro_report_uri | |||
|
|||
# CSP directive updates we're testing that we hope to move to the enforced policy. | |||
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["default-src"] = [csp.constants.SELF] | |||
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["object-src"] = [csp.constants.NONE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, if object-src
is not set, it uses what is in default-src
. So this is saying we explicitly don't allow object-src
, which is a good report-only test to see what that might trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
I'm not aware of any embed/object use on bedrock, so if this validates it, good practice is to explicitly deny it wholesale (because the elements using it have somewhat crappy support for sandboxing etc., so it's better to not allow them to load at all).
@@ -125,6 +122,8 @@ | |||
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["report-uri"] = csp_ro_report_uri | |||
|
|||
# CSP directive updates we're testing that we hope to move to the enforced policy. | |||
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["default-src"] = [csp.constants.SELF] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I like this. We could go even further and set _csp_default_src
only to [csp.constants.SELF]
and if that looks good we can stop adding these to all the sub-directives. Having the .net
and .com
and .org
(which is SELF) doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically it's a kitchen sink right now, making sure any service or hotlink would work without thinking about it too much… Ideally every policy should enumerate the hosts it really needs, e.g. #11943 (comment) — so that it's restricted to more specific hostnames (like some services.mozilla.com
for connect-src
, assets.mozilla.net
or cdn.mozilla.net
for wherever it's needed etc…)
Which should be the next move in tightening, though. Along with specifically allowing just self/www.m.o where needed, instead of *.m.o that would allow anything (XSS) from e.g. MDN, addons.m.o, discourse.m.o, community.m.o, blog.m.o, support.m.o, bugzilla.m.o, pontoon.m.o etc. with some potential user-submitted resources.
So if we consider ditching the ctfassets img src, I can try (RO) instead of specifically removing it to generally restrict the images to just self+GA and skip the defaults completely — to see if that triggers anything…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that explicit is better than wildcards here.
Grepping the source code...
- For
*.mozilla.net
I see we referenceassets.mozilla.net
andmozorg.cdn.mozilla.net
. - For
*.mozilla.com
I seestatic.mozilla.com
. We link out to a lot of mozilla.com links but we don't need those in our CSP. - For
*.mozilla.org
I don't see anything referencing this, so 'self' should suffice and this can be removed.
@@ -28,16 +28,13 @@ | |||
] | |||
_csp_img_src = [ | |||
"data:", | |||
"mozilla.org", | |||
"www.googletagmanager.com", | |||
"www.google-analytics.com", | |||
"images.ctfassets.net", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, aren't Contentful assets also moved to bedrock? #14198
So this could also be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they should be.
@janbrasna Would you like to merge a step towards the end goal on this PR? Or continue to iterate? IMO it'd be nice to get some of these changes in and watch the reports and continue to tweak the CSP in other future PRs. |
@robhudson Actually I still need to investigate a bit further how to RO-restrict the images without breaking staging, integration tests etc. where the hosts are added via env vars — so I'd actually prefer to do that in a separate step. So if you feel like the changes here are isolated enough to start watching for reports on them, I'd prefer to merge this in the current state, and continue with the discussion in a separate PR if these end up being uneventful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. We can commit and watch the reports to see what pops up. Thank you!
One-line summary
Restricts
default-src
to just self (as we otherwise append all the moz hostnames to the individual policies) and also restrictsobject-src
to none. (All inreport-only
definition for now to confirm it's not needed.)Significant changes and points to review
Adds
object-src: none
to report-only for helping out surface any hypothetical use.Defines
default-src: self
in report-only for surfacing any unspecified defaults used in policies.This doesn't mean everything would have to come from
self
now, the removed hosts are appended to every policy when constructing the settings, so this is only to restrict the defaults, i.e. in case we have not defined the policy (well/correctly/at all).(This will be redone anyways regarding #11943 (comment) when every policy gets its own subset of hosts, instead of appending the default wildcards everywhere…)
Also removes some dupes that were added in recent refactors, removes outdated comments referring to libs no longer used etc.
It's written as to not cause conflicts with #14831 — skipping those two rules here, as they have some Webpack (devserver) and Wagtail (cms-admin) impact.
Issue / Bugzilla link
#14896 (+#11943)
Testing
curl -I http://localhost:8000/de/