Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#17837 closed Bug (fixed)

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

Reported by: nomulous Owned by: nobody
Component: contrib.markup Version: master
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 ptone 3 years ago.
patch for trunk
markdown-safe-backport.diff (4.2 KB) - added by ptone 3 years ago.
for 1.3.X branch
markdown-safe-2.diff (6.6 KB) - added by ptone 3 years ago.
Updating to trunk

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 4 years ago by nomulous

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 Changed 3 years ago by yuval_a

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 Changed 3 years ago by Fandekasp

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 3 years ago by Fandekasp (previous) (diff)

comment:5 Changed 3 years ago by Fandekasp

  • Cc lemaire.adrien@… added

Changed 3 years ago by ptone

patch for trunk

Changed 3 years ago by ptone

for 1.3.X branch

comment:6 Changed 3 years ago by ptone

  • 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

Changed 3 years ago by ptone

Updating to trunk

comment:7 Changed 3 years ago by PaulM

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 3 years ago by PaulM

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