Opened 13 years ago

Closed 2 years ago

Last modified 7 months ago

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

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

Download all attachments as: .zip

Change History (53)

comment:1 by Russell Keith-Magee, 13 years ago

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 by Alexey Boriskin, 13 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.

comment:3 by Luke Plant, 13 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 Paul McMillan, 13 years ago

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 by Luke Plant, 13 years ago

Type: Bug

comment:6 by Luke Plant, 13 years ago

Severity: Normal

comment:7 by Chris Beaven, 13 years ago

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 by Jannis Leidel, 13 years ago

Sounds like a plan +1

comment:9 by Luke Plant, 13 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 Ash Christopher, 13 years ago

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

comment:11 by Ash Christopher, 13 years ago

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 13 years ago by Ash Christopher (previous) (diff)

comment:12 by Ash Christopher, 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?

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

comment:13 by Tobias McNulty, 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 Paul McMillan, 13 years ago

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

by Ash Christopher, 13 years ago

Attachment: ticket15619.diff added

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

comment:15 by Paul McMillan, 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 :)

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

comment:16 by Ash Christopher, 13 years ago

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

by Ash Christopher, 13 years ago

Attachment: ticket15619-2.diff added

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

comment:17 by Ash Christopher, 13 years ago

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

comment:18 by Ash Christopher, 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:19 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:20 by anonymous, 12 years ago

Patch needs improvement: set

setting patch needs improvement per comment 18

comment:21 by Ash Christopher, 12 years ago

Beginning work on this patch again.

comment:22 by Ash Christopher, 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.

Once this is complete, we can move ahead with the solution as outlined in the rest of this ticket.

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

comment:23 by Aymeric Augustin, 12 years ago

#7989 was a duplicate.

comment:24 by Vlastimil Zíma, 11 years ago

Cc: vlastimil.zima@… added

in reply to:  3 comment:25 by Vlastimil Zíma, 11 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.

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

comment:26 by Aymeric Augustin, 11 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.

in reply to:  26 ; comment:27 by Vlastimil Zíma, 11 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 raymond.penners@…, 11 years ago

Cc: raymond.penners@… added

in reply to:  27 ; comment:29 by Luke Plant, 11 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.

in reply to:  29 comment:30 by Vlastimil Zíma, 11 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 csrf.django@…, 11 years ago

Cc: csrf.django@… added

comment:32 by Sasha Romijn, 11 years ago

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 by Krzysztof Jurewicz, 10 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:34 by Unai Zalakain, 10 years ago

Cc: unai@… added

Patch LGTM

comment:35 by Vlastimil Zíma, 10 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.

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

comment:36 by Vlastimil Zíma, 10 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 Vlastimil Zíma, 10 years ago

Cc: vlastimil@… added

comment:38 by loic84, 10 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.

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

in reply to:  38 comment:39 by Vlastimil Zíma, 10 years ago

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 by Gwildor Sok, 10 years ago

Cc: Gwildor Sok added

comment:42 by Tim Graham, 8 years ago

Owner: Ash Christopher removed
Status: assignednew

comment:43 by Ramiro Morales, 6 years ago

Owner: set to Ramiro Morales
Status: newassigned

comment:44 by René Fleschenberg, 4 years ago

Has patch: unset
Needs documentation: set
Owner: changed from Ramiro Morales to René Fleschenberg
Patch needs improvement: unset

comment:45 by René Fleschenberg, 4 years ago

Has patch: set

As a first step, I suggest deprecating logout via GET.

PR: https://github.com/django/django/pull/12504

comment:46 by GitHub <noreply@…>, 2 years ago

In 94d8ed55:

Refs #15619 -- Logged out with POST requests in admin.

comment:47 by Mariusz Felisiak, 2 years ago

Needs documentation: unset
Triage Stage: AcceptedReady for checkin
Type: BugCleanup/optimization

comment:48 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In eb07b5be:

Fixed #15619 -- Deprecated log out via GET requests.

Thanks Florian Apolloner for the implementation idea.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@…>

comment:49 by Michael, 20 months 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

comment:50 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

In 9a01311d:

Refs #15619 -- Removed support for logging out via GET requests.

Per deprecation timeline.

comment:51 by GitHub <noreply@…>, 7 months ago

In e2a3a896:

Refs #15619 -- Removed deprecated annotation about logging out via GET requests.

Follow up to 6c57c08ae52f86df843fccb5a3c1c6c45a10a26f.

Note: See TracTickets for help on using tickets.
Back to Top