User assisted XSS

[chrisjohansson]

Environment

• Python version: Python 3.7.5
• NetBox version: v2.8.5

Steps to Reproduce

1. Add the following to any markdown comments section: [click me for XSS](javascript:alert(1))
2. View the comments section and click the link
3. User supplied javascript is executed

Expected Behavior

Javascript URIs to be filtered (or more accurately only http/https URIs to be allowed)

Observed Behavior

User supplied javascript was executed (this could potentially be used to escalate to admin privileges).

[jeremystretch]

NetBox's entire logic around rendering Markdown consists of the following:

def render_markdown(value):
    """
    Render text as Markdown
    """
    # Strip HTML tags
    value = strip_tags(value)

    # Render Markdown
    html = markdown(value, extensions=['fenced_code', 'tables'])

    return mark_safe(html)

As far as I can tell, the Python-Markdown library doesn't provide any mechanisms for filtering hyperlink content. This is a feature that might be requested of the upstream library but IMO is not something that we can reasonably take on in NetBox.

[chrisjohansson]

Hi
I understand your way of thinking and agree that perhaps the upstream library would be the best place for such a filter. I can file a ticket with the Python Markdown project if you think that is helpful?

However please beware of the potentially severe impact to your users. Because this requires user interaction (on the part of a privileged user) the likelihood of this attack is fairly low however the impact can be quite high. Because of the implicit power of the template logic, if a low privileged user can abuse this to escalate to to an account with the ability to add template code (such as 'extras | export template | Can add export template' for example) - s/he can execute arbitrary commands on the server running Netbox.

As such I think it might be worth contemplating some form of mitigation if the upstream library either does not plan on issuing a patch or it takes a long time to issue one.
Regards
Christian

[chrisjohansson]

For referens I filed the following ticket with Python-Markdown: Python-Markdown/markdown#976

[jeremystretch]

I understand the risk, however:

a low privileged user

Modifying an object requires elevated privileges to begin with. There is no risk from unauthenticated accounts or accounts with read-only permission. Given NetBox's role as an infrastructure management system, it seems reasonable that write access of any kind would be granted only to trusted individuals (as opposed to a publicly-facing application).

Addressing this without native support from the Markdown library would require implementing an entire new layer of processing for rendered HTML, e.g. using Bleach. This is non-trivial as it involves the manual whitelisting of allowed tags as well as writing tests to ensure all Markdown rendering remains functional.

I'll leave this open for a while to see if anyone wants to volunteer.

[chrisjohansson]

Sounds reasonable. I agree that any user with write access would already enjoy a certain amount of trust and like I said the fact that it requires a privileged user to actively click on the link obviously makes it lowers the risk.

[jsenecal]

Perhaps this issue could also be used to gather feedback as well on how exactly our user base interacts with markdown. This would help pin-pointing what should be done with Bleach later on.

Also to quote their doc:

Bleach is powerful but it is not fast. If you trust your users, trust them and don’t rely on Bleach to clean up their mess.

So we should keep that in mind...

[jeremystretch]

Regarding performance, if we adopt anything more complex than the current rendering logic, we'll likely move to saving pre-rendered content in the database alongside the raw content. This would require rendering only at write time.

[jeremystretch]

Might it be sufficient to just strip out any string matching e.g. [.+](javascript:.*) prior to Markdown rendering?

[jeremystretch]

I ended up introducing the ALLOWED_URL_SCHEMES configuration parameter, which includes only known-safe schemes (http, https, ftp, etc.). The complete list is documented. We now validate all Markdown-format links against that scheme list prior to rendering Markdown. It is also enforced for custom URL fields.

[chrisjohansson]

Hello again
Sorry that I'm a little late here, I wasn't expecting you to fix this so quickly (kudos)! As you might have seen python-Markdown didn't think it was their problem and also suggested using bleach. I was supposed to make a suggestion for a fix but as I said you were quicker with a fix than I expected. Anyways I found a bypass for your fix which doesn't account for multiline Markdown links, so something like the below still works

[I'm a javascript
 xss link]( javascript:alert(1))

In my humble opinion this should be easier fixed after the Markdown processor has returned. While sanitising untrusted html is very hard and the reason you should use something like bleach, you actually only have to clean the Markdown output which should produce highly compliant html. I think it would be enough to use pythons html.parser lib for this. I wrote the following test code (which could be written much cleaner I'm sure)

    # Render Markdown
    html = markdown(value, extensions=['fenced_code', 'tables'])

    class MyHTMLParser(HTMLParser):
        error = False
        def handle_starttag(self, tag, attrs):
            for key, val in attrs:
                if tag == 'a' and key == 'href':
                    for scheme in settings.ALLOWED_URL_SCHEMES:
                        if val.startswith(scheme):
                            return
                    self.error = True
                
    parser = MyHTMLParser()
    parser.feed(html)
    if (parser.error):
        html = "A link with an invalid scheme was detected - see settings.ALLOWED_URL_SCHEMES"
    
    return mark_safe(html)

Kind Regards
Christian

[jeremystretch]

It would probably be easier to just tweak the regex to ensure multi-line links are captured.

[chrisjohansson]

That would definitely solve my bypass - the question is whether it fixes all? Because the input before the Markdown parser is untrusted you have to get the regex logic exactly the same as what's used in Markdown or run the risk of a bypass. However I admittedly only found this one bypass..

[chrisjohansson]

Hi
Are there any plans to fix the bypass mentioned above? I seem to still be able to produce javascript: links by injecting newlines in the latest (2.9.3) version. Tweaking the regexp should like you suggested do the trick?
/Christian

[chrisjohansson]

ping @jeremystretch

[chrisjohansson]

@jsenecal?

[jsenecal]

Perhaps open a new issue with your new specific use-case (It would get more traction)?

Are you exposing netbox to untrusted users ?

[chrisjohansson]

We're not exposing our instance to untrusted users - I just figured you wanted to plug up this bypass. You already did the hard work of implementing ALLOWED_URL_SCHEMES, why not just fix the regex and be done with it? I can open a new issue if you want, just figured that was unnecessary noise for what amounts to a one line fix?