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

Improper validation of the next_urls parameter #16702

Closed
ncsc-pt opened this issue Jun 24, 2024 · 0 comments · Fixed by #16705
Closed

Improper validation of the next_urls parameter #16702

ncsc-pt opened this issue Jun 24, 2024 · 0 comments · Fixed by #16705
Assignees
Labels
severity: low Does not significantly disrupt application functionality, or a workaround is available status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@ncsc-pt
Copy link

ncsc-pt commented Jun 24, 2024

Deployment Type

Self-hosted

NetBox Version

v3.7.5

Python Version

3.10

Steps to Reproduce

The NetBox application makes use of redirection on several endpoints. The redirect URL is being supplied through the next_urls parameter.
The HTTP parameter next_urls gets filtered by the following snippet in order to only accept relative URL:

class GetReturnURLMixin:
    """
    Provides logic for determining where a user should be redirected after processing a form.
    """
    default_return_url = None
    def get_return_url(self, request, obj=None):
        # First, see if `return_url` was specified as a query parameter or form data. Use this URL only if it's
        # considered safe.
        return_url = request.GET.get('return_url') or request.POST.get('return_url')
        if return_url and return_url.startswith('/'):
            return return_url
        # Next, check if the object being modified (if any) has an absolute URL.
        if obj is not None and obj.pk and hasattr(obj, 'get_absolute_url'):
            return obj.get_absolute_url()
        # Fall back to the default URL (if specified) for the view.
        if self.default_return_url is not None:
            return reverse(self.default_return_url)

However, the filtering function is only checking that the supplied parameter next_url starts with a /.
This way, it is possible to circumvent this filtering by supplying a double-slashed payload. As an example the following payload would work : //www.google.com

POST /extras/bookmarks/6/delete/?return_url=//www.google.com HTTP/1.1
Host: REDACTED
Cookie: csrftoken=uHkNYy7ewwULISUm6oRD79cYuqcYn6GG; sessionid=REDACTED
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: https://REDACTED/ipam/ip-addresses/1/
csrfmiddlewaretoken=bvb14AzMDUJikxAesgo30mlhfK3T9Fk5v2lESYwQZgtTSfkqou5wXln5z05HmBQB&confirm=true

As expected, once the request is submitted, the application responds with a 302 status code which includes the new location header :

HTTP/1.1 302 Found
Server: nginx/1.20.1
Date: Tue, 28 May 2024 09:25:17 GMT
Content-Type: text/html; charset=utf-8
Content-Length: 0
Connection: close
Location: //www.google.com

One could expect the redirection to fail. Indeed, according to RFC 7231, which defines HTTP/1.1 semantics and content, the Location header field's value must be a valid URI reference.

However, as the RFC does not explicitly mention schema less URLs, they are considered valid URI references. As such it is up to the browser to decide which default schema to use. For Chrome and Firefox it is defaulting to HTTP or HTTPS resulting in a valid redirection.

Expected Behavior

An HTTP error response or a redirect to the main page.

Observed Behavior

Redirect to the targeted website.

@ncsc-pt ncsc-pt added status: needs triage This issue is awaiting triage by a maintainer type: bug A confirmed report of unexpected behavior in the application labels Jun 24, 2024
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation complexity: medium Requires a substantial but not unusual amount of effort to implement severity: low Does not significantly disrupt application functionality, or a workaround is available and removed status: needs triage This issue is awaiting triage by a maintainer complexity: medium Requires a substantial but not unusual amount of effort to implement labels Jun 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
severity: low Does not significantly disrupt application functionality, or a workaround is available status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants