Opened 9 years ago

Closed 7 years ago

#5227 closed (fixed)

Redirect security check in login code is incomplete

Reported by: Sander Dijkhuis <sander.dijkhuis@… Owned by: Adrian Holovaty
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 9 years ago by Adrian Holovaty

Resolution: fixed
Status: newclosed

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

comment:2 Changed 7 years ago by anonymous

comment:3 Changed 7 years ago by dnagoda@…

Resolution: fixed
Status: closedreopened
Version: SVN1.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 7 years ago by Alex Gaynor

Resolution: fixed
Status: reopenedclosed

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