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

Escape widget error message #186

Merged
merged 2 commits into from Jun 24, 2021
Merged

Escape widget error message #186

merged 2 commits into from Jun 24, 2021

Conversation

gebhardtr
Copy link
Contributor

@gebhardtr gebhardtr commented Jun 17, 2021

To test, visit:

http://localhost:3030/views/number<h1>.html

@gebhardtr gebhardtr changed the title Fix XSS issue Escape widget error message Jun 18, 2021
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

@gebhardtr thanks for the PR!

Confirmed with master version, and the test payload http://localhost:3030/views/number%3Cimg%20src%20=q%20onerror=prompt(8)%3E.html.

@gebhardtr I've allowed the workflow to run for the PR, and then the tests failed. Would you be able to take a look and see if it's something easy to fix, please? Otherwise I can troubleshoot it over next week I think.

Thanks!
Bruno

@kinow
Copy link
Member

kinow commented Jun 18, 2021

@gebhardtr looks like it's rack-test that's unhappy with the URL in the get call. I've applied the suggestion from this issue and it fixed the build on this PR.

Also confirmed that the test with this change, in the master branch was failing.

Minitest::Assertion: --- expected
+++ actual
@@ -1,2 +1 @@
-# encoding: ASCII-8BIT
-"Drats! Unable to find a widget file named: nowidget-<h1> to render."
+"Drats! Unable to find a widget file named: nowidget-&lt;h1&gt; to render."

Let me know if you agree with the fix. Also, I'll prepare a release once it's merged, and will incldue this as a security bug. Let me know if you are OK with your name appearing as reporter for the bug :)

Thanks!
Bruno

@kinow kinow self-assigned this Jun 18, 2021
@kinow kinow added this to the 1.3.5 milestone Jun 18, 2021
@gebhardtr
Copy link
Contributor Author

Absolutely! Thank you very much, @kinow.

@kinow kinow merged commit d1577b1 into Smashing:master Jun 24, 2021
6 checks passed
@kinow
Copy link
Member

kinow commented Jun 30, 2021

Fix released.

The easiest way to exploit this issue is probably by sending a malicious URL to someone with access to the dashboard. That way the executed code could be used to run malicious JS in Smashing that could be used to steal session cookies or other sensitive data.

Most browsers protect against attacks like this, but there is no guarantee a browser or extension would prevent it. However, secure cookies, and cookies in other domains would not leak to this session, making it harder for an attacker to steal them.

I think the risk is greater if the dashboard is running on localhost for development, or maybe in a subdomain like dashboards.domain.com, and somehow there are cookies from domain.com available to the session.

Thanks again for reporting it @gebhardtr . CVE-2021-35440 will be updated and published soon.

Bruno

@kinow kinow added the security label Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants