Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#17837 closed Bug (fixed)

Markdown filter "safe" mode is vulnerable to e.g. 'onclick' attributes

Reported by: Fletcher Tomalty Owned by: nobody
Component: contrib.markup Version: dev
Severity: Release blocker Keywords: javascript, injection, xss, markdown
Cc: lemaire.adrien@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The Python markdown library used by Django has syntax like this, which is obviously a JS injection vulnerability:

{@onclick=alert(1)}paragraph

Somehow, Python-markdown's safe mode still allows this, and thus, Django does too. Since this "safe" mode is what developers are expected to use to render user input, this is a pretty significant security issue.

I have opened a ticket for Python-markdown here: https://github.com/waylan/Python-Markdown/issues/82

Newer versions of Python-markdown have an "enable_attributes" argument that you can pass, while older versions have a constant "ENABLE_ATTRIBUTES" that is declared at the module level. I'm not sure what the best way to fix this in Django directly would be.

Attachments (3)

markdown-safe.diff (6.5 KB ) - added by Preston Holmes 12 years ago.
patch for trunk
markdown-safe-backport.diff (4.2 KB ) - added by Preston Holmes 12 years ago.
for 1.3.X branch
markdown-safe-2.diff (6.6 KB ) - added by Preston Holmes 12 years ago.
Updating to trunk

Download all attachments as: .zip

Change History (11)

comment:1 by Carl Meyer, 12 years ago

Triage Stage: UnreviewedAccepted

Thanks for the report. In future, please send security-related bugs to security@… rather than filing them in Trac (the Trac new-ticket form makes this same request right above the form.)

comment:2 by Fletcher Tomalty, 12 years ago

Sorry, that was stupid of me. I should have done so, but it slipped my mind because this vulnerability is already public for the Python-Markdown library itself.

comment:3 by Yuval Adam, 12 years ago

python-markdown has decided not to fix this (see github issue).

I'm not sure this falls in Django's scope. (Do we want to start white/blacklisting attrs?)

The question might be whether we want to use some third-party app here to assist in cleaning the markdown attrs.

comment:4 by Adrien Lemaire, 12 years ago

At least the Django doc warns the users about that in https://docs.djangoproject.com/en/dev/ref/contrib/markup/:

Warning
The output of markup filters is marked “safe” and will not be escaped when rendered in a template. Always be careful to sanitize your inputs and make sure you are not leaving yourself vulnerable to cross-site scripting or other types of attacks.

We could complete this doc by referring some 3rd-part apps such as bleach maybe ?
This shouldn't be a release blocker anymore.

Last edited 12 years ago by Adrien Lemaire (previous) (diff)

comment:5 by Adrien Lemaire, 12 years ago

Cc: lemaire.adrien@… added

by Preston Holmes, 12 years ago

Attachment: markdown-safe.diff added

patch for trunk

by Preston Holmes, 12 years ago

Attachment: markdown-safe-backport.diff added

for 1.3.X branch

comment:6 by Preston Holmes, 12 years ago

Has patch: set

After some discussion with Paul at the sprints, we decided to deprecate Python-Markdown<2.1

The "safe" arg to the filter in the patches just attached enables both the safe_mode=True and enable_attributes=False in tandem

by Preston Holmes, 12 years ago

Attachment: markdown-safe-2.diff added

Updating to trunk

comment:7 by Paul McMillan, 12 years ago

Resolution: fixed
Status: newclosed

In [17734]:

[1.3.X] Fixed #17837. Improved markdown safety.

Markdown enable_attributes is now False when safe_mode is enabled. Documented
the markdown "safe" argument. Added warnings when the safe argument is
passed to versions of markdown which cannot be made safe.

comment:8 by Paul McMillan, 12 years ago

In [17735]:

Fixed #17837. Improved markdown safety.

Markdown enable_attributes is now False when safe_mode is enabled. Documented
the markdown "safe" argument. Added warnings when the safe argument is
passed to versions of markdown which cannot be made safe. Deprecated
versions of markdown < 2.1. Many thanks to ptone for the patch.

Note: See TracTickets for help on using tickets.
Back to Top