Code

Opened 5 years ago

Closed 4 years ago

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

Download all attachments as: .zip

Change History (20)

comment:1 Changed 4 years ago by Jille Timmermans

  • Cc django@… added
  • Keywords redirect next added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 4 years ago by russellm

  • Has patch set
  • milestone set to 1.2
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 4 years ago by lukeplant

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

Changed 4 years ago by brutasse

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

comment:4 Changed 4 years ago by brutasse

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready 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 4 years ago by Quis

  • Owner changed from nobody to Quis
  • Patch needs improvement set
  • Status changed from new to assigned

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

comment:6 Changed 4 years ago by Quis

  • Owner Quis deleted
  • Status changed from assigned to new

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

Changed 4 years ago by brutasse

Pach that doesn't only block http

comment:7 Changed 4 years ago by brutasse

  • Patch needs improvement unset

Nice catch Quis, just updated the patch.

comment:8 Changed 4 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 4 years ago by brutasse

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

Are we there?

comment:11 Changed 4 years ago by brutasse

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

comment:12 Changed 4 years ago by brutasse

  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 4 years ago by brutasse

Added some readability thank to Jacob's comments

Changed 4 years ago by brutasse

cleanup

comment:13 Changed 4 years ago by jacob

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

(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 4 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 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.