#11457 closed (fixed)
Login Redirect Security Check Overly Broad
Reported by: | 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)
Change History (20)
comment:1 by , 15 years ago
Cc: | added |
---|---|
Keywords: | redirect next added |
comment:2 by , 15 years ago
Has patch: | set |
---|---|
milestone: | → 1.2 |
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 15 years ago
See also #12534. I don't know why the restriction on spaces is there - does anyone?
by , 15 years ago
Attachment: | 11457.patch added |
---|
The suggested fix in the ticket summary is not enough, see my comment next.
comment:4 by , 15 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → 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 by , 15 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | set |
Status: | new → assigned |
Shouldn't you check for other protocols? https:// can be used exactly the same as http..
comment:6 by , 15 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I didn't want to take ownership.. (/deassign)
comment:8 by , 15 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 , 15 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 , 15 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)
comment:11 by , 15 years ago
Thanks Jille, updated the patch with a new regex that still passes the tests :)
comment:12 by , 15 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Moving back to accepted, it needs to be reviewed by a core dev before being ready for checkin.
comment:13 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I have got the same problem.
You can use .replace('/', '%2F') on the parameter you are passing as a workaround.