Skip to content

Python: Mention more sanitisation options in py/url-redirection qhelp. #15176

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

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

max-schaefer
Copy link
Contributor

Copy link
Contributor

QHelp previews:

python/ql/src/Security/CWE-601/UrlRedirect.qhelp

URL redirection from remote source

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.

Recommendation

To guard against untrusted URL redirection, it is advisable to avoid putting user input directly into a redirect URL. Instead, maintain a list of authorized redirects on the server; then choose from that list based on the user input provided.

If this is not possible, then the user input should be validated in some other way, for example, by verifying that the target URL does not include an explicit host name.

Example

The following example 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, the Django framework provides a function url_has_allowed_host_and_scheme that can be used to check that a URL is safe to redirect to, as shown in the following example:

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('/')

Note that many browsers accept backslash characters (\) as equivalent to forward slash characters (/) in URLs, so it is important to account for this when validating the URL, for example by first replacing all backslashes with forward slashes. Django's url_has_allowed_host_and_scheme function does this automatically, but other libraries may not.

References

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

But we could use a non-django example for how to check for missing hostname.

@max-schaefer
Copy link
Contributor Author

Agreed in principle, but (as far as I can see) at the moment the query only recognises this one particular sanitiser, so if we suggest another one it wouldn't actually make the alert go away. I have raised #15178 to discuss this further.

@max-schaefer max-schaefer marked this pull request as ready for review December 20, 2023 14:05
@max-schaefer max-schaefer requested a review from a team as a code owner December 20, 2023 14:05
@sidshank sidshank requested a review from tausbn January 3, 2024 12:04
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. 👍

@max-schaefer max-schaefer added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jan 12, 2024
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to expand the documentation. Nothing to suggest here 💖

@max-schaefer max-schaefer merged commit a833632 into main Jan 12, 2024
@max-schaefer max-schaefer deleted the max-schaefer/py-url-redirection-qhelp branch January 12, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Python ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants