Opened 6 years ago

Last modified 4 months ago

#15619 new Bug

Logout link should be protected

Reported by: Alexey Boriskin Owned by:
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: vlastimil.zima@…, raymond.penners@…, csrf.django@…, eromijn@…, unai@…, vlastimil@…, Gwildor Sok Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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)

ticket15619.diff (3.7 KB) - added by Ash Christopher 5 years ago.
New patch submitted with a bit more sane method of attack.
ticket15619-2.diff (16.0 KB) - added by Ash Christopher 5 years ago.
Updated code, fixed regression tests and added new regression tests.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 6 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: wontfix
Status: newclosed

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.

comment:2 Changed 6 years ago by Alexey Boriskin

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.

comment:3 Changed 6 years ago by Luke Plant

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 Changed 6 years ago by Paul McMillan

milestone: 1.4
Resolution: wontfix
Status: closedreopened
Summary: Logout link should be a formLogout link should be protected
Triage Stage: UnreviewedDesign 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 Changed 5 years ago by Luke Plant

Type: Bug

comment:6 Changed 5 years ago by Luke Plant

Severity: Normal

comment:7 Changed 5 years ago by Chris Beaven

Easy pickings: unset
Triage Stage: Design decision neededAccepted

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:8 Changed 5 years ago by Jannis Leidel

Sounds like a plan +1

comment:9 Changed 5 years ago by Luke Plant

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 Changed 5 years ago by Ash Christopher

Owner: changed from nobody to Ash Christopher
Status: reopenednew
UI/UX: unset

comment:11 Changed 5 years ago by Ash Christopher

Status: newassigned

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.

Last edited 5 years ago by Ash Christopher (previous) (diff)

comment:12 Changed 5 years ago by Ash Christopher

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?

Last edited 5 years ago by Ash Christopher (previous) (diff)

comment:13 Changed 5 years ago by Tobias McNulty

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 Changed 5 years ago by Paul McMillan

Tobias seems to have hit it on the head. That sounds like the right solution to me too.

Changed 5 years ago by Ash Christopher

Attachment: ticket15619.diff added

New patch submitted with a bit more sane method of attack.

comment:15 Changed 5 years ago by Paul McMillan

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 :)

Last edited 5 years ago by Paul McMillan (previous) (diff)

comment:16 Changed 5 years ago by Ash Christopher

Regression tests fail using this patch. Attempting to fix regression tests.

Changed 5 years ago by Ash Christopher

Attachment: ticket15619-2.diff added

Updated code, fixed regression tests and added new regression tests.

comment:17 Changed 5 years ago by Ash Christopher

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:18 Changed 5 years ago by Ash Christopher

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:19 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:20 Changed 5 years ago by anonymous

Patch needs improvement: set

setting patch needs improvement per comment 18

comment:21 Changed 4 years ago by Ash Christopher

Beginning work on this patch again.

comment:22 Changed 4 years ago by Ash Christopher

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.

Version 0, edited 4 years ago by Ash Christopher (next)

comment:23 Changed 4 years ago by Aymeric Augustin

#7989 was a duplicate.

comment:24 Changed 4 years ago by Vlastimil Zíma

Cc: vlastimil.zima@… added

comment:25 in reply to:  3 Changed 4 years ago by Vlastimil Zíma

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.

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:26 Changed 4 years ago by Aymeric Augustin

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.

comment:27 in reply to:  26 ; Changed 4 years ago by Vlastimil Zíma

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 Changed 4 years ago by raymond.penners@…

Cc: raymond.penners@… added

comment:29 in reply to:  27 ; Changed 4 years ago by Luke Plant

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 in reply to:  29 Changed 4 years ago by Vlastimil Zíma

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 Changed 3 years ago by csrf.django@…

Cc: csrf.django@… added

comment:32 Changed 3 years ago by Erik Romijn

Cc: eromijn@… added
Component: contrib.admincontrib.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 Changed 3 years ago by Krzysztof Jurewicz

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:34 Changed 3 years ago by Unai Zalakain

Cc: unai@… added

Patch LGTM

comment:35 Changed 3 years ago by Vlastimil Zíma

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.

https://github.com/django/django/pull/1963

comment:36 Changed 3 years ago by Vlastimil Zíma

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 Changed 3 years ago by Vlastimil Zíma

Cc: vlastimil@… added

comment:38 Changed 3 years ago by loic84

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.

Last edited 3 years ago by loic84 (previous) (diff)

comment:39 in reply to:  38 Changed 3 years ago by Vlastimil Zíma

Replying to loic84:

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.

We could also keep the form and style the button as a button.

comment:40 Changed 3 years ago by Gwildor Sok

Cc: Gwildor Sok added

comment:42 Changed 4 months ago by Tim Graham

Owner: Ash Christopher deleted
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top