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

Ensure escaping during SSR #1265

Open
kof opened this issue Jan 15, 2020 · 3 comments
Open

Ensure escaping during SSR #1265

kof opened this issue Jan 15, 2020 · 3 comments
Labels
bug It went crazy and killed everyone. complexity:high Best brains need to talk about it. help wanted important The thing you do when you wake up!

Comments

@kof
Copy link
Member

kof commented Jan 15, 2020

Script injection

Expected behavior:
CSS rendered serverside needs HTML escaping by default because of script injection attack.

Describe the bug:

There is a known security issue with rendering CSS serverside and interpolating user content into it. It's the same issue that plagues all backend frameworks forever. React is very explicit about it by providing only one way to render unescaped HTML using dangerouslySetInnerHTML

In JSS this is also possible since any enduser's value can be used and if devs (JSS users) don't escape the attacker can do this:

{
  root: {
    backgroundColor: "#FFF;}</style><script>alert(document.cookie)</script>"
  }
}

In the case of JSS this is only possible with SSR, because the techniques we use on the client style.textContent and sheet.insertRule don't evaluate HTML.

Codesandbox link:
https://codesandbox.io/s/elated-jepsen-qox0m

Versions (please complete the following information):

  • jss: any
  • Browser any
  • OS any

Solution:

When we call registry.toString() we can escape closing tags example implementation I don't know if we need to escape anything else except of the closing </style tag, since this is the only way I can think of how an attacker can inject script. Let me know if there is any other way.

  • Important consideration is that if we go for generic html escaping is that we would escape stuff that's inside of content property or urls of backgrounds, which will result in undesired effects.
  • Another cosnideration is we need to make sure all kinds of characters are converted (unicode etc.)

CSS injection attack

There is another attack vector with CSS injection, which is discussed here.

The problem is well described by @jamesknelson https://frontarm.com/james-k-nelson/how-can-i-use-css-in-js-securely/

TLDR if attacker is able to inject custom CSS, they can capture key strokes and send requests using background images more about it here

Same issue at styled components

@kof kof added bug It went crazy and killed everyone. complexity:high Best brains need to talk about it. important The thing you do when you wake up! labels Jan 15, 2020
@Janpot
Copy link

Janpot commented Jan 15, 2020

FYI: My mitigation as a quick fix was to escape all / characters in the SSRed CSS.

sheets.toString().replace(/(?<!\\)\//g, '\\/');

For context: I raised this issue initially with @material-ui.

@kof
Copy link
Member Author

kof commented Jan 15, 2020

@Janpot as a hot fix this is ok. For a library it needs more than that.

@Janpot
Copy link

Janpot commented Jan 15, 2020

I agree. Also, just want to note that the SSR issue is slightly different than the one raised by @jamesknelson. It has a much higher impact as it allows for arbitrary javascript execution, while the other issue only is about making arbitrary requests. Not that it matters much, just want to make sure there's two issues here, and fixing the script injection solves only half of the problem.
I'm not exactly sure how that other issue could be solved though. (Or whether it should be solved at all by CSS-in-JS libraries)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It went crazy and killed everyone. complexity:high Best brains need to talk about it. help wanted important The thing you do when you wake up!
Projects
None yet
Development

No branches or pull requests

2 participants