Login Redirect Security Check Overly Broad

[dnagoda@…]

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:

[Jille Timmermans]

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

[Luke Plant]

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

[Bruno Renié]

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

[Bruno Renié]

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.

[Quis]

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

[Quis]

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

[Bruno Renié]

Pach that doesn't only block http

[Bruno Renié]

Nice catch Quis, just updated the patch.

[Quis]

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

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

[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).

[Quis]

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

[Bruno Renié]

Are we there?

[Bruno Renié]

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

[Bruno Renié]

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

[Bruno Renié]

Added some readability thank to Jacob's comments

[Bruno Renié]

cleanup

[Jacob]

(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.

[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.

[Jacob]

Milestone 1.2 deleted