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)

secure.py (1.0 KB ) - added by anonymous 14 years ago.
secure.2.py (1.4 KB ) - added by Flier Lu 14 years ago.
openredirect.diff (8.9 KB ) - added by Flier Lu 14 years ago.
openredirect-with-docs.diff (4.5 KB ) - added by David Novakovic 14 years ago.

Download all attachments as: .zip

Change History (28)

by anonymous, 14 years ago

Attachment: secure.py added

comment:1 by Flier Lu, 14 years ago

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

comment:2 by Russell Keith-Magee, 14 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 Flier Lu, 14 years ago

Attachment: secure.2.py added

comment:3 by Flier Lu, 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 Russell Keith-Magee, 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 Flier Lu, 14 years ago

Attachment: openredirect.diff added

comment:5 by Alex Gaynor, 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).

in reply to:  5 comment:6 by Flier Lu, 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 David Novakovic, 14 years ago

Attachment: openredirect-with-docs.diff added

comment:7 by David Novakovic, 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 Luke Plant, 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 David Novakovic, 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 Julien Phalip, 14 years ago

Severity: Normal
Type: New feature

comment:11 by d1b, 14 years ago

Cc: d1b added

comment:12 by David Fischer, 13 years ago

Cc: djfische@… added
Easy pickings: unset

comment:13 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Claude Paroz, 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 to is_safe_url to make sure that the redirection is not going to an unwanted location.

comment:13 by Aymeric Augustin, 12 years ago

#19992 was a duplicate.

comment:14 by Marc-Aurèle Brothier, 9 years ago

Owner: changed from nobody to Marc-Aurèle Brothier
Status: newassigned

comment:15 by Marc-Aurèle Brothier, 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.

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

comment:16 by Tim Graham, 9 years ago

Patch needs improvement: unset

comment:17 by Florian Apolloner, 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 James Bennett, 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 Marc-Aurèle Brothier, 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 Marc-Aurèle Brothier, 9 years ago

Owner: Marc-Aurèle Brothier removed
Status: assignednew

comment:21 by Tim Graham, 9 years ago

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top