evilstreak / markdown-js

A Markdown parser for javascript
7.69k stars 863 forks source link

XSS vulnerability #235

Open mhuggins opened 9 years ago

mhuggins commented 9 years ago

I just tried implementing this in conjunction with bootstrap-markdown, and one of the first things I tested was how JS is stripped from links. It appears that links are not sanitized for JS input. Take the following example:

[link](javascript:alert(alert))

This is being converted to:

<p><a href="javascript:alert(alert)">link</a></p>

Clicking on it shows a popup with the value of alert. This is obviously not a dangerous example, but a well-crafted string might be able to put site users in danger.

ashb commented 9 years ago

So this is a tough one - I know of some people using this module who want javascript: links preserved, but this does suck for user input.

One possible way around this is to add a filter using this technique https://github.com/evilstreak/markdown-js#more-options (using this to loop over all links, rather than this is XSS protection) - if we add something like this to markdown-js then it needs to be optional (but on by default is fine)

evilstreak commented 9 years ago

One of the features of Markdown we still don't support (through lack of care and attention by us maintainers, rather than anything else) is allowing raw HTML to be included. Given that we definitely should support that, and doing so would allow any malicious HTML at all through, I'm not sure about shipping XSS protection with this code base.

That said, some clear documentation about how to do thinks like link sanitisation, stripping out raw HTML, or just doing a yes/no validation on whether the parsed content is safe would be a Very Good Thing.