Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XSS in markdown rendering #7788

Closed
SoufElhabti opened this issue Nov 9, 2021 · 14 comments
Closed

XSS in markdown rendering #7788

SoufElhabti opened this issue Nov 9, 2021 · 14 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@SoufElhabti
Copy link

NetBox version

v3.0.9

Python version

3.8

Steps to Reproduce

  1. installed Netbox from docker https://hub.docker.com/r/netboxcommunity/netbox/
  2. use any input that accepts markdown
  3. submit this payload :
[37qpypz37qpypz37qpypz37qpypz37qpypz]  
37qpypz37qpypz37qpypz37qpypz37qpypz]: javascript:alert(1)
  1. when the submiting the for click the link and the xss will fire up

Expected Behavior

the payload is dangerous and allowing XSS attack

Observed Behavior

executing javascript in admin dashboard

@SoufElhabti SoufElhabti added the type: bug A confirmed report of unexpected behavior in the application label Nov 9, 2021
@jeremystretch
Copy link
Member

  1. use any input that accepts markdown

Please specify exactly what you're doing.

@jeremystretch jeremystretch added the status: revisions needed This issue requires additional information to be actionable label Nov 9, 2021
@SoufElhabti
Copy link
Author

when for example a adding a site in /dcim/sites/ in the comment section submit the payload

 [37qpypz37qpypz37qpypz37qpypz37qpypz]  
37qpypz37qpypz37qpypz37qpypz37qpypz]: javascript:alert(1)

if this wasn't helpful please refer to the video i recorded for the POC
POC VIdeo

@jeremystretch
Copy link
Member

I'm not able to reproduce this on NetBox v3.0.9. I just see the plain string as entered. It's possible this may be unique to the docker image, in which case you'll need to open a separate bug against that project. What version of Markdown do you have installed?

Screenshot_2021-11-09 Foo NetBox

@SoufElhabti
Copy link
Author

sorry for giving you hard time reproducing it. It is :

 [37qpypz37qpypz37qpypz37qpypz37qpypz]  
[37qpypz37qpypz37qpypz37qpypz37qpypz]: javascript:alert(1)

i forgot a [

@kkthxbye-code
Copy link
Contributor

Duplicate of #4717 - the recommended solution for python-markdown is to filter the html with bleach. Another solution could be to swap it out for another parser that enforces rules for links. Third option would be to try to patch the regex filter in render_markdown.

You don't have to do all that weird stuff he's doing in the payload though, a simple example like this should work:

[test
test](javascript:alert(0))

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: revisions needed This issue requires additional information to be actionable labels Nov 9, 2021
@DanSheps
Copy link
Member

DanSheps commented Nov 9, 2021

That does not work, at least under a normally configured instance

@DanSheps
Copy link
Member

DanSheps commented Nov 9, 2021

Found the hole...

So, you need to use a reference style link:

# Does not work
[Test](javascript:alert(1))


# Does work
[Test 2][ref]

[ref]: javascript:alert(1)

@kkthxbye-code
Copy link
Contributor

You are misunderstanding the one I posted. You need the newline to bypass the regex.

I verified on both docker and local versions, also you can see it here on the demo instance.

https://demo.netbox.dev/dcim/devices/17/

Again, it's all mentioned in the duplicate issue I linked. The reference style version is new though I guess, but not really the main issue.

@DanSheps
Copy link
Member

DanSheps commented Nov 9, 2021

Yes, but if you look at the reference style, that is exactly the style that he is using (he just omited the actual link and only included the reference)

So it looks like both methods do bypass the allowed URL schemes.

@DanSheps DanSheps added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation labels Nov 9, 2021
@DanSheps
Copy link
Member

DanSheps commented Nov 9, 2021

Summary:

ALLOWED_URLS_SCHEMES check is bypassed on either of the following conditions:

  1. Multi-line links
  2. Reference style links

The question is, do we want to sink time into this. To quote from the previous issue which quotes from the bleach docs:

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.

As NetBox is not a end-user facing application, do we want to worry about this too much?

@jeremystretch
Copy link
Member

A few thoughts:

  1. If we were to invoke something "heavy" like Bleach, we would need to first transition to pre-rendered HTML for all objects to mitigate the performance penalty. This would be a major change representing significant effort.
  2. There may be valid use cases for javascript: links, particularly where plugins are in use.
  3. Even using Bleach doesn't guarantee complete protection against XSS. The only surefire approach would be to strip all HTML tags, which is obviously undesirable.

@SoufElhabti
Copy link
Author

i suggest something such as navigate-to which will prevent opening stuff like javascript:

@DanSheps
Copy link
Member

i suggest something such as navigate-to which will prevent opening stuff like javascript:

My understanding is no browser supports that tag.

Additionally, as stretch mentioned, there may be instances where javascript: links are desired.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Nov 11, 2021
@jeremystretch jeremystretch self-assigned this Nov 11, 2021
@jeremystretch
Copy link
Member

I've modified and extended the regular expressions to match both examples above (the multi-line link and the reference link). I believe this addresses the concern raised by the bug report.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

4 participants