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

Add support for inline JS inserted via content-for #149

Open
GCheung55 opened this issue Aug 14, 2020 · 7 comments
Open

Add support for inline JS inserted via content-for #149

GCheung55 opened this issue Aug 14, 2020 · 7 comments

Comments

@GCheung55
Copy link

Why?

Addons, such as ember-svg-jar, inserts an inline JS. The inline JS generally violates the CSP script-src directive unless one of the following sources are added to the directive:

  1. Nonce
  2. Hash
  3. unsafe-inline

Nonce

Nonce is added to the <script> tag as an attribute, as well as in the CSP script-src directive, and needs to be unique for each page request.

For SPA's that are not backed by server-side rendering, a unique nonce-value won't be generated each time.

Hash

A hash is generated from the contents of the script tag, including all whitespace and line breaks.

unsafe-inline

This defeats the purpose of the CSP script-src, which would allow any inline JS to load.

Proposal

Generate a hash for every inline JS, and maybe CSS, and add it to the corresponding CSP directive at build time.

From a discussion with @jelhan:

  • Every <script> element (and maybe <style> element) injected into index.html or tests/index.html is allowed in CSP by default
  • ember-cli-content-security-policy addon parses the current content of contentFor hook in its own contenFor hook implementation, extracts all <script> (and maybe <style>) elements, hashes their content and adds the hash to the CSP.
  • It provides a configuration option to restrict this default behavior to some elements (e.g. through a filter function).
@jelhan
Copy link
Collaborator

jelhan commented Aug 15, 2020

Thanks a lot for this great summary.

There is already something similar to the proposed strategy. The addon adds a nonce to a code tag added by Ember CLI for builds that contain tests: https://github.com/rwjblue/ember-cli-content-security-policy/blob/43527edab59a68c8d747fede8cae4573d352baf2/index.js#L278-L285

This prevents that a newly generated ember project violates the default CSP.

The approach proposed here would provide a public API, which covers this case. So it would reduce complexity.

It would have to additional benefits:

  1. We would not alter content pushed by other addons but only read it.
  2. We would not use a static nonce. This caused security concerns even so it's only added to builds, which should never be pushed to production.

@jelhan
Copy link
Collaborator

jelhan commented Aug 15, 2020

It maybe helpful to document why I think assuming that elements added via contentFor at build time are safe always should be okay. Would appreciate if someone reviews these security related assumptions carefully.

Security considerations

The approach discussed above would treat all inline script (and styles) added via contentFor hook as safe. Whatever JavaScript (or styles) are added through this hook at build time will be allowed by the content security policy. An explicit approval of addons which are allowed to inject script (or style) elements or the content which they are adding would not be required.

I think this is acceptable from a security perspective cause of these arguments:

  1. This is build time only. It's very unlikely that an attacker can control anything which is used at build time.
  2. contentFor hook is only one API out of many which allows addons to inject code (or styles) into the build. E.g. the import method allows an addon to inject code directly in the vendor.js (or vendor.css). Both files are treated as safe by defaut.

I think that CSP does not help if you don't trust your build.

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2020

Haven't read things in detail just yet, but leveraging the contentFor hook is a bit problematic in an Embroider world. We need to think about that, and account for it in this design...

@jelhan
Copy link
Collaborator

jelhan commented Aug 21, 2020

Haven't read things in detail just yet, but leveraging the contentFor hook is a bit problematic in an Embroider world. We need to think about that, and account for it in this design...

This proposal is only about supporting addons out of the box, which use contentFor hook. ember-cli-code-coverage is an example of such an addon. It uses contentFor hook to inject a script tag to tests/index.html. ember-svg-jar mentioned above by @GCheung55 is another example. This feature targets addons, which do something like this:

// index.js of some addon

module.exports = {
  contentFor(type) {
    return '<script>window.alert("This code will violate a default CSP")</script>';
  }
}

As far as I know there isn't a similar capability in v2 addon format (Embroider) yet. If one is added later we can explore if a similar zero-config solution for addons using that capability is possible as well. We could maybe even consider that feature when designing such a capability.

@simonihmig
Copy link

@jelhan just pointed me to this issue. Wanted to bring in some context to support this case, but also to include support for inline <style>.

So in our case, ember-a11y-testing injects some inline styles. I could see the CSP violations in the dev tools console, but they did not trigger a failed tests. Probably because the violation event was triggered before this addon has attached its securitypolicyviolation event listener.

However when using ember-cli-fastboot-testing this caused all FastBoot tests to fail, as this addon renders the whole page - including the unsafe inline styles - in FastBoot and then parses that string to insert the DOM into the tests page for running the actual test. At least as of latest Chrome 85 this triggers the securitypolicyviolation, and so made all FastBoot test fail.

We fixed this by manually adding the hash for all inline styles to our CPS policy, but this proposed feature - when including styles support - would have helped here!

@RobbieTheWagner
Copy link
Member

Is there any workaround for this? This seems like a similar issue to #67 and we are hitting this in ember-electron. We use contentFor to insert some scripts, and all of them fail to execute if you use CSP. If we could add a nonce or something, we could work around this, but it is unclear how to proceed.

@jelhan
Copy link
Collaborator

jelhan commented Jan 21, 2021

Is there any workaround for this? This seems like a similar issue to #67 and we are hitting this in ember-electron. We use contentFor to insert some scripts, and all of them fail to execute if you use CSP. If we could add a nonce or something, we could work around this, but it is unclear how to proceed.

There isn't a good work-a-round for addons. 😞

Addons can not modify the CSP of a consuming application. If I didn't missed something they only have two options:

  1. Ask their consumers to add hashes for the script tags to their CSP and update them whenever the content of the scripts change.
  2. Do not inject inline JavaScript.

A nonce can not be used. The security provided by CSS is (partly) broken if an attacker can predict a nonce. The attacker would be able to inject code including the nonce. Please see the § 7.1. Nonce Reuse from CSP Level 3 specification for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants