#15619 closed Cleanup/optimization (fixed)
Logout link should be protected
| Reported by: | Alexey Boriskin | Owned by: | René Fleschenberg |
|---|---|---|---|
| Component: | contrib.auth | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | vlastimil.zima@…, raymond.penners@…, csrf.django@…, eromijn@…, unai@…, vlastimil@…, Gwildor Sok | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
There is a logout link in admin app. It is link, not a form. Therefore it is not CSRF-protected.
Probably it is not so important to protect logout from CSRF attack, because this fact cannot be used to do anything harmful. So this is just a request for purity.
Another reason is that GET request should never change invernal state of the system.
Attachments (2)
Change History (53)
comment:1 by , 14 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
comment:2 by , 14 years ago
Logout link is idempotent, right. But GET /logout/ make a job other than retrieval. It destroys a session. Deletes something. Imagine a bot which crawl every link on the site. After visiting /logout/ the state of the system will change and visiting every link will redirect to login form.
So, I think, in ideal world the /logout/ link should be visited only with DELETE requests. Browsers doesn't allow DELETE (without XHR), so we have two options to emulate it: with GET and with POST. For now it's GET. But this GET is not really "safe", it deletes session.
follow-up: 25 comment:3 by , 14 years ago
The point Russell was making was that 'SHOULD NOT' is not the same as 'MUST NOT'. In practice, while being logged out by a 3rd party might be a nuisance, in general the attackers will gain extremely little except ill-will, and therefore there is little motivation to exploit this, and fairly trivial consequences if they do.
comment:4 by , 14 years ago
| milestone: | → 1.4 |
|---|---|
| Resolution: | wontfix |
| Status: | closed → reopened |
| Summary: | Logout link should be a form → Logout link should be protected |
| Triage Stage: | Unreviewed → Design decision needed |
On the recommendation of Alex Gaynor, I'm reopening this ticket.
The issue is that this presents a really tempting avenue for DoS type attacks. The attack (which I have, through great force of will, refrained from illustrating in this post) is to simply embed the non-side-effect-free url as an image. The link obviously does not display a picture, but the browser does retrieve the content, forcing the user to log out. This makes removal of offensive content particularly obnoxious for administrators.
Fixing this could involve requiring a form, or (since using a link to log out is convenient) a nonce of some sort. Some forums implement the functionality with a pass-through page which submits a form via javascript.
comment:5 by , 14 years ago
| Type: | → Bug |
|---|
comment:6 by , 14 years ago
| Severity: | → Normal |
|---|
comment:7 by , 14 years ago
| Easy pickings: | unset |
|---|---|
| Triage Stage: | Design decision needed → Accepted |
Sure, let's have the admin use a logout view which logs out if request.method == 'POST' otherwise shows an intermediary confirmation page.
Site.login wraps the django.contrib.auth.views logout view and changing that would be backwards incompatible, so it'll have to be a new view (and it may as well live in auth so it can be used in other situations too).
comment:9 by , 14 years ago
In the admin we can also have some jQuery (or other javascript) code that will change the logout link so that it does a POST to the logout view by submitting a (dynamically generated) POST form. That would be better than a pass through page because it requires just one HTTP request.
comment:10 by , 13 years ago
| Owner: | changed from to |
|---|---|
| Status: | reopened → new |
| UI/UX: | unset |
comment:11 by , 13 years ago
| Status: | new → assigned |
|---|
Have this more or less working however, need a csrf token when doing the logout in javascript. Not sure the best way to go about this. Make a call to the url to get the csrf back then use that to submit? Not sure - seems like a wonky idea.
EDIT:
Easy enough - added {{ csrf_token }} to data being posted.
comment:12 by , 13 years ago
| Has patch: | set |
|---|
Added patch but still needs work - looking for feedback.
https://code.djangoproject.com/attachment/ticket/15619/ticket15619.diff
The logout confirmation screen is no longer used - is it something we actually need? If so, any suggestions on how to keep it other than replacing the <html /> element with the payload from the AJAX request? Or is that a valid way to do it?
comment:13 by , 13 years ago
1) Why POST the form over AJAX? Can't you just put a logout form on all admin pages that the browser submits when the logout link is clicked?
2) The logout link should still point to the logout confirmation page unless the click event is co-opted by JavaScript and converted into a POST. This way the confirmation page will still come into play if someone has JavaScript disabled.
comment:14 by , 13 years ago
Tobias seems to have hit it on the head. That sounds like the right solution to me too.
by , 13 years ago
| Attachment: | ticket15619.diff added |
|---|
New patch submitted with a bit more sane method of attack.
comment:15 by , 13 years ago
| Needs documentation: | set |
|---|---|
| Needs tests: | set |
| Patch needs improvement: | set |
It's more usual to say
if request.method == "POST"
The "are you sure you want to log out" isn't translated.
It also needs tests and documentation.
Otherwise, the method looks pretty good to me. I'd like someone who's more familiar with the admin coding conventions than I to make the final call, but it's about ready. Thanks for keeping at this :)
comment:16 by , 13 years ago
Regression tests fail using this patch. Attempting to fix regression tests.
by , 13 years ago
| Attachment: | ticket15619-2.diff added |
|---|
Updated code, fixed regression tests and added new regression tests.
comment:17 by , 13 years ago
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
| Patch needs improvement: | unset |
comment:18 by , 13 years ago
As per julienphalip's feedback on irc:
"it'd be good to test the actual login status after using both the POST and GET methods. It seems the patch only looks at what template is being used."
Suggested using:
self.assertTrue(SESSION_KEY not in self.client.session)
comment:20 by , 13 years ago
| Patch needs improvement: | set |
|---|
setting patch needs improvement per comment 18
comment:22 by , 12 years ago
Talking to julienphalip on #django-dev - we are going to look at getting ModelAdmin.media() to return only the js files needed for a given view. This may require changing ModelAdmin.media() to be a method that takes arguments, rather than staying as a property.
comment:24 by , 12 years ago
| Cc: | added |
|---|
comment:25 by , 12 years ago
Replying to lukeplant:
The point Russell was making was that 'SHOULD NOT' is not the same as 'MUST NOT'. In practice, while being logged out by a 3rd party might be a nuisance, in general the attackers will gain extremely little except ill-will, and therefore there is little motivation to exploit this, and fairly trivial consequences if they do.
Really?
[[Image(https://code.djangoproject.com/logout)]]
EDIT(aaugustin): added quotes around the image wiki markup.
follow-up: 27 comment:26 by , 12 years ago
Congratulations, you've proved you like wasting our time.
Don't be surprised if your comments are ignored from now on.
By the way, this isn't even an proof-of-concept against Django, it's against Trac.
follow-up: 29 comment:27 by , 12 years ago
Replying to aaugustin:
Congratulations, you've proved you like wasting our time.
Don't be surprised if your comments are ignored from now on.
By the way, this isn't even an proof-of-concept against Django, it's against Trac.
It is the same problem in Django as is in Trac. It would be very easy to add a lot fake images to whatever site powered by Django, some are listed at Django homepage. Or Django Project admin itself :)
[[Image(https://www.djangoproject.com/admin/logout/)]]
comment:28 by , 12 years ago
| Cc: | added |
|---|
follow-up: 30 comment:29 by , 12 years ago
Replying to vzima:
It is the same problem in Django as is in Trac. It would be very easy to add a lot fake images to whatever site powered by Django, some are listed at Django homepage. Or Django Project admin itself :)
Please stop arguing with us when we already agree with you. See comment number 4, which is after mine.
comment:30 by , 12 years ago
Replying to lukeplant:
Replying to vzima:
It is the same problem in Django as is in Trac. It would be very easy to add a lot fake images to whatever site powered by Django, some are listed at Django homepage. Or Django Project admin itself :)
Please stop arguing with us when we already agree with you. See comment number 4, which is after mine.
My main point is that this ticket should be closed as soon as possible because the bug has security consequences. The bug is opened 2 years and it does not seem its patch will be included into 1.5 either. The last patch probably requires no update except comment:18 and then it got stuck.
Anyway, based on last patch from ashchristopher I created a github branch https://github.com/vzima/django/tree/15619-protected-logout with updated patch which considers comment:18.
Also I moved the base code from admin logout to auth logout so logouts are protected also outside of admin application.
Feedback welcome, so we can finally close this issue.
comment:31 by , 12 years ago
| Cc: | added |
|---|
comment:32 by , 11 years ago
| Cc: | added |
|---|---|
| Component: | contrib.admin → contrib.auth |
| Needs documentation: | set |
The patch no longer applies cleanly and an update for the contrib.auth documentation is not included. A change like this also belongs in the release notes, as it causes a backwards incompatibility.
comment:33 by , 11 years ago
| Needs documentation: | unset |
|---|---|
| Patch needs improvement: | unset |
I’ve added the documentation and made a few changes to vzima’s patch: https://github.com/django/django/pull/1934
comment:35 by , 11 years ago
For a few days I have the branch on work, but KJ was a bit faster :) I provide my pull as well, I found there few things differ, though I replaced logout link with form as well.
comment:36 by , 11 years ago
| Patch needs improvement: | set |
|---|
I'd rather note this here, in case it gets lost on github: KJ didn't fix the logout links in password change templates.
comment:37 by , 11 years ago
| Cc: | added |
|---|
follow-up: 39 comment:38 by , 11 years ago
The input that masquerades as an anchor doesn't render all that well across various browsers, also it'll break for people with custom CSS.
I would replace it with <a href="/admin/logout/" id="logout-link"> and a jQuery click handler along those lines:
$('#logout-link').click(function() { $(this).parents('form').submit(); return false; })
People without JS can still logout because the href points to the intermediary page.
comment:39 by , 11 years ago
Replying to loic84:
The
inputthat masquerades as an anchor doesn't render all that well across various browsers, also it'll break for people with custom CSS.
We could also keep the form and style the button as a button.
comment:40 by , 11 years ago
| Cc: | added |
|---|
comment:41 by , 10 years ago
comment:42 by , 9 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:43 by , 7 years ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:44 by , 5 years ago
| Has patch: | unset |
|---|---|
| Needs documentation: | set |
| Owner: | changed from to |
| Patch needs improvement: | unset |
comment:47 by , 3 years ago
| Needs documentation: | unset |
|---|---|
| Triage Stage: | Accepted → Ready for checkin |
| Type: | Bug → Cleanup/optimization |
comment:49 by , 2 years ago
Something that maybe more likely than XSS logout attack... is if some not tech savy user clicks back, or navigates to the logged out url, and sees the message "You are logged out", and thinks they are logged out now, and its safe to close the browser, but actually since Logout only happens via POST now, they are actually still logged in. Yes one can mitagate the issue with some javascript on the logged out page, but maybe the average developer might miss this point when reading: https://docs.djangoproject.com/en/dev/releases/4.1/#log-out-via-get
The HTTP spec says (9.1.1) that GET requests "SHOULD NOT have the significance of taking an action other than retrieval", and "ought to be considered 'safe'". It also says (9.1.2) that GET has the property of idempotence. A logout link is idempotent. Therefore, we are HTTP compliant.
As for CSRF; I fail to see why this is relevant. Given that there is no incoming data, and no data processing, I cannot see an CSRF weakness, even in-principle.