Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Explore more CSP tightening (report-only) #14897

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Explore more CSP tightening (report-only) #14897

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd28718
Restrict CSP a bit more (tighten report-only default-src to self)
janbrasna Nov 9, 2024
ab3e5b2
Restrict CSP even more (tighten report-only object-src)
janbrasna Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions bedrock/settings/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,13 @@
]
_csp_img_src = [
"data:",
"mozilla.org",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 'self' covers this already

Copy link
Contributor Author

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…)

"www.googletagmanager.com",
"www.google-analytics.com",
"images.ctfassets.net",
Copy link
Contributor Author

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?

]
_csp_script_src = [
# TODO fix things so that we don't need this
# TODO change settings so we don't need unsafes even in dev
csp.constants.UNSAFE_INLINE,
# TODO snap.svg.js passes a string to Function() which is
# blocked without unsafe-eval. Find a way to remove that.
csp.constants.UNSAFE_EVAL,
"www.googletagmanager.com",
"www.google-analytics.com",
Expand Down Expand Up @@ -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]
Copy link
Member

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.

Copy link
Contributor Author

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…

CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["object-src"] = [csp.constants.NONE]
janbrasna marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

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.

Copy link
Contributor Author

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).

CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["frame-ancestors"] = [csp.constants.NONE]
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["style-src"].remove(csp.constants.UNSAFE_INLINE)

Expand Down