Avoid open redirect issue with whitelist

[anonymous]

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

[Flier Lu]

Just register a account to follow up the ticket, please consider to merge it to next version. Thanks

[Russell Keith-Magee]

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.

[Flier Lu]

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)

[Russell Keith-Magee]

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.

[Alex Gaynor]

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

[Flier Lu]

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?

[David Novakovic]

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

[Luke Plant]

The logging call is definitely wrong - have a look at the examples in the latest patch on #12012 for how to do it correctly.

[David Novakovic]

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.

[Jacob]

Milestone 1.3 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Claude Paroz]

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 to is_safe_url to make sure that the redirection is not going to an unwanted location.

[Aymeric Augustin]

#19992 was a duplicate.

[Marc-Aurèle Brothier]

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.

​https://github.com/marcaurele/django/tree/ticket_13751

[Florian Apolloner]

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?

[James Bennett]

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.

[Marc-Aurèle Brothier]

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.