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

Fix #80 and #87 #88

Closed
wants to merge 2 commits into from
Closed

Fix #80 and #87 #88

wants to merge 2 commits into from

Conversation

qll
Copy link
Contributor

@qll qll commented Feb 25, 2016

In order to fix #80, a simple call to escape is sufficient. However, #87 is actually a beast. It is fairly easy to fix if you would use a whitelist of allowed schemes (HTTP, HTTPS, ...) but for a general markdown parser I figured this would be too much of a burden to use for developers. You probably expect things to just work. Thus, I attempted to implement a blacklist. The attack vectors this could should protect against are listed in the test case, but here is an explanation for each of them:

  • javascript:alert1``: Standard. You protected against this already.
  • jAvAsCrIpT:alert1``: Just a little variation on the original vector (still works in every browser)
  • javascript:alert1``: The entity attack. Every browser will first decode the entity and then execute our payload (https://jsfiddle.net/kLv02bun/).
  • \x1Ajavascript:alert1``: The "weird" one. Different browsers allow different characters in front of (and after) a scheme. This vector is for Chrome (https://jsfiddle.net/fxdfn1g6/). This is the reason I had to cut out nonalphanumeric characters in the escape_link function.
  • data:text/html,<script>alert1``</script>: Just a variation on the scheme. Even though it alerts in every browser, it is actually only a danger in Firefox.
  • vbscript:msgbox: Protect against old Internet Explorer XSS. For good measure.

Note, that I had to fix an unrelated test case which used links. As far as my understanding of the HTML spec goes, it is more correct now, as & always has to be transformed to &amp; due to its special meaning.

@lepture
Copy link
Owner

lepture commented Feb 26, 2016

Great. Thanks. Merged in 7720190

@lepture lepture closed this Feb 26, 2016
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.

renderer link not escaped
2 participants