fix: URL redirection from authenticate_user

[odaysec]

fix the problem to validate the user-provided URL before using it in the redirection. We can use the urlparse function from the Python standard library to parse the URL and check that it does not include an explicit host name. This ensures that only relative paths within the application's domain are allowed for redirection. modify the authenticate_user method in the AuthenticationHelpers class to include this validation. Specifically, we will replace the current redirection logic with a check that ensures the next parameter in the URL is a relative path.

Directly incorporating user input into a URL redirect request without validating the input can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a malicious site that looks very similar to the real site they intend to visit, but which is controlled by the attacker.

POC

The following vulnerable shows an HTTP request parameter being used directly in a URL redirect without validating the input, which facilitates phishing attacks:

from flask import Flask, request, redirect

app = Flask(__name__)

@app.route('/')
def hello():
    target = request.args.get('target', '')
    return redirect(target, code=302)

If you know the set of valid redirect targets, you can maintain a list of them on the server and check that the user input is in that list:

from flask import Flask, request, redirect

VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"

app = Flask(__name__)

@app.route('/')
def hello():
    target = request.args.get('target', '')
    if target == VALID_REDIRECT:
        return redirect(target, code=302)
    else:
        # ignore the target and redirect to the home page
        return redirect('/', code=302)

Often this is not possible, so an alternative is to check that the target URL does not specify an explicit host name. For example, you can use the urlparse function from the Python standard library to parse the URL and check that the netloc attribute is empty.

from flask import Flask, request, redirect
from urllib.parse import urlparse

app = Flask(__name__)

@app.route('/')
def hello():
    target = request.args.get('target', '')
    target = target.replace('\\', '')
    if not urlparse(target).netloc and not urlparse(target).scheme:
        # relative path, safe to redirect
        return redirect(target, code=302)
    # ignore the target and redirect to the home page
    return redirect('/', code=302)

For Django application, you can use the function url_has_allowed_host_and_scheme to check that a URL is safe to redirect to, as shown in the following code:

from django.http import HttpResponseRedirect
from django.shortcuts import redirect
from django.utils.http import url_has_allowed_host_and_scheme
from django.views import View

class RedirectView(View):
    def get(self, request, *args, **kwargs):
        target = request.GET.get('target', '')
        if url_has_allowed_host_and_scheme(target, allowed_hosts=None):
            return HttpResponseRedirect(target)
        else:
            # ignore the target and redirect to the home page
            return redirect('/')

References

XSS Unvalidated Redirects and Forwards Cheat Sheet
urllib.parse
CWE-601

[somethingnew2-0]

urlparse was renamed to urllib.parse in Python 3, it should be updated to that and also imported

https://docs.python.org/3/library/urllib.parse.html#module-urllib.parse

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

api/authentication.py:50

• [nitpick] Consider parsing next_url once into a variable (e.g., parsed_url) instead of calling urlparse twice for improved readability and minor performance benefits.

if not urlparse(next_url).netloc and not urlparse(next_url).scheme:

[fearnoeval]

Original message

I don't believe there's an issue here. For reference, here's the original:

redirect_uri = "{login}?next={here}".format(
    login=url_for("oidc_auth.login"),
    here=quote_plus(request.url),
)

• login is a string pulled from the flask-oidc Blueprint, so it's not user input
• here is a quoted version of request.url, which is the full request URL. If the full request URL is not for this application, then the request will have never reached this application

Am I missing something?

I was indeed missing something in my original comment above. I was directed to this PR thinking it was calling out an existing vulnerability, but it appears its intention is to fix a bug, and is defending the approach used. Apologies for that.

That being said, I do think this has issues in that it will only redirect correctly if the next query parameter exists, which is not the common case. For instance, if one is running locally and tries to visit http://localhost:3000/requests while not logged in, the next query parameter is not set, so one would not be redirected to /requests after a successful login.

If I understand correctly, as these changes stand, if the next query parameter is not passed, the conditional introduced will always be entered since both not urlparse("").netloc and not urlparse("").scheme are true. From there:

• redirect_uri will be /oidc/login?next=
• return redirect(redirect_uri) will run with that value
• The redirect to /oidc/login?next= goes back to the client, which will then GET it
• The underlying flask-oidc library handling /oidc/login will set session["next"] to the empty string since it's the value associated with the next query parameter before redirecting to Okta
• When Okta hands back control by redirecting the client to /oidc/authorize, flask-oidc will read the empty string out of session["next"], and then redirect using that
• When that response reaches the browser, it'll be a redirect with a Location header set to the empty string, which will hang the flow indefinitely at /oidc/authorize