-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
CSP guide updates #36157
CSP guide updates #36157
Conversation
Preview URLs External URLs (6)URL:
(comment last updated: 2024-10-22 18:54:44) |
|
||
## Threats | ||
- the `default-src` directive is set to `'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.
Is it worth adding a brief explanation here of each directive? Something like
- the `default-src` directive is set to `'self'` | |
- the `default-src` directive is set to `'self'`, which allows only same-origin resources to be loaded for directives that aren't specified. |
One thing I've always found quite powerful is that default-src is a catchall. The implication being that you typically lock down everything, then progressively allow access. You mention it below, but maybe something here too.
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.
PS I understand this is just the first mention, and you want to show the format of the directive string. Feels a little "abstract" even for an overview.
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.
On reflection, perhaps a paragraph after the image stating something like
The first (default-src
) directive sets a restriction that will be used to allow only same-origin resources to be loaded for directives that aren't specified. The second explicitly ....
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.
Done in 1ada7b0, although I didn't really want to. At this point I only really want to explain that, syntactically, there are these things called directives. By explaining what particular directives do I worry that we're trying to explain everything at once, and making it overwhelming.
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 slightly prefer it as you have now done it, though certainly arguable. If you really hate it feel free to remove.
```http | ||
Content-Security-Policy: default-src 'self' example.com *.example.com | ||
#### Nonces | ||
|
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'm not sure of the best way to do this, but if Nonces/Hashes are the recommended way to manage script resource then it would be better to have them before location based info. Minimally here we might forward reference the CSP Strict section with a note like
Note: Nonces and hashes are recommended over location based restrictions for script resources. See [CSP strict] below ...
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 think structurally it would be straightforward to move "Nonces" and "Hashes" to be the first H4 headings under "Fetch directives". But I don't want to do this yet, I want confirmation that we really do only want to recommend strict CSP. The "Practical security guides" (https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides) that @chrisdavidmills migrated from mozilla.org does not recommend strict CSP, and I'd like to be sure we have consensus for this change.
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 moving them would be better. Who do we need to talk to for consensus?
W.r.t. the implementation guide, that just says "CSP", so its not a for or against the strict kind.
@chrisdavidmills Just FYI it refers to things like Clickjacking protection using iframes, but does not mention CSP in this context or COEP that provide a more layered defense for this particular problem. Not sure if worth addressing.
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 moving them would be better.
Fair enough, I've moved them in 8f6af6d. Still have 'none'
first though.
Who do we need to talk to for consensus?
I think Chris is in touch with the Mozilla security team.
W.r.t. the implementation guide, that just says "CSP", so its not a for or against the strict kind.
It is though, the table row on CSP in https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides#content_security_fundamentals links to https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/CSP#steps_for_implementing_csp, which describes an allowlist-based approach.
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.
OK. I've pinged him directly to look at this. Honestly I think we can do this in any case.
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 think it is OK to go ahead and do. The moz sec folk are being a bit slow at getting back to me on this;
I'll ping them again soon.
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
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.
All those changes look great. See last few comments.
|
||
```http | ||
Content-Security-Policy: | ||
script-src 'nonce-{RANDOM}'; |
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.
- What about stylesheets?
Good question. I just copies this from the web.dev and OWASP articles. One thing that bothers me in general about strict CSP is that it doesn't say anything about controlling other resource types, only JS. @aaronshim , do you know what we should put here?
@wbamberg It would be good if we can address this (note, I have separated into its own thread and resolved the original).
If we're going to recommend strict CSP we should provide a recommendation for the things it does not cover, even if that is just a note that "strict CSP does not cover other types, and these would normally need to be covered by allowlists".
Other than that I have no more comments. I am approving because because I consider this very much better than the original and I have nothing else to add as a reviewer.
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.
In practice, we don't have extensive experience rolling out style-based CSPs the same way we have for nonce-based script-restricting "Strict CSP"s-- partially because of our threat models around style injections-- so we don't have as strong of an opinion on style-src
restriction. Our priority is getting the awareness around "Strict CSP" (specifically around script-src
) out there.
|
||
Websites should protect themselves against XSS by sanitizing this input before including it in the page. A CSP provides a complementary protection, which can protect the website even if sanitization fails. | ||
|
||
If sanitization does fail, there are various forms the injected malicious code can take in the document, including: |
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.
minor nit: Is it worth adding javascript:
URIs as an additional example here? These are tricky because they allow JS injection in link/html contexts even if on* handlers and tag insertion is locked down.
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.
-> 6f3cd1b
files/en-us/web/http/csp/index.md
Outdated
|
||
- We have a separate hash for every script in the document. | ||
- For the external script "main.js", we also include the `integrity` attribute, and give it the same value. | ||
- Unlike the example using nonces, both the CSP and the content can be static, because the hashes stay the same. |
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.
minor suggestion: Should we elaborate on this point to explicitly call out the contrast to the point in the nonce-only policy above where nonce-only policies are mentioned to be unsuitable for static pages, clientside-rendered SPAs, etc, and explicitly suggest a hash-based approach for these use cases?
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.
-> 2c9204f
files/en-us/web/http/csp/index.md
Outdated
|
||
A website administrator for an online banking site wants to ensure that all its content is loaded using TLS, in order to prevent attackers from eavesdropping on requests. | ||
Inline `<script>` elements are allowed if they are protected by a nonce or a hash, as described above. If a directive contains nonce or hash expressions, then the `unsafe-inline` keyword is ignored by browsers. |
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.
minor nit: I wonder whether there is a formatting approach to make this warning pop on the page a bit more, similar to the other warnings?
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'm not sure what's for the best here. I think notes and warnings work best when they describe something "out of line" - like a recommendation that's kind of editorializing over the facts, as we do in the warning just above. I think when a note/warning contains substantive information that's part of the main topic, like this, it's maybe paradoxically easy to miss, because people scan warnings/notes and the main text separately.
So what I've done is just add a linebreak so this appears in its own paragraph, and is less easy to miss.
|
||
The `unsafe-eval` keyword can be used to override this behavior, and as with `unsafe-inline`, and for the same reasons: **developers should avoid `unsafe-eval`**. | ||
|
||
Unlike `unsafe-inline`, the `unsafe-eval` keyword does still work in a directive that contains nonce or hash expressions. |
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.
In practice, we have had some difficulty removing unsafe-eval
from cases where there are legitimate usages, i.e. dynamic module loading-- perhaps this is out of scope, but is it worth mentioning that we can make unsafe-eval
a bit safer by adopting Trusted Types? (Because TT will be an extra check that the strings fed into eval
meet some specified policy.)
We can link it in later as well once we have more documentation describing TT.
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.
-> 268b66b
|
||
The browser will upgrade the first link to HTTPS, but not the second, as it is navigating to a different origin. | ||
|
||
This directive is not a substitute for the {{httpheader("Strict-Transport-Security")}} header (also known as HSTS), because it does not upgrade external links to a site. Sites should include this directive and the `Strict-Transport-Security` header. | ||
|
||
## Testing your policy | ||
|
||
To ease deployment, CSP can be deployed in report-only mode. |
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.
Is it worth calling out that this is not available as a <meta>
tag and link it in earlier in the article when we mention <meta>
tags?
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.
-> 7da65cf
I didn't add a forward link from the bit about meta tags, because that bit is extremely introductory and I wanted to get an overview for people, without them getting bogged down in details yet.
An alternative that might be better would be to have a separate H2 "Delivering your policy", maybe before "Testing your policy", where we could go into all the details? But I think I would like to do that as a follow-up.
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.
Sounds good, thanks-- although I wonder whether how many people will start sending report-only policies through <meta>
tags and whether even a super quick warning can help save some time here.
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.
Never mind, I see the call out right below this thread! Thanks!
Thank you for the comments @aaronshim ! I made most of them and argued with a couple, please let me know what you think. |
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.
Thanks so much for all the work you put in to this. I think this looks great-- but I don't think I can check the "Approval" box in the Github UI (because I don't have explicit access to this repo).
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.
FWIW I still love this following your response to @aaronshim comments :-). Merge at will.
All right, thanks for the reviews! |
This is more or less a rewrite of the CSP guide.
The main motivation is to fix #35812, which says we should document strict (nonce or hash based) CSPs and recommend them over allowlist-based CSPs. So this version adds much more material on how to use nonces and hashes, and outlines (and recommends) strict CSP.
Another motivation is to help address the issue raised in https://publications.cispa.saarland/2986/1/roth2020csp.pdf, which says essentially that because using CSPs to control resource loads has such a bad rep, people don't use the other features of it like clickjacking protection, and the docs don't talk about these other features enough. For example:
and
So in this version I have a single section on "Controlling resource loading" but other sections on "Clickjacking protection" and "Upgrading insecure requests". Especially the second of these, although easy to deploy, is quite a subtle use case that wasn't at all well documented anywhere on MDN as far as I could see.
Various other things.
report-to
, for which we discuss compat, and trusted types, which aren't in this guide.