Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#11457 closed (fixed)

Login Redirect Security Check Overly Broad

Reported by: dnagoda@… Owned by:
Component: contrib.auth Version: 1.0
Severity: Keywords: auth login redirect next
Cc: dnagoda@…, django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I believe that the fix implemented for bug #5227 is overly broad. If the original URL contains a GET parameter that is itself a URL, then the resultant 'next' parameter created during the redirect to the login screen will look like this:

/original/path/?param=http%3A//example.com/

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

If the desire to to protect against redirect URLs that start with '' (scheme-less URL) then I think it's better to be explicit and change line 24 of django.contrib.auth.views to this:

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

Attachments (5)

11457.patch (2.9 KB) - added by Bruno Renié 7 years ago.
The suggested fix in the ticket summary is not enough, see my comment next.
11457-2.patch (3.6 KB) - added by Bruno Renié 7 years ago.
Pach that doesn't only block http
11457-3.patch (3.7 KB) - added by Bruno Renié 7 years ago.
Are we there?
11457-4.patch (3.9 KB) - added by Bruno Renié 7 years ago.
Added some readability thank to Jacob's comments
11457-5.patch (3.9 KB) - added by Bruno Renié 7 years ago.
cleanup

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by Jille Timmermans

Cc: django@… added
Keywords: redirect next added

I have got the same problem.
You can use .replace('/', '%2F') on the parameter you are passing as a workaround.

comment:2 Changed 7 years ago by Russell Keith-Magee

Has patch: set
milestone: 1.2
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by Luke Plant

See also #12534. I don't know why the restriction on spaces is there - does anyone?

Changed 7 years ago by Bruno Renié

Attachment: 11457.patch added

The suggested fix in the ticket summary is not enough, see my comment next.

comment:4 Changed 7 years ago by Bruno Renié

Needs tests: unset
Triage Stage: AcceptedReady for checkin

The attached patch fixes it, and adds a test. The problem is that the redirect_to field is unquoted, so just checking "if ':' in redirect_to" is not enough.

The patch redirects to settings.LOGIN_REDIRECT_URL if redirect_to starts with "" or "http://".

Like Luke, no idea of why there is a restriction on spaces.

comment:5 Changed 7 years ago by Quis

Owner: changed from nobody to Quis
Patch needs improvement: set
Status: newassigned

Shouldn't you check for other protocols? https:// can be used exactly the same as http..

comment:6 Changed 7 years ago by Quis

Owner: Quis deleted
Status: assignednew

I didn't want to take ownership.. (/deassign)

Changed 7 years ago by Bruno Renié

Attachment: 11457-2.patch added

Pach that doesn't only block http

comment:7 Changed 7 years ago by Bruno Renié

Patch needs improvement: unset

Nice catch Quis, just updated the patch.

comment:8 Changed 7 years ago by Quis

Shouldn't the not_secure regex be: '[:]+:' instead of '[/]+:' (/ -> :) ?

(FTR: I am Jille Timmermans; I needed to register due to the spamfilter)

comment:9 Changed 7 years ago by Bruno Renié

With the regex you suggest, the tests doesn't pass.

If you call /login/?next=http%3Aexample.com, the redirection should be blocked (this URL matches the not_secure pattern). If you call /login/?next=/view/?param=http%3Aexample.com, then it is fine to redirect to /view/?param=http%3Aexample.com (there is a '/' before http://example.com that makes it a safe URL to redirect to).

comment:10 Changed 7 years ago by Quis

You are right. However / is not the right separator: http:// should be allowed after a ? (eg: /login/?next=somepage?url=http://example.com)

Changed 7 years ago by Bruno Renié

Attachment: 11457-3.patch added

Are we there?

comment:11 Changed 7 years ago by Bruno Renié

Thanks Jille, updated the patch with a new regex that still passes the tests :)

comment:12 Changed 7 years ago by Bruno Renié

Triage Stage: Ready for checkinAccepted

Moving back to accepted, it needs to be reviewed by a core dev before being ready for checkin.

Changed 7 years ago by Bruno Renié

Attachment: 11457-4.patch added

Added some readability thank to Jacob's comments

Changed 7 years ago by Bruno Renié

Attachment: 11457-5.patch added

cleanup

comment:13 Changed 7 years ago by Jacob

Resolution: fixed
Status: newclosed

(In [12635]) Fixed #11457: tightened the security check for "next" redirects after logins.

The new behavior still disallows redirects to off-site URLs, but now allows
redirects of the form /some/other/view?foo=http://....

Thanks to brutasse.

comment:14 Changed 7 years ago by Jacob

(In [12636]) [1.1.X] Fixed #11457: tightened the security check for "next" redirects after logins.

The new behavior still disallows redirects to off-site URLs, but now allows
redirects of the form /some/other/view?foo=http://....

Thanks to brutasse.

Backport of [12635] from trunk.

comment:15 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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