Opened 14 years ago
Closed 9 years ago
#13751 closed New feature (wontfix)
Avoid open redirect issue with whitelist
Reported by: | anonymous | Owned by: | |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | open redirect, security |
Cc: | d1b, djfische@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
An open redirect is an application that takes a parameter and redirects a user to the parameter value without any validation. This vulnerability is used in phishing attacks to get users to visit malicious sites without realizing it.
For example, the login page could be rewrite with a suspicious next
parameter
http://your_domain_name/login/next=http://www.google.com
and the user will be redirect to the target URL after login, you could check the following document for more detail and suggestion.
http://www.google.com/support/webmasters/bin/answer.py?answer=171297
Even it is not a security issue from Django itself, we still need to provide security mechanism to avoid it, such as whitelist, secure hash etc
#!/usr/bin/env python import re import logging from urlparse import urlparse import django.http from django.conf import settings class HttpResponseSafeRedirect(django.http.HttpResponse): status_code = 302 def __init__(self, redirect_to, whitelist=[], fallback_to=None): django.http.HttpResponse.__init__(self) self['Location'] = redirect_to if urlparse(self['Location']).scheme: matched = False for pattern in whitelist: if hasattr(pattern, 'match'): matched = pattern.match(self['Location']) break else: matched = self['Location'].startswith(pattern) break if not matched: logging.warn("found open redirect attack to %s", self['Location']) self['Location'] = fallback_to or settings.LOGIN_REDIRECT_URL django.http.HttpResponseRedirect = HttpResponseSafeRedirect
Attachments (4)
Change History (28)
by , 14 years ago
comment:1 by , 14 years ago
comment:2 by , 14 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Agreed that we should do something to address this problem.
However, the patch needs to be integrated into Django's source tree, and it needs to be tested and documented.
The patch also requires some improvements -- in particular:
- A common whitelist (REDIRECT_WHITELIST) should be defined in the settings file, augmented by the values provided to the redirect instance.
- The whitelist should assume that all URL patterns are regular expressions, just like URL patterns do.
- There is a backwards compatibility problem. We can't just turn this feature on without causing a major inconvenience to the existing userbase. We will need to phase this in over several versions. This probably means that:
- the settings.py template should have a REDIRECT_WHITELIST setting, but the global settings should not
- if there is no REDIRECT_WHITELIST defined (i.e., this is an old project being upgraded) a warning should be raised, and *all* URLs should be whitelisted. This allows existing sites to be notified that something needs to be addressed without causing a breakage.
- In the next release, we can remove the warning and enable whitelisting for all redirects.
by , 14 years ago
Attachment: | secure.2.py added |
---|
comment:3 by , 14 years ago
Got it and update the patch, thanks for your comments
Just paste the patch, because I can't attach it to the ticket
#!/usr/bin/env python import re import logging from urlparse import urlparse import django.http from django.conf import settings class HttpResponseSafeRedirect(django.http.HttpResponse): status_code = 302 def __init__(self, redirect_to, whitelist=[], fallback_to=None): django.http.HttpResponse.__init__(self) self['Location'] = redirect_to if urlparse(self['Location']).scheme: effective_whitelist = set(whitelist + getattr(settings, 'REDIRECT_WHITELIST', [])) matched = False for pattern in effective_whitelist: matched = re.compile(pattern).match(self['Location']) if matched: break if not matched: logging.warn("found open redirect attack to %s", self['Location']) self['Location'] = fallback_to or settings.LOGIN_REDIRECT_URL def hack_django(cls, *args, **kwds): if hasattr(settings, 'REDIRECT_WHITELIST'): return HttpResponseSafeRedirect(*args, **kwds) else: print "WARN: Use REDIRECT_WHITELIST setting and protect your end user " \ "from the open redirect vulnerability. <http://www.owasp.org/index.php/Open_redirect>" return object.__new__(cls, *args, **kwds) django.http.HttpResponseRedirect.__new__ = staticmethod(hack_django)
comment:4 by , 14 years ago
Thanks for updating the patch, but it's still a long way from being ready for trunk. Please read the contribution guide for details on how to prepare a patch for Django's trunk.
In particular:
- The patch should be a diff against trunk. We're working on an security-related update to HttpRedirect, not a "hack".
- The patch needs tests
- The patch needs documentation
Regarding the changes you've made since last time -- there's no need to move the whitelist through a set. This removes the ability of developers to prioritize pattern matches, and takes extra effort to eliminate duplicates on insertion.
by , 14 years ago
Attachment: | openredirect.diff added |
---|
follow-up: 6 comment:5 by , 14 years ago
I'm pretty uncomfortable with django.http needing to know about LOGIN_REDIRECT_URL, it feels like a really ugly coupling of 2 components that should have a 1 way dependency (auth obviously using django.http, but certainly not the reverse).
comment:6 by , 14 years ago
Replying to Alex:
I'm pretty uncomfortable with django.http needing to know about LOGIN_REDIRECT_URL, it feels like a really ugly coupling of 2 components that should have a 1 way dependency (auth obviously using django.http, but certainly not the reverse).
I agree with you, but we need a fallback URL when we found the open redirect. From my viewpoint, LOGIN_REDIRECT_URL is a alternatives base on the exist setttings, or we could add a new setting FALLBACK_URL?
What's your suggestion?
by , 14 years ago
Attachment: | openredirect-with-docs.diff added |
---|
comment:7 by , 14 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Hey, I couldn't get sphinx to render the docs properly, so I couldn't check the html in a browser, would appreciate it if someone could check that for me.
Also i'm not entirely sure about the use of logging, perhaps there should be something that fits in with the new logging system in #12012
Cheers :)
comment:8 by , 14 years ago
Patch needs improvement: | set |
---|
The logging call is definitely wrong - have a look at the examples in the latest patch on #12012 for how to do it correctly.
comment:9 by , 14 years ago
Dropping a comment here so I don't forget.
Following a conversation with Russ.. I'll change the redirect object to have a flag indicating that a malicious redirect may have happened, then check that flag in the CommonMiddleware and log the message at that point.
This is to get around the issue that there is no request object in scope where the logging call is made at the moment.
comment:10 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:11 by , 14 years ago
Cc: | added |
---|
comment:12 by , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
comment:12 by , 12 years ago
Even if we don't cover all use cases of this ticket, I think that progress has been made with commit [a2f2a399566dd68ce] (#18856).
I think we should now better document:
- Add
is_safe_url
in docs/ref/utils - Add a note in
HttpResponseRedirect
docs that if the path parameter comes from user input, it should be passed first tois_safe_url
to make sure that the redirection is not going to an unwanted location.
comment:14 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:15 by , 9 years ago
Added a new settings REDIRECT_WHITELIST_URLS to allow a list of urls for redirection outside the application host. The match is done in utils.http.is_safe_url(). All tests passed, and the new setting has been documented.
comment:16 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:17 by , 9 years ago
I am for wontfixing, I do not see a good usecase for this now that we have ALLOWED_HOSTS already. Russ, since you initially accepted it: what are your thoughts on that?
comment:18 by , 9 years ago
I feel like there are some tricky issues here, especially with the example given in the documentation -- the part where if a URL in REDIRECT_WHITELIST_URLS is a prefix of the URL being redirected to especially scares me, because it creates the possibility of breaking out of this feature by finding an open redirect on *that* host.
comment:19 by , 9 years ago
After another thought on that fix, I must say that I share apollo13 point of view and don't see a good use case. I'm for a wontfixing.
comment:20 by , 9 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:21 by , 9 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Just register a account to follow up the ticket, please consider to merge it to next version. Thanks