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 two persistent XSS flaws #105

Merged
merged 1 commit into from
Feb 27, 2016
Merged

Fix two persistent XSS flaws #105

merged 1 commit into from
Feb 27, 2016

Conversation

qll
Copy link
Contributor

@qll qll commented Feb 23, 2016

I guess I owe you a bit of explanation for this pull request out of the blue: There are two distinct XSS vulnerabilities in the forum's markdown parser. Both affect the [text](url) link syntax.

  1. While the code attempts to prevent javascript:-Links, it actually fails to do so. An attacker can use entities to bypass the check, as browsers will first decode the entities when encountered in attribute values. An exemplary exploit would be [text](javascript&colon;alert1) (the payload uses the backtick to avoid parantheses). This exploit is however not the only one concerning the URL parsing. In Firefox, the following payload suffices to create a link which will run on the same origin as the page: [text](data:text/html,<script>alert1</script>). Additionally, there is vbscript: and a range of extremely evil protocol handlers. Thus, the patch uses a whitelist to prevent all of this in one swoop. One drawback is that relative and kinda-relative URLs do not work in the link tag anymore (/from/root/path and path/to/something). Its up to you to decide if this is acceptable.
  2. The second exploit actually uses the lack of escaping on the link itself. Here is an attack vector: [text]("><script>alert1</script>). Simple, but works. The solution is to simply escape the link, too. This is the more dangerous attack of the two as it does not require user interaction. It was found by @thejh.

@nitely
Copy link
Owner

nitely commented Feb 23, 2016

Oh, nice finds. The first one I think you should report it to mistune project. The second one, It seems I misplace the escaping and didn't test it properly.

@qll
Copy link
Contributor Author

qll commented Feb 23, 2016

And I will send a smiliar patch to the mistune project, too. Thanks for the suggestion.

@nitely
Copy link
Owner

nitely commented Feb 24, 2016

One drawback is that relative and kinda-relative URLs do not work in the link tag anymore (/from/root/path and path/to/something). Its up to you to decide if this is acceptable.

Why not to check the url starts with / ?

@qll
Copy link
Contributor Author

qll commented Feb 25, 2016

That's actually a good idea. What's left are relative/url/paths. I think I have a solid idea for that, too. It's better to wait for the mistune pull request then.

@nitely
Copy link
Owner

nitely commented Feb 25, 2016

What's left are relative/url/paths.

They could be checked with something like markdown.render(text, safe_rel_urls=['my_path/']).

I'd usually not care about relative urls, it may be even safer not to allow them, but I'm using them for image uploads (I could create a data migration to add the full url though).

@nitely
Copy link
Owner

nitely commented Feb 25, 2016

...or just prepend / if not ìs_valid_url().

@qll
Copy link
Contributor Author

qll commented Feb 25, 2016

I figured that such limitations are too much of a burden when considering such a general markdown parser as mistune is. Here is my pull request lepture/mistune#88. Let's see if that gets accepted. I guess the blacklist approach proposed in that pull request would actually be sufficient for the forum, too. People are going to use the forum in unexpected areas and will maybe want to link to unexpected things (bitcoin:// or mailto:// or whatever).

Anyway, thank you for being so patient and figuring out a solution with me. My initial pull request here seems a bit rushed.

@nitely
Copy link
Owner

nitely commented Feb 25, 2016

People are going to use the forum in unexpected areas and will maybe want to link to unexpected things (bitcoin:// or mailto:// or whatever).

I'll probably go with the whitelist approach, I'll add a settings.MARKDOWN_ALLOWED_PROTOCOLS later, since I've to override those methods anyway.

My initial pull request here seems a bit rushed.

I'll likely merge it later thought, at least to fix the escaping and so you appear as a colaborator (for the lack of a colaborators file).

Thank you!

nitely added a commit that referenced this pull request Feb 27, 2016
Fix two persistent XSS flaws
@nitely nitely merged commit 86e9d05 into nitely:master Feb 27, 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.

2 participants