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

Validate CSP directives #325

Closed
gthess opened this issue Jun 4, 2018 · 43 comments
Closed

Validate CSP directives #325

gthess opened this issue Jun 4, 2018 · 43 comments

Comments

@gthess
Copy link
Collaborator

gthess commented Jun 4, 2018

Check that at least the bare minimum sane CSP directives are configured.
This also means that internet.nl's configuration needs to comply with this.

@baknu
Copy link
Contributor

baknu commented Jun 19, 2018

@gthess
Copy link
Collaborator Author

gthess commented Sep 21, 2018

This is related to #79

@gthess
Copy link
Collaborator Author

gthess commented Sep 21, 2018

@baknu
Copy link
Contributor

baknu commented Jul 31, 2020

First step is to test for 'unsafe-inline'.

See also: https://en.internet.nl/article/x-xss-protection-removed-and-improvement-for-no-mx-domains/
And see also CSP test explanation.

@baknu baknu added this to the v1.3 milestone Jul 31, 2020
@gthess
Copy link
Collaborator Author

gthess commented Aug 25, 2020

For the first step decide if unsafe-inline should lead to warning or stay informational.

@gthess gthess self-assigned this Dec 3, 2020
@gthess
Copy link
Collaborator Author

gthess commented Dec 9, 2020

New checks include:

  • Check for unsafe-invalid;
  • Check for unsafe-eval;
  • default-src, frame-src and frame-ancestors need to defined and be 'self' or 'none';
  • http: is not used as a scheme.

If the header does not comply with the above, it is not considered securely configured.
Failure of the test is also updated to a warning instead of informational.

gthess added a commit that referenced this issue Dec 9, 2020
@gthess gthess added the done label Dec 9, 2020
@baknu
Copy link
Contributor

baknu commented Dec 15, 2020

Great that this is implmented. Few questions:

  • Do you mean unsafe-inline instead of unsafe-invalid?
  • Do we also check for a nonce when unsafe-inline is used and do we consider that to be sufficiently secure in line with the DigiD audit norm?
  • Is the following interpretation of what/how we test correct?

  1. Secure (do's):
  • default-src, frame-src and frame-ancestors should be defined and be 'self' or 'none'
  1. Insecure (don'ts):
  • unsafe-inline and unsafe-eval should not used.
  • http: should not be used as a scheme.

Below a citation from the DigiD audit norm:

Content-Security-PolicyDe Content-Security-Policy (CSP) geeft de browser instructies over welke resources vanaf welke locatie
mogen worden ingeladen enhoe deze mogen worden gebruikt. Een CSP kan fijnmazige instructies bevatten per soort resource,
zoals afbeeldingen, stylesheets en scripts. Bij het gebruik van een CSP zijn standaard de uitvoering van inline scripts en de
eval()-functie uitgeschakeld. Te configureren waarden: default-src ‘self’; frame-src ‘self’; frame-ancestors ‘self’; Sta geen onveilige
configuratie toe door het gebruik van 'unsafe-inline' (tenzij gebruik wordt gemaakt van een nonce) en 'unsafe-eval'. Het is niet
toegestaan bronnen beginnend met http:// te whitelisten

Source: https://www.norea.nl/download/?id=7851

@gthess
Copy link
Collaborator Author

gthess commented Dec 16, 2020

  • unsafe-inline indeed, it was a typo.
  • If you want to use inline code you can use nonce or hash sources which have nothing to do with unsafe-inline.
  • Yes.

@baknu
Copy link
Contributor

baknu commented Dec 16, 2020

Thanks.

Regarding your 2nd answer: Ok, clear. The DigiD audit norm states that the following should not be used, which confused me:
NL

'unsafe-inline' (tenzij gebruik wordt gemaakt van een nonce)

EN (translated)

'unsafe-inline' (unless a nonce is used)

Is the following addition correct?

"default-src, frame-src and frame-ancestors should be defined and be 'self' or 'none'"

-->

"default-src, frame-src and frame-ancestors should be defined and exclusively be 'self' or 'none'"

In the CSP spec I read:

To mitigate the risk of cross-site scripting attacks, web developers SHOULD
include directives that regulate sources of script and plugins. They can do so
by including:

  • Both the script-src and object-src directives, or
  • a default-src directive
    In either case, developers SHOULD NOT include either 'unsafe-inline', or data:
    as valid sources in their policies. Both enable XSS attacks by allowing code to be
    included directly in the document itself; they are best avoided completely."

The DigiD audit norm does not 'forbid' data:. However wouldn't it make sense to also consider that as an insecure configuration?

Are there any other CSP settings that you would have expected to be prescribed by the DigiD audit norm?

@gthess
Copy link
Collaborator Author

gthess commented Dec 16, 2020

  1. It is confusing, I believe they mean if you inline code use it under a nonce.
  2. What do you mean exclusively? That only that value is allowed? Then no.
  3. Yes.
  4. I would also add unsafe-hashes. It is part of the newer CSP Level 3 and not supported in all browsers yet.

@baknu
Copy link
Contributor

baknu commented Dec 16, 2020

ad 1. Yep
ad 2. Ok. Can 'none' also be combined with other values?
ad 3. Would it be trivial to also check for that? Or shall we put in the extra recommendations in the test explanation.
ad 4. Idem

  1. Would it also make sense to check for '*' and 127.0.0.1 (local host) as a source?

@gthess
Copy link
Collaborator Author

gthess commented Dec 17, 2020

  1. Technically no. But browsers do not raise an error on that. The existence of 'none' guarantees that the directive will result in an empty set of sources, regardless of what else is specified in the directive. We follow that and also allow 'none' to coexist with other syntactically.
  2. It is trivial.
  3. For wildcard (*) host yes, for localhost why?

On second thought I am not sure about data:. There seems to be some legitimate usage (e.g., embedding images in HTML/CSS). It all boils down on how you use it; as with all the directives in CSP wrt the site content. I believe mentioning the sparing use as a recommendation would be enough.

For the wildcard localhost I would say only for the default-src, frame-src and frame-ancestors that we also check for 'self'. Otherwise a directive of 'self' * for example would not make any sense.

@baknu
Copy link
Contributor

baknu commented Dec 17, 2020

Ad 2: Ok. So we also syntactically check the CSP record. Is that a full syntax check?
Ad 3: Wrt data:: The wording in the spec is pretty strong. I read it as data: should not be used in the script-src, object-src and default-src directive. Would it make sense to only check these directives for data: ?
Ad 4. Wildcard and localhost: yep, let's check for the wildcard in default-src, frame-src and frame-ancestor. Indeed having 127.0.0.1 might be a direct security issue, but it does not seem clean either to have it in a prod environment.

Btw this evaluation tool is interesting to have a look at: https://csp-evaluator.withgoogle.com/

@gthess
Copy link
Collaborator Author

gthess commented Dec 17, 2020

  1. Yes.
  2. Link?
  3. ok

@gthess
Copy link
Collaborator Author

gthess commented Dec 18, 2020

I realize that "2. Yes" above is not clear :)
We know about the CSP syntax but we don't enforce it (as in SPF and DMARC). We ignore things that we don't understand, as browsers do.

@baknu
Copy link
Contributor

baknu commented Dec 18, 2020

Ad 2: Okay
Ad 3: https://www.w3.org/TR/CSP3/#csp-directives
Ad 4a: So we now check for "*" and give a warning in case we detect it, right? I will update content alike.
Ad 4b: Local host (127.0.0.1) is probably more an "informational" item. We could either check for it and present a blue "i" in case we detect it. Or we could just give guidance on this in the text explanation and not explitly test for it. What do you think?

@gthess
Copy link
Collaborator Author

gthess commented Dec 18, 2020

3: It's not very clear to me. I read that as "Define script-src and object-src or default-src. And btw don't include 'unsafe-inline', or data: in your policies.". But I can see how you can also interpret it your way. I can make the change to check those 3 for data:.

4a: Yes, only for those 3 directives. We don't give a warning, we fail the test which happens to be a warning.

4b: Localhost seems like a configuration error or something really specific for some sources. Although it seems logical I don't have enough evidence to suggest it shouldn't be there.

@baknu
Copy link
Contributor

baknu commented Dec 18, 2020

Ad 3: I am not sure yet. MDN says the following:

data: Allows data: URIs to be used as a content source. This is insecure; an attacker
can also inject arbitrary data: URIs. Use this sparingly and definitely not for scripts.

source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/img-src

Is it consistent with the interpretation of the spec to only test script-src and object-src or default-src for data: because only these affect scripts?

Ad 4a: Ok

Ad 4b: Ok

@baknu baknu added the question label Dec 18, 2020
@gthess
Copy link
Collaborator Author

gthess commented Dec 22, 2020

  1. data: can be used anywhere to load binary content. I believe those 3 are sufficient enough. script-src for sure. object-src is still ok as most (if not all) the objects loaded there are either deprecated or will not get any further updates. Some recommendations also mention that it is advisable to restrict object-src to 'none'. default-src also for sure as it will catch any non-defined data: references.

Btw, I am not sure if we want to go an extra step (with data:, or even object-src to 'none') if we want to follow a given CSP guideline. digid? Given that all these will result in only warnings I would say we can go ahead. What do you think?

@baknu
Copy link
Contributor

baknu commented Dec 23, 2020

Yes, let's go for the extra step. We mostly follow the DigiD guideline, but also keep thinking ourselves :-). I plan to give our additions as feedback to DigiD.

So:

  • object-src should be 'none' (or maybe 'self' ?)
  • default-src and script-src should not contain data:

Okay?

Three additional questions:

  1. Where did you find the recommendations on object-src?
  2. Regarding default-src, frame-src and frame-ancestors being self or none: To me it seems that this is mostly effective if these are the only exclusively allowed values. By requiring this for default-src you make sure that policies that need to allow external sources are specified per source type. Otherwise (i.e. if any other external sources could be added to default-src besides self) requiring self for default-src does not seem to have a real benefit. To make it more practical maybe we could/should require 'default-src' for example.nl to be either 'none' or 'self' and optionally https://*.example.nl. What do you think?
  3. Are we now also checking for unsafe-hashes? Do you have a pointer to background info why this is considered to be insecure?

@gthess
Copy link
Collaborator Author

gthess commented Dec 23, 2020

  1. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy

    Elements controlled by object-src are perhaps coincidentally considered legacy HTML elements and are not receiving new standardized features (such as the security attributes sandbox or allow for <iframe>). Therefore it is recommended to restrict this fetch-directive (e.g., explicitly set object-src 'none' if possible).

    Also https://csp-evaluator.withgoogle.com/ suggests to restrict object-srcto 'none'

  2. I think this is too far. If I know that I load resources from the current location and an external location why do I have to define every single directive for the external location instead of putting it on the default?

  3. We don't check for unsafe-hashes. The word "unsafe" in the directive :) Also https://w3c.github.io/webappsec-csp/#unsafe-hashes-usage. I believe attention is not yet drawn to it as it is introduced with CSP3 which is currently a draft.

@baknu
Copy link
Contributor

baknu commented Dec 23, 2020

  1. Ok, clear. Let's check for it. :-)
  2. I get you. But then it does not make sense to check default-src for self. I think the rationale behind the DigiD guideline is that you don't specify a generic allowance policy for a resource but a specific one per source type. A generic one might seem handy, because it just works without having to bother about the source types. But it seems better/safer to have a seperate policy for things like scripts and fonts, especially when you have multiple external source suppliers. I could ask DigiD about their intention.
  3. I thought we also checked for that based on your reply Validate CSP directives #325 (comment). But maybe it is too early since it is not standardised.

Btw I also ran into the following scanner which is quite restrictive: https://cspscanner.com/

@baknu
Copy link
Contributor

baknu commented Dec 23, 2020

Some remarks:

  • Just updated the EN test explanation based on our discussion. Will do the NL explanation later.
  • We should carefully check the rationale for every don't and do in test explanation.
  • Should we also allow default-src to have a subdomain of itself as a source (e.g. www.example.nl for example.nl). That's is what we currently do in our own CSP for Internet.nl.
  • "CSP protects a website against cross-site scripting (XSS) attacks. " is now in the test explanation. However, CSP does more than this.
  • In the future, especially when we change the requirement level into 'required', we can differentiate in the requirement level of the several findings. Maybe some findings will stay a warning (orange !) while others are bad (red X). However we would need different verdicts and a proper way to present the findings and their respective requirement levels (maybe through tech table).

@baknu
Copy link
Contributor

baknu commented Jan 11, 2021

Thanks, I just updated content in EN and NL accordingly.

@baknu
Copy link
Contributor

baknu commented Jan 26, 2021

Feedback that I received: Shouldn't we also check for upgrade-insecure-request as a do?
Check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/upgrade-insecure-requests

@gthess
Copy link
Collaborator Author

gthess commented Jan 26, 2021

Not sure.
Also this quote from the link:

The upgrade-insecure-requests directive is evaluated before block-all-mixed-content and if it is set, the latter is effectively a no-op. It is recommended to set either directive, but not both, unless you want to force HTTPS on older browsers that do not force it after a redirect to HTTP.

I don't think it is a straight forward decision. In that case we would need at least one of them. But for sites that already have only https sources both of those options don't make sense. Having just the scheme https: as default would make more sense in that case.

@baknu
Copy link
Contributor

baknu commented Jan 27, 2021

Here I read that block-all-mixed-content is depcrecated: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/block-all-mixed-content So we can focus on upgrade-insecure-request.

I believe upgrade-insecure-request is some sort of fall back in case no policy or a policy with a http: scheme is configured for a source. However, we require that there's a restrictive default-src which enforces to have a specific policy in place for other sources used. Furthermore, we require that every source uses https: as a scheme. So, with these requirements the fall back of upgrade-insecure-request does not make any difference, right?

@gthess
Copy link
Collaborator Author

gthess commented Feb 2, 2021

Yes, we check that all sources need to go over https. So requiring upgrade-insecure-request would not add anything.
But we should consider allowing upgrade-insecure-request and https: to be present in default-src.

@baknu
Copy link
Contributor

baknu commented Feb 3, 2021

  1. It seems upgrade-insecure-request is itself a directive and not a value that can be use in the default-src. See: https://www.w3.org/TR/upgrade-insecure-requests/ Or am I missing something?
    It seems the upgrade-insecure-request directive not only affects resources that are loaded into the page, but all requests like hyperlinks as well. Furthermore one could argue that having upgrade-insecure-request is some sort of generic fallback for resources when you forget to use https: which makes a policy more robust and less error prone regarding HTTPS. So maybe we should require both,, but I am not sure. What do you think?
  2. I thought we already required https: when a domain is used. So if "the domain itself, or a subdomain or superdomain" (= citation from test explanation) is used in default-src it should be https:

@gthess
Copy link
Collaborator Author

gthess commented Feb 3, 2021

  1. My bad, I was hasty with my response; it is a directive. But my argumentation still holds, we shouldn't require this especially if it affects all links in a page. That would make it impossible to link to external http-only pages. I read that upgrade-insecure-request is intended for legacy apps/sites with a lot of insecure URLs that need rewrittting, thus the directive that you can shove in CSP and not having to touch the legacy application. I wouldn't want to make it a requirement.
  2. We do check for every <host-source> and <scheme-source> to not contain http. I am referring to the https: <scheme-source> as in: default-src 'self' https:;.

@baknu
Copy link
Contributor

baknu commented Feb 3, 2021

  1. In that way https: works as a fallback scheme for resources when there's is no specific directive AND when no default-src domain is configured, But we require default-src to have the value 'none' or 'self', and additionally the domain itself, or a subdomain or superdomain could also be defined. So, although we could allow for it, it seems this https: value for default-src will never have effect for a policy that meets our requirements.

@gthess
Copy link
Collaborator Author

gthess commented Feb 3, 2021

  1. Correct. But we will fail people that use it there and I have seen quite a few suggestions online to put https: in default-src. I get the feeling that we had that discussion before btw.

@baknu
Copy link
Contributor

baknu commented Feb 3, 2021

  1. Ok. Let's allow for it. Can't remember a previous discussion on this but your memory is probably better than mine :-)

@gthess
Copy link
Collaborator Author

gthess commented Feb 5, 2021

OK change already live on dev. With this then I believe CSP is done.

@baknu
Copy link
Contributor

baknu commented Feb 11, 2021

Yes, thanks. I will update content.

@baknu baknu removed the question label Mar 7, 2021
@baknu baknu closed this as completed Mar 7, 2021
@thestinger
Copy link

By the way, form-action and base-uri should be set explicitly since they don't fall back to default-src due to being introduced in v2 as new features.

There's also a new navigate-to as part of the CSP 3 draft but since that's a draft it probably shouldn't be recommended yet as the semantics may change before it's fully standardized.

@sinteur
Copy link
Contributor

sinteur commented Mar 23, 2021

Solid point @thestinger

After a quick discussion about this:

It seems all navigation directives don't fallback. Also other CSP checkers didn't suggest/mention anything about them so it went under the radar.

They could be included in a future release as directives that need to be there because they don't fallback.

I suggest we start rejecting on form-action and base-uri starting 1.5 and advising on navigate-to as soon as it’s not a draft any more.

For base-uri I would guess it is easy to suggest 'self'; in the results.

For form-action maybe 'self'/'none' + any other domain (same as frame-ancestors)?

@thestinger what do you think about the above?

@sinteur
Copy link
Contributor

sinteur commented Mar 23, 2021

(and @thestinger perhaps best to open a new issue for this one?)

@thestinger
Copy link

base-uri can usually be none because the <base> feature usually isn't used. The next recommendation after that would be self which is fine too. It would be pretty odd to need more than that.

@thestinger
Copy link

The reason that certain things don't fall back is because they were added in CSP v2 (base-uri, form-action) and now CSP v3 (navigate-to). They couldn't break compatibility with CSP 1 with default-src set since suddenly the new things would start following that and sites would break. That's why it's a bit of a mess like this.

It's important for form-action since you probably do want to whitelist where forms upload data similar to how you want that for connect-src for JS making connections.

mxsasha added a commit that referenced this issue Mar 28, 2023

Verified

This commit was signed with the committer’s verified signature.
mxsasha Sasha Romijn
mxsasha added a commit that referenced this issue Mar 28, 2023

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants