XSS vulnerability

[jorilallo]

It seems that link and image titles can be abused easily. Here's and example:

[Poolparty!](http://www.missionmission.org/wp-content/uploads/2013/08/pool-party-560x328.jpg" onmouseover=alert(document.cookie) id=test)

This is prevented on titles but not in urls. Fix is pretty easy:

url = escapeCharacters(url,"*_");
url = url.replace(/"/g,""");

[tgienger]

Wouldn't it be better to be more specific with the regular expression? For this particular one I'm using:

text = text.replace(/(\[((?:\[[^\]]*\]|[^\[\]])*)\]\([ \t]*()(?:(?:['"])?((?:(?:http|https):\/\/)?(?:[\w\-_]+(?:(?:\.[\w\-_]+)+))(?:[\w\-\.,@?^=%&:\/~\+\!#]*[\w\-\@?^=%&\/~\+#]))['"]?)\))/g, writeAnchorTag);

This doesn't allow spaces at the end of the url or anything between the quote and ending parenthesis.
example:

[parses](http://www.example.com)
[does not parse](http://www.example.com" )

UPDATE: This one allows for inline url with title - the one I'm currently using:

text = text.replace(/(\[((?:\[[^\]]*\]|[^\[\]])*)\]\([ \t]*()(?:(?:['"])?((?:(?:http|https):\/\/)?(?:[\w\-_]+(?:(?:\.[\w\-_]+)+))(?:[\w\-\.,@?^=%&:\/~\+\!#]*[\w\-\@?^=%&\/~\+#]))['"]?)()()(?:\s)?(?:['"](.*)(?:['"]))?\))/g, writeAnchorTag);

example:

[parses](http://www.example.com "Title")
[does not parse](http://www.example.com "title" ) // No spaces or characters after last parenthesis
[does not parse](www.example.com  "title") // More than one space between url and title

When adding inline title it must have two quotes.

[tivie]

@tgienger @jorilallo Maybe some simple xss prevention is a good idea. A couple of approaches come to mind:

1. Adding some basic XSS escaping that you can activate through options. This can run after the conversions are done and over the produced HTML. We could also provide a way to change the XSS escaping function so that users can run more sophisticated algorithms.
2. Providing a simple hook system that enables you to switch in place a determined sub converter.
3. Adding a pre parser that cleans the input directly.

However, I'm a bit averse to add this kind of protection into the core converters because:

• I don't think it's the job of a converter/parser. A converter's output should match the input as closely as possible.
• This puts the extra burden of assuring security into the library, something I don't feel confident (or competent) enough to do.
• This does not prevent XSS attacks if showdown is used as a client-side rich text editor (nothing prevents me from inspecting and altering the library in such way that input is no longer escaped).
  • This can lead to a false sense of security
• Also, since markdown allows embeded html tags, Showdown would need to start parsing and escaping those too.
• npm has a great many number of security libraries and filters that deal with this kind of issues. Those can be run on Showdown output (which is pure html)

Opinions and feedback are welcome.

[pdeschen]

This is definitely not the responsibility of a converter to protect against XSS. I would defer to using a XSS filter on showdown output. See https://github.com/leizongmin/js-xss.

That being said, I think we should add a note about XSS aspects.

[pdeschen]

Best way to handle this would be to have a showdown-xss output type extension dealing with such matter. Anyone up to the task? I've added a reference in Contributing wiki page.

Closing for now.

[markgeraty]

@pdeschen We just published showdown-xss-filter to npm using the library you suggested above. Seems to work great, hope that helps!

[pdeschen]

@markgeraty nice! you should add unit test and bower support eventually, with a dep over xss (which is already available within bower)

[markgeraty]

Created issues for those - feel free to create an issue for anything else you think we are missing!

VisionistInc/showdown-xss-filter#1
VisionistInc/showdown-xss-filter#2

[tivie]

@markgeraty Awesome! Kudos!

[tivie]

For more information, please see the wiki page about Markdown's XSS Vulnerability and how to mitigate it