Skip to content
This repository

Safe mode *must* disallow attributes like "onload", "onclick" #82

Closed
fletom opened this Issue March 05, 2012 · 2 comments

2 participants

Fletcher Tomalty Waylan Limberg
Fletcher Tomalty

Possibly by disabling attributes altogether (like enable_attributes=False), or by applying a filter. There is a fairly comprehensive list at http://ha.ckers.org/xss.html#XSS_Event_handlers

This is the mode that people are expected to use when rendering user input. This has to be the default, especially since most people don't know about or use this attribute syntax in the first place. The output must be actually guaranteed safe, not just partly safe. This vulnerability is just as dangerous as javascript: links.

{@onclick=alert('hi')}some paragraph

In fact, all Django sites that user markdown current have this vulnerability, because the markdown template filter does not pass enable_attributes=False when called with 'safe'.

Waylan Limberg
Owner

As we have documented:

Note that "safe_mode" does not alter the enable_attributes option, which
could allow someone to inject javascript (i.e., {@onclick=alert(1)}). You
may also want to set enable_attributes=False when using "safe_mode".

Unfortunately, this is an issue of poor naming. Perhaps it should have been called "raw_html" rather than "safe_mode". However, we have left it for historical (backward compatible) reasons. Too many other libraries and projects expect the current name so we leave it.

The fact is, with our extension API, we have no way of ensuring that any third party extensions are "safe" either. If you really want something that is "safe", then I suggestion a third party post-processor which completely sanitizes markdown's output. Perhaps something like bleach.

I should also note that this request is for a blacklist that blocks "attributes like 'onload', onclick'." Whereas solutions like bleach implement a whitelist. Whitelists block everything that is not known to be safe, which is a much safer approach. If markdown was to continue on this blacklist approach, we would constantly be chasing new "unsafe" input. For that reason, we will not be implementing the suggested solution. As existing solutions already exist, I'm closing this wontfix.

Waylan Limberg waylan closed this March 05, 2012
Fletcher Tomalty

You are right that a blacklist is never the right answer. I meant a filter of non-whitelisted attributes, and only providedthe blacklist for reference.

It's not an issue of poor naming. The "safe" mode does more than just not interpret "raw_html", like filtering javascript: links. It should do as it says and make the output safe.

I understand that backwards compatibility is an issue, but security is even more important. Security should always be the default, not an extra option like enable_attributes. I suspect that many, many sites using markdown have this vulnerability. Not fixing it would be simply irresponsible.

Waylan Limberg waylan reopened this March 26, 2012
Waylan Limberg waylan closed this issue from a commit May 03, 2012
Waylan Limberg Fixed #82. 'enable_attributes' honors 'safe_mode'.
Note that you can still explicitly set 'enable_attributes' and that
value will be honored regardless of 'safe_mode'. However if 'safe_mode'
is on and 'enable_attributes' is not explicitly set, then
'enable_attributes' defaults to False.
c64c196
Waylan Limberg waylan closed this in c64c196 May 03, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.