starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

User assisted XSS #3749

New Issue
Closed
opened 2025-12-29 18:30:57 +01:00 by adam · 17 comments
adam commented 2025-12-29 18:30:57 +01:00
Owner
Copy Link

Originally created by @chrisjohansson on GitHub (Jun 4, 2020).

Originally assigned to: @jeremystretch on GitHub.

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).

Originally created by @chrisjohansson on GitHub (Jun 4, 2020). Originally assigned to: @jeremystretch on GitHub. <!-- NOTE: IF YOUR ISSUE DOES NOT FOLLOW THIS TEMPLATE, IT WILL BE CLOSED. This form is only for reproducible bugs. If you need assistance with NetBox installation, or if you have a general question, DO NOT open an issue. Instead, post to our mailing list: https://groups.google.com/forum/#!forum/netbox-discuss Please describe the environment in which you are running NetBox. Be sure that you are running an unmodified instance of the latest stable release before submitting a bug report, and that any plugins have been disabled. --> ### Environment * Python version: Python 3.7.5 * NetBox version: v2.8.5 <!-- Describe in detail the exact steps that someone else can take to reproduce this bug using the current stable release of NetBox. Begin with the creation of any necessary database objects and call out every operation being performed explicitly. If reporting a bug in the REST API, be sure to reconstruct the raw HTTP request(s) being made: Don't rely on a client library such as pynetbox. --> ### 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 <!-- What did you expect to happen? --> ### Expected Behavior Javascript URIs to be filtered (or more accurately only http/https URIs to be allowed) <!-- What happened instead? --> ### Observed Behavior User supplied javascript was executed (this could potentially be used to escalate to admin privileges).
adam added the status: acceptedtype: feature labels 2025-12-29 18:30:57 +01:00
adam closed this issue 2025-12-29 18:30:58 +01:00
adam commented 2025-12-29 18:31:00 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 4, 2020):

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.

@jeremystretch commented on GitHub (Jun 4, 2020): NetBox's entire logic around rendering Markdown consists of the following: ```python 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](https://python-markdown.github.io/) 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.
adam commented 2025-12-29 18:31:02 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Jun 5, 2020):

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 commented on GitHub (Jun 5, 2020): 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
adam commented 2025-12-29 18:31:02 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Jun 5, 2020):

For referens I filed the following ticket with Python-Markdown: https://github.com/Python-Markdown/markdown/issues/976

@chrisjohansson commented on GitHub (Jun 5, 2020): For referens I filed the following ticket with Python-Markdown: https://github.com/Python-Markdown/markdown/issues/976
adam commented 2025-12-29 18:31:02 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 5, 2020):

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.

@jeremystretch commented on GitHub (Jun 5, 2020): 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](https://github.com/mozilla/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.
adam commented 2025-12-29 18:31:02 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Jun 5, 2020):

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.

@chrisjohansson commented on GitHub (Jun 5, 2020): 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.
adam commented 2025-12-29 18:31:02 +01:00
Author
Owner
Copy Link

@jsenecal commented on GitHub (Jun 9, 2020):

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...

@jsenecal commented on GitHub (Jun 9, 2020): 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](https://github.com/mozilla/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...
adam commented 2025-12-29 18:31:02 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 9, 2020):

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 commented on GitHub (Jun 9, 2020): 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.
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 15, 2020):

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

@jeremystretch commented on GitHub (Jun 15, 2020): Might it be sufficient to just strip out any string matching e.g. `[.+](javascript:.*)` prior to Markdown rendering?
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 15, 2020):

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.

@jeremystretch commented on GitHub (Jun 15, 2020): 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.
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Jun 18, 2020):

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

@chrisjohansson commented on GitHub (Jun 18, 2020): 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
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Jun 18, 2020):

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

@jeremystretch commented on GitHub (Jun 18, 2020): It would probably be easier to just tweak the regex to ensure multi-line links are captured.
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Jun 18, 2020):

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 commented on GitHub (Jun 18, 2020): 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..
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Sep 9, 2020):

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 commented on GitHub (Sep 9, 2020): 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
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Sep 16, 2020):

ping @jeremystretch

@chrisjohansson commented on GitHub (Sep 16, 2020): ping @jeremystretch
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Sep 28, 2020):

@jsenecal?

@chrisjohansson commented on GitHub (Sep 28, 2020): @jsenecal?
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@jsenecal commented on GitHub (Sep 28, 2020):

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

Are you exposing netbox to untrusted users ?

@jsenecal commented on GitHub (Sep 28, 2020): Perhaps open a new issue with your new specific use-case (It would get more traction)? Are you exposing netbox to untrusted users ?
adam commented 2025-12-29 18:31:03 +01:00
Author
Owner
Copy Link

@chrisjohansson commented on GitHub (Sep 29, 2020):

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?

@chrisjohansson commented on GitHub (Sep 29, 2020): 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?
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label status: accepted type: feature
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#3749
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?