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

Further granularity of unsafe-inline styles #45

Open
jonathanKingston opened this issue Dec 3, 2015 · 14 comments
Open

Further granularity of unsafe-inline styles #45

jonathanKingston opened this issue Dec 3, 2015 · 14 comments
Labels
Milestone

Comments

@jonathanKingston
Copy link
Contributor

@jonathanKingston jonathanKingston commented Dec 3, 2015

Could we consider decoupling <style> and style="..." usage in style-src 'unsafe-inline' CSP setup?

The rationale is that as far as I am aware style="..." has no modern security issues in CSP supporting browsers; where as given selector support <style> could be manipulated to check for data on the page and load images.

For example there are various teams wanting to implement CSS parsers to get around the style="..." support issue, where as <style> seems mostly easy to move to a secure setup. So for example Ember team is willing to take the property in and set it as JavaScript attributes after parsing to mitigate the warning.

There could also be room to discuss if granularity for SVG <style> tags could be separate too I guess?

@mozfreddyb mentioned that he would like to see scoped styles granularity also which probably makes sense too.

/cc @mikewest

@mikewest mikewest added the CORE label Dec 6, 2015
@jonathanKingston

This comment has been minimized.

Copy link
Contributor Author

@jonathanKingston jonathanKingston commented Apr 7, 2016

For example @marumari's new site pokeinthe.io she was required to use default-src 'none'; frame-ancestors 'none'; style-src 'self' 'unsafe-inline' on images like:

https://pokeinthe.io/theme/images/icons/linkedin.svg

Which uses these:

style="fill-rule:evenodd;clip-rule:evenodd;stroke-linejoin:round;stroke-miterlimit:1.41421;"

This sort of thing is common place in an SVG however all other browsers besides Firefox don't block this when loading an external document.

@april

This comment has been minimized.

Copy link
Contributor

@april april commented Apr 7, 2016

I will say that it's near impossible to find any site of a significant size that doesn't have style="..." at least somewhere in their codebase. Even on my lousy personal website where I went well out of my way to not use any style attributes (which is actually annoyingly painful), I still got caught flat-footed because my content creation tools (Affinity Designer) use style attributes in their SVG exports.

In my scanning of the top 100000 or so sites, of the sites that use CSP, very few of them don't have 'unsafe-inline' inside style-src. GitHub, Twitter, Facebook, AMO (addons.mozilla.org, which I personally helped with) — they all have 'unsafe-inline' inside style-src.

The fact that style attributes and style tags so are tightly coupled means that a lot of sites have actually made themselves vulnerable to cross site styling attacks completely unnecessarily, out of a need to support style attributes. If we're going to call it 'unsafe-inline', then it should actually be, you know, unsafe.

My messed up nginx config, just to make SVGs work:

add_header Content-Security-Policy "default-src 'none'; child-src https://air.mozilla.org; font-src 'self' https://fonts.gstatic.com; frame-ancestors 'none'; frame-src https://air.mozilla.org; img-src 'self'; media-src 'self'; script-src 'self'; style-src 'self' https://fonts.googleapis.com";

location ~ \.svg$ {
    add_header Content-Security-Policy "default-src 'none'; frame-ancestors 'none'; style-src 'self' 'unsafe-inline'";
}
@michaelficarra

This comment has been minimized.

Copy link
Contributor

@michaelficarra michaelficarra commented Apr 11, 2016

@marumari

then it should actually be, you know, unsafe

It is unsafe. An adversary who has control over a style attribute may manipulate the styling to mislead a user into taking actions they don't want to take or believing something that is not true.

I would support separating 'unsafe-inline' and 'unsafe-inline-attr' or something, but I would not support simply allowing style attributes by default or naming the keyword anything that doesn't include unsafe.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented May 9, 2017

@arturjanc, et al: Anyone interested in squeezing a proposal into CSP3?

@jonathanKingston

This comment has been minimized.

Copy link
Contributor Author

@jonathanKingston jonathanKingston commented Feb 21, 2018

Repinging here due to the panic around: https://news.ycombinator.com/item?id=16422696

I also want to highlight we could probably do something about the font range attack in CSS too (restricting unicode-range to bigger ranges), however @annevk thinks that might become a wackamole exercise.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Feb 21, 2018

@jonathanKingston: It's not clear what that PoC has to do with inline styles. It relies on attribute selectors, which you can't put into style attributes. I think there's probably something we could do to mitigate that style of attack (either by banning attribute selectors entirely or in part on input field values, opting-in via something like writeonly, or something else I haven't thought of), but it's not clear what CSP should be doing differently. We give folks the ability to block CSS from untrusted sources. What more would you like to see?

@jonathanKingston

This comment has been minimized.

Copy link
Contributor Author

@jonathanKingston jonathanKingston commented Feb 21, 2018

@mikewest the proposal here is regarding the attack vector of <style> whilst style= is much more common and less of an attack vector.

If we added a new directive (or two) unsafe-inline-block and unsafe-inline-attr then pages could permit either/or and not be vulnerable to the other.

My issue here isn't with attributes and perhaps this thread has taken a turn in this direction. The original topic here and the attack vector described in the Hacker News thread is regarding <style> which has a much broader set of issues than the attribute.

@arturjanc

This comment has been minimized.

Copy link

@arturjanc arturjanc commented Feb 21, 2018

I'm not sure why I never commented here (sorry!), but I like this approach, as evidenced in #202 which is duped to this issue.

+1 to what @jonathanKingston is saying: it would be valuable for developers to allow style= attributes while enforcing restrictions on stylesheets (i.e. <style> blocks or external CSS loaded via link#rel=stylesheet). In particular, this would be easy to deploy in applications using nonce-based policies because it would only require adding nonces to <style> / <link> elements without making any other application changes.

The main problem here is, as usual, backwards-compatibility with CSP2. If we add 'unsafe-inline-attr' as a new style-src keyword, the application would need to set a policy of:

style-src 'unsafe-inline' 'unsafe-inline-attr' 'nonce-foo'

The problem is that in CSP2 browsers, the nonce will cause the browser to ignore 'unsafe-inline', and style attributes will be blocked. One way to tackle this is similar to what we've considered for 'unsafe-hashed-attributes' and introduce a new spelling for nonces/hashes, e.g.

style-src 'unsafe-inline' 'unsafe-inline-attr' 'csp3-nonce-foo'

This would work properly in CSP<=2 browsers (only 'unsafe-inline' will be recognized), and in CSP3 browsers (the browser will enforce the nonce on style blocks, but will permit the use of attributes).

@jonathanKingston

This comment has been minimized.

Copy link
Contributor Author

@jonathanKingston jonathanKingston commented Feb 21, 2018

A different name might assist the issue @arturjanc speaks of. Perhaps block-style-tags which would be a noop if unsafe-inline wasn't present.
However this would start a precedent of naming which is just as bad (ignoring upgrade-insecure).

@vrastogi

This comment has been minimized.

Copy link

@vrastogi vrastogi commented Feb 21, 2018

I would also like to second work on this issue. Many of the legacy websites we are working with have hundreds of style attributes. They are often prepared with automatic frameworks and it is going to be very difficult to make these websites CSP compatible.

The new spelling proposal of @arturjanc looks good.

@arturjanc

This comment has been minimized.

Copy link

@arturjanc arturjanc commented Feb 26, 2018

As another data point, this proposal would also mitigate (at least partly) issues such as https://gist.github.com/securityMB/d9e84bd3c7c245895360808360b9dc4e because inline style attributes do not allow an attacker to style other elements in a way that allows for CSS-only exfiltration. JFYI.

@april april mentioned this issue Mar 14, 2018
32 of 32 tasks complete
@april

This comment has been minimized.

Copy link
Contributor

@april april commented Mar 14, 2018

Just adding this for reference, but a couple weeks ago I expanded on the CSS keylogger attack to create a much more reliable attack. I also stood up a permanent demo site here:

https://no-csp-css-keylogger.badsite.io/

@andypaicu

This comment has been minimized.

Copy link
Collaborator

@andypaicu andypaicu commented Apr 5, 2018

I have sent an e-mail on public-webappsec but in case someone follows this thread more closely, I will also leave a comment here.

I have put together an explainer for this and I would like to hear your thoughts.

https://docs.google.com/document/d/1_nYS4gWYO2Oh8rYDyPglXIKNsgCRVhmjHqWlTAHst7c/edit?usp=sharing

@simevidas

This comment has been minimized.

Copy link

@simevidas simevidas commented Sep 2, 2019

For what it’s worth, on a website with a default-src 'none'; style-src 'self' CSP, inline styles in the style attribute are blocked in Firefox but are allowed in Chrome, Safari, and Firefox Nightly.

(I just discovered this browser inconsistency on my website and wanted to state it here, for the record.)

@thatbudakguy thatbudakguy mentioned this issue Oct 21, 2019
5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.