Opened 15 years ago

Closed 14 years ago

Last modified 12 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: no UI/UX: no

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é 14 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é 14 years ago.
Pach that doesn't only block http
11457-3.patch (3.7 KB ) - added by Bruno Renié 14 years ago.
Are we there?
11457-4.patch (3.9 KB ) - added by Bruno Renié 14 years ago.
Added some readability thank to Jacob's comments
11457-5.patch (3.9 KB ) - added by Bruno Renié 14 years ago.
cleanup

Download all attachments as: .zip

Change History (20)

comment:1 by Jille Timmermans, 14 years ago

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 by Russell Keith-Magee, 14 years ago

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

comment:3 by Luke Plant, 14 years ago

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

by Bruno Renié, 14 years ago

Attachment: 11457.patch added

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

comment:4 by Bruno Renié, 14 years ago

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 by Quis, 14 years ago

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 by Quis, 14 years ago

Owner: Quis removed
Status: assignednew

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

by Bruno Renié, 14 years ago

Attachment: 11457-2.patch added

Pach that doesn't only block http

comment:7 by Bruno Renié, 14 years ago

Patch needs improvement: unset

Nice catch Quis, just updated the patch.

comment:8 by Quis, 14 years ago

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

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

comment:9 by Bruno Renié, 14 years ago

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 by Quis, 14 years ago

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

by Bruno Renié, 14 years ago

Attachment: 11457-3.patch added

Are we there?

comment:11 by Bruno Renié, 14 years ago

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

comment:12 by Bruno Renié, 14 years ago

Triage Stage: Ready for checkinAccepted

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

by Bruno Renié, 14 years ago

Attachment: 11457-4.patch added

Added some readability thank to Jacob's comments

by Bruno Renié, 14 years ago

Attachment: 11457-5.patch added

cleanup

comment:13 by Jacob, 14 years ago

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 by Jacob, 14 years ago

(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 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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