fix CSP by injecting styles using CSSOM

[jelhan]

This uses CSS Object Model (CSSOM) to modify stylesheets.
Changing stylesheets via CSSOM does not violate Content-Security-Policy style-src 'none'.

CSSOM is still a working draft but the features been used should be supported by all target browsers:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/styleSheets#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule#Browser_compatibility

Creating an empty style element does not violate CSP.

-webkit- prefix is not needed anymore for keyframe.
Inserting CSS rule @-webkit-keyframes throws a SyntaxError in IE11.

Done basic manual testing using samples in recent versions of:

• Chrome (Desktop & Mobile)
• Firefox
• Microsoft Edge
• IE 11

Fixes #5208 together with #5909

Live example: https://codepen.io/jelhan/pen/jXYymO

Please note the CSP meta tag defined in settings. You need to update SHA hashes if you change any JavaScript in this Codepen as it violates CSP otherwise.

[benmccann]

@jelhan can you share more details about the meta tag that you mentioned? I didn't see that in the fiddle though I may have missed it. Is there anything we should document for our users about needing to set a sha hash?

[jelhan]

can you share more details about the meta tag that you mentioned? I didn't see that in the fiddle though I may have missed it.

This is setting a Content-Security-Policy (CSP) using meta tag in Codepen. This demonstrate that the changes in this PR allow Chart.js to be used with the strictest possible CSP default-src 'none'.

There are some SHA hashes but only to make Codepen working together with CSP. The two hashes provided as script-src are for the script tags injected by Codepen. One is present explicitly in HTML tab and containing the Chart.js build. The other one is for the content of JavaScript tab, which is also injected as script tag by Codepen. The two hashes provided as style-src are for the style tags generated by Codepen. You will notice them with IDs #cm-font-family and #cm-font-size in DOM.

Is there anything we should document for our users about needing to set a sha hash?

This PR is actually about not requiring any SHA hashes to pass a strict Content-Security-Policy. f90ee8c injects styles into document not using CSSOM. This requires either style-src 'unsafe-inline' or a SHA hash over the styles added. That's what #5208 and the errors mentioned there is about.

I had tried another approach in #5946 to avoid the usage of experimental CSSOM feature but that one failed. After some investigation I think injecting styles in a CSP complaint way is only possible through CSSOM. Specification may still be in Working Draft status but the features used in this PR seems to be stable and are supported by all major browsers.

[simonbrunel]

-webkit- prefix is not needed anymore for keyframe (and animation)

Can you link the article / post that confirms this point? I don't know if caniuse refers to keyframe or animation or both.

I'm a bit worry about relying on an experimental / draft feature, even if well supported. Also, with this approach, we can't inject prefixed styles to target all browsers (e.g. -webkit-) because rules must be valid for all browsers. This is fine now because apparently we don't need prefix for keyframe and animation, but could be an issue later ... maybe.

That being said, I'm fine to merge this PR with requested changes and figure out later how to handle more styles if we, by any bad luck, encounter any issue with this method.

[jelhan]

Can you link the article / post that confirms this point? I don't know if caniuse refers to keyframe or animation or both.

Test would have failed if it's still required for Chrome isn't it?

Please have a look in Chromium bug report tracking the removing of prefix: https://bugs.chromium.org/p/chromium/issues/detail?id=154771#c30 enabled CSS animations without prefix by default and https://bugs.chromium.org/p/chromium/issues/detail?id=154771#c33 removed the configuration option to disable unprefixed CSS animations.

Also, with this approach, we can't inject prefixed styles to target all browsers (e.g. -webkit-) because rules must be valid for all browsers.

Only IE11 throws on insert rule @-webkit-keyframes. Firefox and Microsoft Edge passed. Didn't tested Safari.

[kurkle]

Changing stylesheets via CSSOM does not violate Content-Security-Policy style-src 'none'

I'm worried that this will not hold. Whats wrong with nonce? That should be stable approach.

[jelhan]

I'm worried that this will not hold. Whats wrong with nonce? That should be stable approach.

Wouldn't that mean to generate a nonce on every page load, injecting that as configuration into the page so that it could be read by Chart.js and including it in CSP header? That sounds far more complicate that current requirement of adding a SHA hash for injected styles.

[simonbrunel]

Open the page with the Content-Security-Policy: style-src 'self' directive set

I'm not familiar enough with CSP, I may have done things wrong but I tried to reproduce the case described in #5208 and this PR doesn't seem to fix it when style-src 'self', but neither with style-src 'none':

<meta http-equiv="Content-Security-Policy" content="style-src 'none'">
<script src="http://10.0.75.1:8080/dist/Chart.bundle.js"></script>

Your pen embeds Chart.js code directly in a <script> tag but I don't think it's the same as linking an external source.

[jelhan]

@simonbrunel Arghs you are right. Didn't noticed that one. Since there isn't any API yet for constructing CSSStyleSheet through CSSOM this limits our options to hijacking an existing stylesheet and inserting the rules in it. Code would look like:

function injectCSS(platform, rules) {
	var sheet = document.styleSheets[0];
	rules.forEach(function(rule) {
		sheet.insertRule(rule, style.sheet.cssRules.length);
	});
});

This would add the edge case of documents not having any stylesheet but that one shouldn't be a real world issue.

I guess there is no other option for #4591 than inserting these styles globally?

[simonbrunel]

This would add the edge case of documents not having any stylesheet ...

... which still needs to be supported

I guess there is no other option for #4591 than inserting these styles globally?

I don't think there is another method that would work on all supported browsers (including IE10).

In #5208, some suggested to:

Add a nonce attribute and make it possible to set the nonce. For the head element, we need to set an attribute (either nonce or integrity on the programatically created style element (either could be passed in as a config option to the constructor).

What does that mean exactly, how can we support this method?

[jelhan]

Add a nonce attribute and make it possible to set the nonce. For the head element, we need to set an attribute (either nonce or integrity on the programatically created style element (either could be passed in as a config option to the constructor).

What does that mean exactly, how can we support this method?

A nonce is a cryptographically strong random values that could be used to whitelist <style> and <script> tags. It's meant to be used by server-side template engines that generate dynamic style or script elements. It must be included in CSP policy. A nonce must be unpredictable and generated for every page load.

<html>
  <head>
    <meta http-equiv="Content-Security-Policy" content="style-src 'nonce-2726c7f26c''">
    <style nonce="2726c7f26c">
      // some styles
    </style>
  </head>
</html>

A good explination is given in this question on StackOverflow: https://stackoverflow.com/questions/42922784/what-s-the-purpose-of-the-html-nonce-attribute-for-script-and-style-elements

Generating a cryptographically strong random value on each page load puts a hard burden on infrastructure. It's a show-stopper for my use case and I guess for a lot other use cases.

Since the content of <style> element created by Chart.js is static, whitelisting it through a SHA hash would be a better option IMO. But it would still require all users of this library to either relax there CSP (style-src 'unsafe-inline') or to add that SHA to it.

I guess there is no other option for #4591 than inserting these styles globally?

I don't think there is another method that would work on all supported browsers (including IE10).

Is it only about IE10? Could consider to drop support cause it's not even support by Microsoft anymore.

This would add the edge case of documents not having any stylesheet ...

... which still needs to be supported

Could be more relaxed on that edge case cause it's a very rare case. E.g. we could create a style tag (and therefore violate CSP) only in that case. Or we could throw that at least one stylesheet needs to be present. It's not a perfect solution but a better one than violating CSP in all cases.

[simonbrunel]

Is it only about IE10?

I think IE11 is also concerned but it took us a huge effort (and tons of time) to get this method stabilized that I'm not down to experiment other approaches that may break many use cases. Unfortunately, we don't have all the unit tests needed to guarantee that we don't break any integration if we change that method. So to me, that's not an option to make CSP happy.

Injecting our style in a foreign element sounds a bit like a hack and may not work as expected in all cases (we will also have to support Shadow DOM at some point). I don't have enough knowledge about CSP to judge what's the best approach, would be great to have feedback from users from #5208 (@fxOne, @ETNyx, @jrberlin, @NicoHaase, @vletoux, @smariel, @hxoht, @schalkventer, @panuhorsmalahti)

@jelhan can you remove any ticket #number from your commit message because it pollutes the original ticket. Also, don't squash (and force-push) your changes (we will do it on merge), it will be easier for us to follow your changes and track the history of our iterations.

[benmccann]

IE10 is not a supported browser. Only IE11+ is supported: #5788

[simonbrunel]

Chart.js works fine on IE10 but, you right, MS drop support for this browser (January 12, 2016) so we may evaluate MutationObserver to replace that keyframe/animation stuff, which seems supported by IE11 but we will certainly break some of our users integrations.

[vletoux]

I'll be happy to test it if someone points me out to a compiled release (i'm not a .js guy)

[simonbrunel]

@vletoux Thanks, unfortunately this PR doesn't fully fix the CSP issue.

[simonbrunel]

I pushed a branch with mutation observer (PR #5955) instead of CSS animation (I still want to perform additional tests before submitting a PR). Would be great to have a few people testing this branch on different browsers / projects and check that it doesn't break responsiveness.

@jelhan can you please verify that it fixes all CSP issues?

[vletoux]

I'm not a .js guy but:
Aren't there a fixed list of style to add to fix the CSP problem ?
(I mean that the style is not constructed with dynamic values ...)
Then if it is the case, why trying to solve this problem with dynamic style insertion ?

A proposal: you may then use an external .css file and if needed, enable a fallback to add the style manually as today.
So instead of modifying with javascript the "style" attribute, with mystyle: myvalue, you modify the "class" attribute with chartjs_mystyle_myvalue.
This is how I fixed my own style CSP problem.

[panuhorsmalahti]

Yes, adding a css file would be one way to fix it (that's what I did). The downside is that it would be a major update (=breaking change), and would require some extra work to start using the css file.

[sebiniemann]

Whether or not this is a breaking change depends on how you introduce it.

It could also be a feature release, by adding a new option that disables the current CSS injection and requires to manually link the css file.

This way, the default behaviour wouldn't change, while security concerned users (like us) can already resolve this CSP issue by enabling the new option. In addition, the old way could already be marked as deprecated to easea later breaking change, when its completely removed.

[jelhan]

It could also be a feature release, by adding a new option that disables the current CSS injection and requires to manually link the css file.

I've also thought about another option that would not even require a separate CSS file but also be backward compatible: We could add a config option that controls which stylesheet should be used to inject styles. By default a new stylesheet is created by inserting a <style> tag (current behavior). But if user passes a selector that matches an existing stylesheet, this one is used. We would keep the ability to dynamically insert rules and wouldn't have to care about a separate CSS file and all the questions that might come up with that one (stylelint, minification etc.). As soon as there is a public API to create stylesheets in a CSP complaint way and all target browsers support it, that config option could be dropped again.

[sebiniemann]

I am not really sure I completely got what you mean. But maybe I also misunderstood the reason for the stylesheet that is currently included via <style>.

But if user passes a selector that matches an existing stylesheet, this one is used.

Where would this stylesheet come from? I assume that it is still located within a file, that is shipped together with Chart.js (cause at least some functionality of Chart.js depends on it)?

---

Some thoughts on a complete other leaf (more regarding the CSSOM solution):

I am not sure how a public, CSP-compatible API should work, without compromising the security the current CSP rules provide. Because by disabling unsafe-inline, any CSS information has to be located within a file, which cannot be manipulated anymore after it is served by the web server. Which also renders malicious manipulations futile.

By opening this up with an API, one would have to compromise as far as I can see it. While this may be possible for some users, it excludes the more security focused folks. And I think to uphold security was the main reason, to add some CSP rules in the first place.

In summary, as long as Chart.js is not depending on such a dynamic solution (what I cannot see at this point, but maybe I just misunderstood the reason for this stylesheet -- so please bear with me 😃 ) I would prefer a more straight forward solution, as provided by @panuhorsmalahti in #5208 (comment).

[jelhan]

But if user passes a selector that matches an existing stylesheet, this one is used.
Where would this stylesheet come from? I assume that it is still located within a file, that is shipped together with Chart.js (cause at least some functionality of Chart.js depends on it)?

Stylesheet refers to a CSSStyleSheet. This may be created by a <style></style> or a <link href="style.css" rel="stylesheet">.

The code would look like: document.querySelector(config.stylesheetSelector).sheet.insertRule(cssRule);.

In most cases it should be safe if Chart.js insert it's styles in an existing stylesheet. In that case a developer may simple set the selector to link[rel="stylesheet"] and Chart.js will use the first element that matches these selector to insert the rules. In other cases a developer might configure the webserver to inject <style nonce="randomstring" data-stylesheet-for="chart.js"></style> into served HTML and provide style[data-stylesheet-for="chart.js"] as stylesheet selector configuration to Chart.js.

I am not sure how a public, CSP-compatible API should work, without compromising the security the current CSP rules provide. Because by disabling unsafe-inline, any CSS information has to be located within a file, which cannot be manipulated anymore after it is served by the web server. Which also renders malicious manipulations futile.

You could manipulate styles in JavaScript without violating CSP. You could insert additional rules in an existing stylesheet using CSSStyleSheet.insertRule(). You could modify the styles of an Element using CSSOM, e.g. document.createElement('p').style.display = 'none';. Only limitation is creating a stylesheet. This is not possible without violating CSP yet.

This is not adding additional risks cause it could only be executed by JavaScript that is not violating CSP. Of course this doesn't help if you allow script-src: 'unsafe-eval'' but if an attacker can inject JavaScript you don't have to care about styles anymore.

[sebiniemann]

True -- and unfortunately -- CSP is not yet there to completely fulfil its role, wherefore many ways to manipulate things are still open. However, I would not say that these manipulations are within the spirit of CSP and always there in the future.

Regarding the CSSOM manipulations, it is already listed under https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src#Unsafe_style_expressions, that most of these manipulations (like the insertRule() function used in this MR) require style-src: 'unsafe-eval' -- or are otherwise blocked. I also believe this was already discussed a few years ago on the W3C mailing list and is therefore an already established rule (at least in the sense of the specification).

---

Because of this, I am unsure how and if this could be any solution to the innermost problem. I think the core challenge is that the most secure a website can be is to be first fully static and only dynamic where it absolutely needs to be, since any kind of DOM/CSSOM manipulation could be malicious.

Using just another way to manipulate something may be a workaround, but does not sounds like a solution on the long run (purely my opinion 😉 ). I was therefore questioning myself if this CSS information only has to be dynamically inserted, in order to support the current implementation, but could actually be delivered statically (as mentioned by @panuhorsmalahti).

Going this way, the preferable solution seems to do it statically and only think of an easy implementation (so we do not focus to much attention on something that will be removed later on) to still support the current way, while painlessly moving to a static file inclusion.

For me, this could be a simple boolean switch, controlling whether the CSS information should be dynamically included or not.

[jelhan]

Regarding the CSSOM manipulations, it is already listed under https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src#Unsafe_style_expressions, that most of these manipulations (like the insertRule() function used in this MR) require style-src: 'unsafe-eval' -- or are otherwise blocked. I also believe this was already discussed a few years ago on the W3C mailing list and is therefore an already established rule (at least in the sense of the specification).

Manipulation of elements style using CSSOM is discussed here in detail: w3c/webappsec-csp#212 I think it's a question of what is considered as parsing in sense of CSP3 specification.

CSSStyleSheet.insertRule() is another case. Insert a CSS rule is listed as gated on the unsafe-eval source expression explicitly. But that's not how it's implemeted in major browsers at least to my findings. I think spec has only taken the cases of inserting a CSS rule via <style> of <link> element into account and not via CSSStyleSheet.insertRule(). I wasn't able to find any discussion about that one in the repo. But Chrome, Firefox and Edge don't block it as far as I have investigated.

To verify yourself, I've put a basic example together

// index.html
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Test CSP</title>
    <meta http-equiv="Content-Security-Policy" content="default-src 'self'">    
    <link rel="stylesheet" href="style.css">
    <script src="script.js"></script>
  </head>
  <body>
    <p>You will see white text on green background if Content-Security-Policy is not blocking `CSSStyleSheet.insertRule()` nor modifing element styles via CSSOM.
  </body>
</html>

// script.js
window.addEventListener("DOMContentLoaded", function() {
  document.querySelector('p').style.color = 'white';
  document.styleSheets[0].insertRule('body { background-color: green; }');

  // violate CSP to verify that it's working
  document.addEventListener("securitypolicyviolation", function() {
    alert('CSP is working');
  });
  document.createElement('p').setAttribute('style', 'color: white');
});


// style.src
// could be empty but must exist

I've uploaded the files here: https://jhanschke.de/test-csp/ Please report if you find any browser in which this isn't working. I've confirmed myself with recent versions of Chrome, Firefox and Microsoft Edge.

[sebiniemann]

@jelhan Just for the case, I hope I did not offended you in any way, as such discussion can easily become quite heated 😅

Only because some security option is not yet supported in major browsers, does not means the current solution is secure. Simply switching one way to dynamically add CSS (which is already marked as insecure) with another way (which is just not yet reported/blocked) is only hiding the underlying problem. I also don't see any option that makes this API more secure than the other one, nor that a secure CSS manipulation was the main focus by the authors of this API (CSSOM). From what I get, the main focus is to ease CSS manipulations via JS.

What I think we can agree on is the fact, that being able to manipulate code/data is always less secure compared to an immutable implementation. While I understand your point that the current situation is far from providing a fully immutable way to do things, I do not see the need to pursue a path that is neither more secure, nor even necessary in the first case, as Chart.js does not require to dynamically insert/change CSS.

So while the static solution is not fully immutable, it still already closes insecure ways to manipulate CSS and also fully resolves this issue.

---

If you are interested, I found mentions of the proposal for the style-src: 'unsafe-eval' requirement here: https://lists.w3.org/Archives/Public/public-webappsec/2013Jul/0006.html

Please be gracious with me that I do not put more effort into my Google search to find a better source for this just now, as I do not believe it will add something substantially to the actual discussion. I can only point out that a W3C security group already raised concerns about this and I also see in your mentioned discussion (w3c/webappsec-csp#212) many concerns regarding CSSOM and CSP.