Opened 5 years ago

Closed 6 weeks ago

#13751 closed New feature (wontfix)

Avoid open redirect issue with whitelist

Reported by: anonymous Owned by:
Component: HTTP handling Version: master
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 5 years ago.
secure.2.py (1.4 KB) - added by flier 5 years ago.
openredirect.diff (8.9 KB) - added by flier 5 years ago.
openredirect-with-docs.diff (4.5 KB) - added by dpn 5 years ago.

Download all attachments as: .zip

Change History (28)

Changed 5 years ago by anonymous

comment:1 Changed 5 years ago by flier

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 5 years ago by russellm

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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.

Changed 5 years ago by flier

comment:3 Changed 5 years ago by flier

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 Changed 5 years ago by russellm

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.

Changed 5 years ago by flier

comment:5 follow-up: Changed 5 years ago by 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).

comment:6 in reply to: ↑ 5 Changed 5 years ago by flier

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?

Changed 5 years ago by dpn

comment:7 Changed 5 years ago by dpn

  • 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 Changed 5 years ago by lukeplant

  • 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 Changed 5 years ago by dpn

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 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 4 years ago by d1b

  • Cc d1b added

comment:12 Changed 4 years ago by davidfischer

  • Cc djfische@… added
  • Easy pickings unset

comment:13 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 3 years ago by claudep

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 Changed 2 years ago by aaugustin

#19992 was a duplicate.

comment:14 Changed 8 weeks ago by marcaurele

  • Owner changed from nobody to marcaurele
  • Status changed from new to assigned

comment:15 Changed 8 weeks ago by marcaurele

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 Changed 8 weeks ago by timgraham

  • Patch needs improvement unset

comment:17 Changed 8 weeks ago by apollo13

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 Changed 8 weeks ago by ubernostrum

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 Changed 6 weeks ago by marcaurele

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 Changed 6 weeks ago by marcaurele

  • Owner marcaurele deleted
  • Status changed from assigned to new

comment:21 Changed 6 weeks ago by timgraham

  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top