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

Policy to allow only custom properties in inline CSS #174

Open
J0WI opened this issue Jan 14, 2017 · 13 comments
Open

Policy to allow only custom properties in inline CSS #174

J0WI opened this issue Jan 14, 2017 · 13 comments
Milestone

Comments

@J0WI
Copy link

J0WI commented Jan 14, 2017

Introduction

A common challenge in a CMS to implement CSP is user defined styling.
Popular CMS allows the user to choose from a list of preserved templates and customize some basic colors or background images. Those custom values are usually served as inline CSS like the following example:

<style>
/* generated from user preferences */
.main .article {
	background-color: #00FF00;
	background-image: url("/media/uploads/background.png");
	color: 000000;
}
</style>

With the current draft I only see a few possibilities to make this working with CPS:

  • Generally allow 'unsafe-inline'
  • Hash the generated styles from the server and inject a nonce/hash policy into the CSP directive
  • Load user defined styles from a server generated file
  • Restrict the choice of the user to list of predefined classes

None of them seems to be a nice practice.

Goal

A nice solution would be to only allow custom properties as inline CSS. This allows to read user defined styles without the risk of malicious CSS injection:

<style>
/* generated from user preferences */
:root {
	--user-article-background-color: #00FF00;
	--user-body-background-image: url("/media/uploads/background.png");
	--user-text-color: black;
}
.header {
	background-image: url("/media/uploads/forbidden.png");  /* blocked, no custom property */
}
</style>

Proposal

I propose to call this policy 'unsafe-variables' or 'inline-variables', but I'm sure that they are better names for it. So a CSP header for this would look like:

Content-Security-Policy: style-src 'self' 'unsafe-variables';

(Note that in this case the use of the variables would be defined in a static CSS stylesheet on 'self')

Further thoughts

For additional security, the policy could restrict the custom properties to a specific prefix (like --user) or selector (like :root). This would allow to avoid overwriting properties used by the CMS itself:

<style>
/* generated from user preferences */
:root {
	--user-text-color: black; /* allowed */
	--system-link-before: "evil content"; /* blocked, prefix not allowed */
}
input {
	--user-text-color: #333; /* blocked, selector not allowed */
}
</style>

Inline variables in JS

This policy may be also used for script-src, but of course this is much more risky. So at least a restriction to a specified prefix should be required if this policy will be also considered for JS.

Risk

Using this policy would not be completely without any risk, especially together with 'unsafe-eval', but it could help to decrease the usage of 'unsafe-inline' for CSS.

@mozfreddyb
Copy link
Contributor

The use case you describe fits well to nonces, but they are only supported for scripts.

@mikewest
Copy link
Member

  1. @J0WI, thank you for the suggestion! I'm not terribly excited about subsetting CSS in the way you're suggesting, as it adds a reasonable amount of complexity to the CSS engine for benefits that aren't really clear to me. You note that hashes and nonces exist, but exclude them as not "nice practice". At an initial pass, I'd agree with Freddy that allowing specific inline style blocks seems like a good fit for hashes and nonces. Can you tell me more about why you rejected that approach?

  2. Nonces work just fine for <link> and <style>. The algorithm you're pointing to, @mozfreddyb, has special rules to prevent dangling markup attacks from reusing a nonce by injecting an incomplete <script src='https://evil.com/evil.js' before an expected script tag.

@mozfreddyb
Copy link
Contributor

Ugh, Mondays. Thanks @mikewest.

@J0WI
Copy link
Author

J0WI commented Jan 16, 2017

Thanks for the feedback.

I'm not terribly excited about subsetting CSS in the way you're suggesting, as it adds a reasonable amount of complexity to the CSS engine for benefits that aren't really clear to me.

Yes, I had that point too. But since we already have CSS Variables, hashes/nonces/sri and CSP in <meta> tags all working together in a complex way, I thought it's not that big deal anymore.
I hope that the implementation can be simplified with strict guidelines in the spec.

You note that hashes and nonces exist, but exclude them as not "nice practice". At an initial pass, I'd agree with Freddy that allowing specific inline style blocks seems like a good fit for hashes and nonces. Can you tell me more about why you rejected that approach?

I think that the way this would be implemented is not a nice practice. It would result in a server side function, that just read (and check) the user input and generate the nonce for it. So the nonce will always match the code snipped. This does not really add additional security and if the check fails (or the input is not checked at all) this results in the same problem as we have with 'unsafe-inline'.
Hashes/nonces and SRI are only secure when they are not dynamically generated.

With CSS Variables a user can only change the values of predefined properties, so this is much more restrictive policy and therefore preferable.

@mozfreddyb
Copy link
Contributor

I think that the way this would be implemented is not a nice practice. It would result in a server side function, that just read (and check) the user input and generate the nonce for it. So the nonce will always match the code snipped. This does not really add additional security and if the check fails (or the input is not checked at all) this results in the same problem as we have with 'unsafe-inline'.
Hashes/nonces and SRI are only secure when they are not dynamically generated.

I can not agree with this. If you supply a hash or a nonce for a piece of code (script or style), you're saying that it is not evil.
If your application wants to supply user input inline, it should sanitize and whitelist appropriately. That is something you would need to do for browsers that don't support CSP anyway.

@J0WI
Copy link
Author

J0WI commented Feb 11, 2017

If we could trust all sanitize functions, we would have no issues with code injection at all.

@mikewest mikewest added this to the Future milestone May 10, 2017
@mikewest
Copy link
Member

I don't intend to address this in CSP3. I'm not sure it's a good idea in general, but I'll leave it open as a future feature request.

@dav-is
Copy link

dav-is commented Jan 6, 2019

@mikewest A better usecase is when using CSS variables to effectively whitelist dynamic CSS properties. It would allow JS to modify styles dynamically within reason. It wouldn't need a <style> tag, these would be directly applied to elements. There's so many use cases for this. Most sites today have to use unsafe-inline because of this limitation. I'd advocate for using inline-variables instead of calling it unsafe.

Also worth noting that this type of behavior can't be achieved with nonces or hashes.

Here's a decent example: https://css-tricks.com/updating-a-css-variable-with-javascript/

And: https://eager.io/blog/communicating-between-javascript-and-css-with-css-variables/

@dveditz
Copy link
Member

dveditz commented Jan 9, 2019

The scripted setting examples in those articles work for me fine, either creating styles from nothing or altering variables in an inline block that has a nonce. @dav-is do you have a specific example of where this isn't working for you with a CSP?

@dav-is
Copy link

dav-is commented Jan 9, 2019

@dveditz You're right. I guess the initial styles need to be defined in the stylesheet, but changes can be applied to <element style="--var: newValue"> through JavaScript.

@nico3333fr
Copy link

Same comment: manipulating CSS variables from JavaScript leads to use unsafe-inline for CSS. And when you need strong CSP policies, this is de facto a goodbye to manipulate them by JS. :-\
(just had the case today at work)

A lot of CSS tutos - even if I think this is a bad idea in a lot of cases - are doing this.

An intermediary solution as inline-variables as proposed by @dav-is could be a bridge between JS and CSS without sacrifying the entire style protection from CSP.

@ProgramMax
Copy link

ProgramMax commented May 2, 2021

I ran into a similar situation. I am using a nonce for my style and script tags, which I believe to be correct. If a user is able to inject into my page (but not my style/script), things are protected. Indeed, CSS variables make the situation tricky. A blanket inline-variables seems easy enough. Although, I think it would be good to allow nonce on all styleable tags.

.Thing { background: var(--theme-color); }
// myElement.setAttribute('style', '--theme-color: black'); // Note: This parses the attribute which triggers CSP and will be blocked
myElement.style = "--theme-color: black;" // This parses the property, not the attribute. CSP is not invoked. It is not blocked.

That JS block / workaround is awkward. After all, it is coming from trusted, nonced content.
But there is a deeper problem: Server side rendering.

<div class="Thing" style="--theme-color: black"></div>

This hopes to accomplish the same thing as my JS above, only it was done on the server while the page was being generated. But this will be blocked. And fair enough. How do we distinguish between a malicious user's injected styling and a trusted server side rendering??

Well, we already do that with nonce.

What if I could:

<div class="Thing" nonce="magic_nonce_value" style="--theme-color: black"></div>

This opens a can of worms with nonce hijacking, though. Someone could craft an unclosed tag to gain the nonce and duplicate the style attribute:

<div class="Malicious" style="malicious style"
<div class="Thing" nonce="magic_nonce_value" style="--theme-color:black"></div>

It would extend the Is element nonceable? to include every tag type. And in multiple ways:

  • If previously, good web devs only supplied nonces to their script and style tags, then we only needed to check for "<script" and "<style" attributes to deem something non-nonceable. Now we would need to check against every tag opening. Beware that we can now create custom tags. So any spec wording considering this would need to call that out.
  • Right now, only a script tag looks for hijacking. (Why doesn't style, btw? Or a link tag?) If any tag can be nonceable, they all need to look for hijacking. (And again, with custom tags this is gets messier.)

Preventing nonce hijacking and allowing nonce on any tag is the solution I prefer. Especially if the nonce can cascade to children. That gives me control over areas I have deemed safe from injection.

@ProgramMax
Copy link

Possibly half-answering my own question: The style tag can only appear in the head. And likely a user's injection will be in the body, not the head. However, a link tag to a stylesheet can appear in the body (it is body-ok: https://html.spec.whatwg.org/multipage/links.html#body-ok ).

There is perhaps a related conversation going on in #212 .

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

No branches or pull requests

7 participants