Opened 8 years ago

Closed 6 years ago

#5227 closed (fixed)

Redirect security check in login code is incomplete

Reported by: Sander Dijkhuis <sander.dijkhuis@… Owned by: adrian
Component: Contrib apps Version: 1.0
Severity: Keywords: auth
Cc: sander.dijkhuis@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The security check for the value of redirect_to in django.contrib.auth.views.login is incomplete. It's meant to block incorrect URLs and external locations, but it will still redirect to external sites if the URL doesn't include the protocol name. This is because '' isn't blocked. So currently, /accounts/login/?next=example.com/ will redirect the user to http://example.com/ after a successful authentication. This can be considered a small security problem.

It can be fixed by modifying line 20:

            if not redirect_to or '://' in redirect_to or ' ' in redirect_to:

should be:

            if not redirect_to or '//' in redirect_to or ' ' in redirect_to:

Change History (4)

comment:1 Changed 8 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

(In [6004]) Fixed #5227 -- Made the redirect security check in django.contrib.auth.views.login() tighter. Thanks, Sander Dijkhuis

comment:2 Changed 6 years ago by anonymous

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

comment:3 Changed 6 years ago by dnagoda@…

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from SVN to 1.0

I believe that the implemented fix is overly broad. My concrete example is the OAuth token authorization cycle. In order for a user to authorize the token a GET request is made to the OAuth provider with an oauth_callback parameter containing the callback URL. Properly escaped this parameter will look like this:

oauth_callback=http%3D//example.com/

Given that the check for '' in the redirect checks the entire string, a query parameter as above will cause the security check to be triggered and the user will be redirected incorrectly.

If the concern is that a redirect request that starts with '' could (that's a big "could" because I believe that it's browser dependent what the behavior would be) then I think the appropriate fix is to change the check to this:

if not redirect_to redirect_to.startswith('//') or '://' in redirect_to or ' ' in redirect_to:

comment:4 Changed 6 years ago by Alex

  • Resolution set to fixed
  • Status changed from reopened to closed

This bug was fixed 2 years ago, if you think there is a new bug please file a new ticket.

Note: See TracTickets for help on using tickets.
Back to Top