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.

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

Update CSP practical guide for consistency #37227

Conversation

chrisdavidmills
Copy link
Contributor

@chrisdavidmills chrisdavidmills commented Dec 16, 2024

Description

After the main MDN CSP guide was updated to follow accepted advice from elsewhere (such as OWASP and Google), it became clear that our practical CSP guide (which is referenced by the HTTP Observatory tool) also needing updating to be consistent with this advice.

This PR attempts to do that. We need to be careful before merging that this is not only correct but also gels with the reports returned by Observatory and Mozilla's sec team.

Also alerting @mozfreddyb and @argl to this PR.

Motivation

Additional details

Related issues and pull requests

@chrisdavidmills chrisdavidmills requested a review from a team as a code owner December 16, 2024 11:15
@chrisdavidmills chrisdavidmills requested review from hamishwillee and removed request for a team and hamishwillee December 16, 2024 11:15
@github-actions github-actions bot added the Content:Security Security docs label Dec 16, 2024
@github-actions github-actions bot added the size/m [PR only] 51-500 LoC changed label Dec 16, 2024
Copy link
Contributor

github-actions bot commented Dec 16, 2024

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you for this, Chris! I had a few comments. I also wonder though: why do we need this document, in this form, as well as https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#strict_csp ? It seems like they cover essentially the same ground, except https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#strict_csp adds some more details, that someone would have to know anyway if they want to implement strict CSP?

Should we consider making this doc much shorter, and just say "implement strict CSP, here's the link"?

@chrisdavidmills
Copy link
Contributor Author

Thank you for this, Chris! I had a few comments. I also wonder though: why do we need this document, in this form, as well as https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#strict_csp ? It seems like they cover essentially the same ground, except https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#strict_csp adds some more details, that someone would have to know anyway if they want to implement strict CSP?

Should we consider making this doc much shorter, and just say "implement strict CSP, here's the link"?

@wbamberg I intended that this document be a short, snappy, "quickfire" guide to what you need to, do, which links back to your guide for a much more in-depth treatment of the topic. It looks from your comments that I perhaps haven't gone far enough. I'll revisit it now, considering your comments. I'll also look at the Observatory scoring criteria, to make sure I don't miss anything out that really needs to be in here.

@chrisdavidmills
Copy link
Contributor Author

Thanks @wbamberg! I also chopped down some of the sections a bit, and removed all the extra examples at the bottom, and they didn't seem that useful really, and diluted the message of the guide. WDYT?

@wbamberg
Copy link
Collaborator

Thanks @wbamberg! I also chopped down some of the sections a bit, and removed all the extra examples at the bottom, and they didn't seem that useful really, and diluted the message of the guide. WDYT?

I'm not going to harp on about this endlessly, but for example, we just landed a guide about XSS, which describes strict CSP as one of the main defenses. The relevant bit is just this: https://developer.mozilla.org/en-US/docs/Web/Security/Attacks/XSS#deploying_a_csp . It doesn't quote a strict CSP, or talk about steps to deploying it, or strict-dynamic, or any other considerations. It really just says very quickly what a strict CSP is, and points to the guide page, which ought to give people all this detail and more. CSP is complicated, and duplicating a lot of these details introduces a lot of scope for errors.

@chrisdavidmills chrisdavidmills requested a review from a team as a code owner December 19, 2024 11:14
@chrisdavidmills chrisdavidmills requested review from hamishwillee and removed request for a team December 19, 2024 11:14
@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Dec 19, 2024
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the Content:HTTP HTTP docs label Dec 19, 2024
@chrisdavidmills chrisdavidmills removed the request for review from hamishwillee December 19, 2024 11:15
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Dec 19, 2024
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, Chris! I like the shape of this document a lot better now. I have a few more comments but they are all little.


## Problem

The main problem this article focuses on is Cross-site scripting ({{Glossary("Cross-site_scripting", "XSS")}}) attacks. These are generally due to a lack of control and awareness of the sources from which site resources are loaded. This problem gets more difficult to manage as sites become larger and more complex and increasingly rely on third-party resources such as JavaScript libraries.

> [!NOTE]
> CSP is one part of a complete strategy for protecting against XSS attacks. There are other factors involved, such as [output encoding](/en-US/docs/Web/Security/Attacks/XSS#output_encoding) and [sanitization](/en-US/docs/Web/Security/Attacks/XSS#sanitization), which are also important.

CSP can also help to fix other problems, which are covered in other articles:

- [Preventing clickjacking](/en-US/docs/Web/Security/Practical_implementation_guides/Clickjacking) by stopping your site being embedded into {{htmlelement("iframe")}} elements. This is done using the CSP [`frame-ancestors`](/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors) directive.
- Preventing [manipulator-in-the-middle](/en-US/docs/Glossary/MitM) (MiTM) attacks by upgrading any HTTP connections to HTTPS. This is helped by the CSP [`upgrade-insecure-requests`](/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors) directive. See [HSTS implementation](/en-US/docs/Web/Security/Practical_implementation_guides/TLS#http_strict_transport_security_implementation) for more details.
Copy link
Collaborator

@wbamberg wbamberg Dec 19, 2024

Choose a reason for hiding this comment

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

Suggested change
- Preventing [manipulator-in-the-middle](/en-US/docs/Glossary/MitM) (MiTM) attacks by upgrading any HTTP connections to HTTPS. This is helped by the CSP [`upgrade-insecure-requests`](/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors) directive. See [HSTS implementation](/en-US/docs/Web/Security/Practical_implementation_guides/TLS#http_strict_transport_security_implementation) for more details.
- Preventing [manipulator-in-the-middle](/en-US/docs/Glossary/MitM) (MiTM) attacks by upgrading any HTTP connections to HTTPS. This is helped by the CSP [`upgrade-insecure-requests`](/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors) directive. See [Upgrading insecure requests](/en-US/docs/Web/HTTP/CSP#upgrading_insecure_requests).

(a better link I think, I am biased though of course!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated ;-)

- [Web workers](/en-US/docs/Web/API/Web_Workers_API) (via [document directives](/en-US/docs/Glossary/Document_directive)).
- Embedded (i.e., {{htmlelement("iframe")}}) content.
- Navigation / form submission destinations (via [navigation directives](/en-US/docs/Glossary/Navigation_directive)).
Strict CSPs are preferred over [location-based](/en-US/docs/Web/HTTP/CSP#location-based_policies) policies, also called allowlist policies, where you specify which domains scripts can be run from. This is because allowlist policies often end up allowing unsafe domains, which defeats the entire point of having a CSP, and they can get very large and unwieldy, especially if you are trying to permit services that require many third party scripts to function.

### Steps for implementing CSP
Copy link
Collaborator

Choose a reason for hiding this comment

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

wonder if it's worth calling this "Steps for implementing a strict CSP"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree; it is not just that — strict CSP is the main thrust of it, but it also covers other things, like alternatives if strict CSP isn't for you, and reporting. I'm going to leave this as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, fair enough.

Comment on lines 41 to 42
1. Begin by trying out a strict CSP, as outlined above, then start to pinpoint resources that are failing to load as a result of the policy.
2. Make sure that external and internal scripts (included via {{htmlelement("script")}} elements) that you want to run have the correct nonce inserted into the [`nonce`](/en-US/docs/Web/HTML/Element/script#nonce) attributes by the server. If you are instead using hashes, external scripts should have the correct hash inserted into [`integrity`](/en-US/docs/Web/HTML/Element/script#integrity) attributes.
Copy link
Collaborator

@wbamberg wbamberg Dec 19, 2024

Choose a reason for hiding this comment

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

This 1-2 reads a little odd to me. I don't think realistically you will start by sending the header and then add nonce/hash to all the scripts that break (I mean for one thing the hashes are directly tied to individual resources). I think it's more likely that you will update your website to generate nonces or hashes and insert them into the documents you serve, in one step.

It might also be worth having a step about, decide whether to use nonces or hashes, which depends on whether you can dynamically generate content to serve (nonces) or need to serve static content (hashes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I have updated the structure, merging the two bullets into one and adding an extra bullet about choosing nonces or hashes.

```
1. Begin by trying out a strict CSP, as outlined above, then start to pinpoint resources that are failing to load as a result of the policy.
2. Make sure that external and internal scripts (included via {{htmlelement("script")}} elements) that you want to run have the correct nonce inserted into the [`nonce`](/en-US/docs/Web/HTML/Element/script#nonce) attributes by the server. If you are instead using hashes, external scripts should have the correct hash inserted into [`integrity`](/en-US/docs/Web/HTML/Element/script#integrity) attributes.
3. If an allowed script goes on to load other, third-party scripts, those scripts will fail to load because they won't have the required nonce or hash. Mitigate this problem by adding the [`strict-dynamic`](/en-US/docs/Web/HTTP/CSP#the_strict-dynamic_keyword) directive, which gives scripts loaded by the first script the same level of trust without being explicitly given a nonce or hash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
3. If an allowed script goes on to load other, third-party scripts, those scripts will fail to load because they won't have the required nonce or hash. Mitigate this problem by adding the [`strict-dynamic`](/en-US/docs/Web/HTTP/CSP#the_strict-dynamic_keyword) directive, which gives scripts loaded by the first script the same level of trust without being explicitly given a nonce or hash.
3. If an allowed script goes on to load third-party scripts, those scripts will fail to load because they won't have the required nonce or hash. Mitigate this problem by adding the [`strict-dynamic`](/en-US/docs/Web/HTTP/CSP#the_strict-dynamic_keyword) directive, which gives scripts loaded by the first script the same level of trust without being explicitly given a nonce or hash.

I think the first comma in "load other, third-party scripts, those scripts will" was ungrammatical, it technically wants to be "load other, third-party, scripts, those scripts will" (i.e. "other scripts that are third-party scripts") but that's horrible of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, your suggestion is much better ;-)

Updated.

Comment on lines 46 to 50
6. If you are still having trouble with certain items, you can consider an alternative, more permissive policy. For example:
- Add specific sources as highlighted during testing; for example, `style-src 'self' https://example.com/`.
- `default-src https:` still provides some protection, disabling unsafe inline/`eval()` and only allowing loading of resources (images, fonts, scripts, etc.) over HTTPS.
- `unsafe-hashes` allows you to use hashes to enable usage of inline event handlers and `style` attribute values.
- `unsafe-eval` allows dynamic evaluation of strings as JavaScript, but beware: it defeats much of the purpose of having a CSP.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
6. If you are still having trouble with certain items, you can consider an alternative, more permissive policy. For example:
- Add specific sources as highlighted during testing; for example, `style-src 'self' https://example.com/`.
- `default-src https:` still provides some protection, disabling unsafe inline/`eval()` and only allowing loading of resources (images, fonts, scripts, etc.) over HTTPS.
- `unsafe-hashes` allows you to use hashes to enable usage of inline event handlers and `style` attribute values.
- `unsafe-eval` allows dynamic evaluation of strings as JavaScript, but beware: it defeats much of the purpose of having a CSP.
6. If you are unable to remove usages of `eval()`, you can add [`unsafe-eval`](/en-US/docs/Web/HTTP/Headers/Content-Security-Policy#unsafe-eval) keyword to your strict CSP: although this makes it significantly weaker.
7. If you are unable to remove event handler attributes, you can add the [`unsafe-hashes`](/en-US/docs/Web/HTTP/Headers/Content-Security-Policy#unsafe-hashes) to your strict CSP: this is somewhat unsafe, but much safer than allowing all inline JavaScript.
8. If you are unable to get a strict CSP to work, an allowlist-based CSP is much better than none, and a CSP like `default-src https:` still provides some protection, disabling unsafe inline/`eval()` and only allowing loading of resources (images, fonts, scripts, etc.) over HTTPS.

So this is just a suggestion but what I'm trying to get at it that your point 6 here seems to me to cover two cases of thing: relaxations which you can make in the context of a strict CSP (unsafe-eval, unsafe-hashes) and departures from a strict CSP. So I'm trying to distinguish these and make it clearer that the things in point 8 are in a place where you've given up on strict CSP.

In particular I thought the previous text could imply that if you just add default-src https to a strict CSP, then it will allow more permissive scripts ("only allowing loading of ... scripts ... over HTTPS.") but it won't of course because strict CSP sets script-src which will be used instead. I know you do say "an alternative, more permissive policy" but I think it's not that clear that this means "abandoning strict CSP".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could even pull step 8 out of the list into a subsequent paragraph, to emphasise that this is a departure from the main approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion is really good. I've included it, but rewritten it a bit.

And yes, I've made 8. a paragraph below the list rather than another bullet. It is a departure rather than a subsequent step on the main approach.

Content-Security-Policy: script-src 'strict-dynamic' 'nonce-2726c7f26c'
```
> [!WARNING]
> If at all possible, avoid including unsafe sources inside your CSP. Examples include `unsafe-inline`, `data:` URIs inside `script-src`, `object-src`, or `default-src`, and overly broad sources or form submission targets. Similarly, the use of `script-src 'self'` can be unsafe for sites with JSONP endpoints. These sites should use a `script-src` that includes the path to their JavaScript source folder(s).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
> If at all possible, avoid including unsafe sources inside your CSP. Examples include `unsafe-inline`, `data:` URIs inside `script-src`, `object-src`, or `default-src`, and overly broad sources or form submission targets. Similarly, the use of `script-src 'self'` can be unsafe for sites with JSONP endpoints. These sites should use a `script-src` that includes the path to their JavaScript source folder(s).
> If at all possible, avoid including unsafe sources inside your CSP. Examples include:
> - `unsafe-inline`
> - `data:` URIs inside `script-src`, `object-src`, or `default-src`
> - overly broad sources or form submission targets
>
> Similarly, the use of `script-src 'self'` can be unsafe for sites with JSONP endpoints. These sites should use a `script-src` that includes the path to their JavaScript source folder(s).

Is the JSONP thing only relevant to self? I thought it was an issue for any kind of location-based policy. I don't really understand this very well though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestion applied. I don't know about the JSONP thing either.


```http-nolint
Reporting-Endpoints: csp-endpoint="https://example.com/csp-reports"
## Report-only CSPs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have made this an H3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree with this. It's not really a standalone topic. Updated.

```http
Content-Security-Policy: default-src 'none'; frame-ancestors 'none'
```
> [!NOTE] > `report-to` is preferred over the deprecated `report-uri`; however, both are still needed because `report-to` does not yet have full cross-browser support.
Copy link
Collaborator

@wbamberg wbamberg Dec 19, 2024

Choose a reason for hiding this comment

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

I guess the > at the start of this is a workaround for the new note format not working when the content starts with a backtick? In this situation Josh uses the old format. You could also do:

Suggested change
> [!NOTE] > `report-to` is preferred over the deprecated `report-uri`; however, both are still needed because `report-to` does not yet have full cross-browser support.
> [!NOTE] The `report-to` directive is preferred over the deprecated `report-uri` directive; however, both are still needed because `report-to` does not yet have full cross-browser support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's just what prettier does to it as a result of the error. I usually fix such instances in the manner you've suggested, but I missed this one. Fixed now!

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, Chris! I'm not merging in case you want feedback from the people tagged in your description, but otherwise I'm happy for this to go.

@chrisdavidmills chrisdavidmills merged commit a7c51be into mdn:main Dec 21, 2024
8 checks passed
@chrisdavidmills chrisdavidmills deleted the fix-csp-practical-implementation-guide branch December 21, 2024 14:21
@chrisdavidmills
Copy link
Contributor Author

Thank you for the updates, Chris! I'm not merging in case you want feedback from the people tagged in your description, but otherwise I'm happy for this to go.

Thank YOU, @wbamberg! I think this is much better now, and it doesn't really lack any information that it had before. I am therefore electing to merge it rather than leave it lying around. If anyone else has concerns about the content update, I can always create a new PR to handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTTP HTTP docs Content:Security Security docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants