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

Sanitize content to avoid XSS #60

Closed
Framartin opened this Issue Mar 4, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@Framartin

Framartin commented Mar 4, 2017

Markdown Preview Plus makes Chrome/Chromium vulnerable to XSS attacks on files that are not designed to be interpreted by web applications.

How to reproduce

  1. An malicious user creates a txt file (or another format supported by Markdown Preview Plus) with the following content: <script>alert(0)</script>
  2. He uploads it on a remove server using a web application
  3. If the web application allows the opening of txt files in the browser, Markdown Preview Plus is a vector for XSS attacks, because the JS payload will be executed

This behavior makes all users of Markdown Preview Plus vulnerable to XSS attacks in a lot of web sites, because these websites are not designed to escape or force the download of txt files.

How to fix

Markdown Preview Plus should sanitize the content in order to avoid XSS.

@volca

This comment has been minimized.

Owner

volca commented Mar 6, 2017

Markdown Preview Plus use library marked to render html. I think sanitize option can resolve the issue.

see markedjs/marked#203

@Framartin

This comment has been minimized.

Framartin commented Mar 6, 2017

Thanks for pointing this issue.

I have changed the marked option by adding sanitize: true, in js/config.js#L13. It's working as expected to prevent XSS.

I want to make sure that MathJax is still working. But there is something that I don't understand:

  • MathJax is correctly rendered in file:///.../markdown-preview/test/test_mathjax.markdown by the 0.5.6 version of the extension from the Store
  • But MathJax in the same file isn't rendered when I'm using a local version of the 0.5.6 version of the extension cloned from the GitHub repository without any modification

I have make sure that in both cases the option Enable MathJax was checked in the options. Am I doing something wrong?

@Framartin

This comment has been minimized.

Framartin commented Mar 6, 2017

Ok. I got it: MathJax seems to work only if the auto-reload option is checked. That was the difference between the store version (auto-reload was enabled) and my local version (auto-reload disabled). Pretty strange.

@Framartin

This comment has been minimized.

Framartin commented Mar 6, 2017

The problem of enabling the sanitize option in marked is that MathJax don't work anymore, because its content is printed as raw HTML inside the page. Any idea on how to fix that?

Everything else seems fine.

@Framartin

This comment has been minimized.

Framartin commented Mar 6, 2017

Also, marked should be updated without using the marked.min.js file, because of this issue.

@volca

This comment has been minimized.

Owner

volca commented Mar 7, 2017

Thank you for notify on with this issue. I will push a new version soon.

@volca volca added the Bug label Mar 7, 2017

@volca

This comment has been minimized.

Owner

volca commented Mar 7, 2017

MathJax is really a problem if we change sanitize to true. I think we can resolve part of this issue -- when mathjax is enabled, change sanitize to false.

@volca volca closed this in 1181f04 Mar 7, 2017

volca pushed a commit that referenced this issue Mar 7, 2017

@Framartin

This comment has been minimized.

Framartin commented Jul 24, 2017

CVE-2017-11593 is attributed to this issue.

@volca

This comment has been minimized.

Owner

volca commented Jul 24, 2017

Thanks for let me know it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment