LzOggar commented 5 years ago

Description

A security problem exists when markdownx.js dynamically interprets the content of the form without first checking the content. The application is vulnerable to XSS attack.

Versions

django 1.11.13 and 2.2.5,
python 2.7.16 and 3.7.4.
django-markdownx 2.0.28.
Preview

xenatisch commented 5 years ago

Hi @LzOggar

Thanks for reporting this. This, however, is not something new. It is more of a design dilemma, and has been since I started my contributions to this project. Here's a summary:

It has been argued that this is not an issue for MarkdownX to address. After all, it is the Markdown transpiler that should filter this out. MarkdownX is but a medium.
If we attempt to filter all HTML tags, it would mean that developers won't have the flexibility to incorporate custom tags into their Markdown editors. Whilst tempting for obvious, I do not think that it is a decision for us, but one for the developer. After all, this is not meant to be the end product.

So what now? I think the best course of action would be to:

Warn developers of the possibility of XSS -- e.g. using your very informative GIF.
Come up with a settings variable to block all HTML tags and render them as text -- note that we cannot check every attribute of every tag to ensure there's no JS code. It take too much resources. Although that option is open the developers.

I am not particularly supportive of the latter option because it invades a jurisdiction that belongs to the transpiler. Indeed most, if not all decent transpilers offer the option to ignore raw HTML. If we decide to go down that route, it could get very messy, very soon. That's because if we want to support all transpilers, we're gonna have to parse the resulting HTML file, and that's a can of worms no one wants to open.

Anyhow, these are my thoughts. Happy to hear yours.

eon01 commented 5 years ago

I was trying to discover if a user can inject JS and the surprise was that I was able to pop up alerts and do everything JS can do. So I googled this and landed here. I was also able to add things like:

<body onfocus="alert('')"></body>

or

<meta http-equiv=refresh content=1>

or anything that could block or freeze the page, the browser or the OS ..etc

I think security, in this case, should come before features. I'm using Bleach now in the view to clean all suspect JS code but would love to hear if there are better solutions.

LzOggar commented 5 years ago

Hi,

firstable, I hope you are using whitelist with Bleach.clean function.

More over, you can sanitize all user input with Bleach and save result in db. The render will not execute any arbitrary code. But, i don't know if you patch js middleware with Bleach. So, it could be not working with dynamic render.

Please, let me know more about your patch. Thanks you.

Regards.

eon01 commented 5 years ago

For the rendering, I think it's possible if you create a template tag, something like:

from django import template
register = template.Library()
import bleach

@register.filter
def sanitize(value):
    return bleach.clean(value)

then override the widget template (markdownx/widget2.html):

<div class="markdownx">
    {{ markdownx_editor }}
    <div class="markdownx-preview"></div>
</div>

and use JS to apply the template tag to the preview DIV. I didn't test this yet, but it could help.

LzOggar commented 5 years ago

So, on your advice, I tried and I did not success then I found new way and I got it. Let's explain ;

Add new global var to "markdownx/settings" as you can see here :
Edit "markdownx/utils", modify "mardownify" function as well as you can see here : and save it.

Enjoy ! Here is a valid way and I think the best way to patch this. But, i am not a really fan because, we have to change the default code of markdownx.

eon01 commented 5 years ago

You can do it without modifying the package code by overriding the markdownify() function in your settings.py:

Create a file called utils.py and add your new markdownify() and keep the original one.

MARKDOWNX_MARKDOWNIFY_FUNCTION = '<myapp>.utils.markdownify'

By the way, the new markdownify you used will strip HTML like:

<a href="#">Click</a>
<a href="javascript:alert('XSS')">Click</a>

But not JS inside markdown:

[test](javascript:alert\('test'\))

This is what I used:

from functools import partial

from django.conf import settings
from django.utils.safestring import mark_safe

import markdown
import bleach

def markdownify(text):

    # Bleach settings
    whitelist_tags = getattr(settings, 'MARKDOWNIFY_WHITELIST_TAGS', bleach.sanitizer.ALLOWED_TAGS)
    whitelist_attrs = getattr(settings, 'MARKDOWNIFY_WHITELIST_ATTRS', bleach.sanitizer.ALLOWED_ATTRIBUTES)
    whitelist_styles = getattr(settings, 'MARKDOWNIFY_WHITELIST_STYLES', bleach.sanitizer.ALLOWED_STYLES)
    whitelist_protocols = getattr(settings, 'MARKDOWNIFY_WHITELIST_PROTOCOLS', bleach.sanitizer.ALLOWED_PROTOCOLS)

    # Markdown settings
    strip = getattr(settings, 'MARKDOWNIFY_STRIP', True)
    extensions = getattr(settings, 'MARKDOWNIFY_MARKDOWN_EXTENSIONS', [])

    # Bleach Linkify
    linkify = None
    linkify_text = getattr(settings, 'MARKDOWNIFY_LINKIFY_TEXT', True)

    if linkify_text:
        linkify_parse_email = getattr(settings, 'MARKDOWNIFY_LINKIFY_PARSE_EMAIL', False)
        linkify_callbacks = getattr(settings, 'MARKDOWNIFY_LINKIFY_CALLBACKS', None)
        linkify_skip_tags = getattr(settings, 'MARKDOWNIFY_LINKIFY_SKIP_TAGS', None)
        linkifyfilter = bleach.linkifier.LinkifyFilter

        linkify = [partial(linkifyfilter,
                callbacks=linkify_callbacks,
                skip_tags=linkify_skip_tags,
                parse_email=linkify_parse_email
                )]

    # Convert markdown to html
    html = markdown.markdown(text, extensions=extensions)

    # Sanitize html if wanted
    if getattr(settings, 'MARKDOWNIFY_BLEACH', True):

        cleaner = bleach.Cleaner(tags=whitelist_tags,
                                 attributes=whitelist_attrs,
                                 styles=whitelist_styles,
                                 protocols=whitelist_protocols,
                                 strip=strip,
                                 filters=linkify,
                                 )

        html = cleaner.clean(html)

    return mark_safe(html)

It's the same function here.

In your settings you should add the following configurations and customize them according to your needs:

## markdownify
MARKDOWNIFY_WHITELIST_TAGS = [
  'a',
  'abbr',
  'acronym',
  'b',
  'blockquote',
  'em',
  'i',
  'li',
  'ol',
  'p',
  'strong',
  'ul',
  'pre',
  'code',
]
MARKDOWNIFY_WHITELIST_PROTOCOLS = [
    'http',
    'https',
]
MARKDOWNIFY_LINKIFY_PARSE_EMAIL = True
MARKDOWNIFY_LINKIFY_SKIP_TAGS = ['pre', 'code', ]
MARKDOWNIFY_WHITELIST_ATTRS = [
    'href',
    'src',
    'alt',
    'class',
]

LzOggar commented 5 years ago

The solution seems worked fine. Case closed.

jonathan-daniel commented 4 years ago

Very irresponsible to not mention this in the docs

neutronX / django-markdownx

Security issue - Vulnerable to XSS attack #163

Description

Versions

Preview