Code

Opened 3 years ago

Last modified 3 months ago

#15619 assigned Bug

Logout link should be protected

Reported by: void Owned by: ashchristopher
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: vlastimil.zima@…, raymond.penners@…, csrf.django@…, eromijn@…, unai@…, vlastimil@…, Gwildor 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 ashchristopher 3 years ago.
New patch submitted with a bit more sane method of attack.
ticket15619-2.diff (16.0 KB) - added by ashchristopher 3 years ago.
Updated code, fixed regression tests and added new regression tests.

Download all attachments as: .zip

Change History (42)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

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 3 years ago by void

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 follow-up: Changed 3 years ago by 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.

comment:4 Changed 3 years ago by PaulM

  • milestone set to 1.4
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Summary changed from Logout link should be a form to Logout link should be protected
  • Triage Stage changed from Unreviewed to 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 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:6 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:7 Changed 3 years ago by SmileyChris

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to 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:8 Changed 3 years ago by jezdez

Sounds like a plan +1

comment:9 Changed 3 years ago by lukeplant

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 3 years ago by ashchristopher

  • Owner changed from nobody to ashchristopher
  • Status changed from reopened to new
  • UI/UX unset

comment:11 Changed 3 years ago by ashchristopher

  • Status changed from new to 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.

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

comment:12 Changed 3 years ago by ashchristopher

  • 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 3 years ago by ashchristopher (previous) (diff)

comment:13 Changed 3 years ago by tobias

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 3 years ago by PaulM

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

Changed 3 years ago by ashchristopher

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

comment:15 Changed 3 years ago by PaulM

  • 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 3 years ago by PaulM (previous) (diff)

comment:16 Changed 3 years ago by ashchristopher

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

Changed 3 years ago by ashchristopher

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

comment:17 Changed 3 years ago by ashchristopher

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:18 Changed 3 years ago by ashchristopher

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 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:20 Changed 3 years ago by anonymous

  • Patch needs improvement set

setting patch needs improvement per comment 18

comment:21 Changed 20 months ago by ashchristopher

Beginning work on this patch again.

comment:22 Changed 20 months ago by ashchristopher

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 20 months ago by ashchristopher (next)

comment:23 Changed 19 months ago by aaugustin

#7989 was a duplicate.

comment:24 Changed 14 months ago by vzima

  • Cc vlastimil.zima@… added

comment:25 in reply to: ↑ 3 Changed 14 months ago by vzima

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 14 months ago by aaugustin (previous) (diff)

comment:26 follow-up: Changed 14 months ago by 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.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 14 months ago by vzima

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 14 months ago by raymond.penners@…

  • Cc raymond.penners@… added

comment:29 in reply to: ↑ 27 ; follow-up: Changed 14 months ago by 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.

comment:30 in reply to: ↑ 29 Changed 14 months ago by vzima

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 12 months ago by csrf.django@…

  • Cc csrf.django@… added

comment:32 Changed 10 months ago by erikr

  • Cc eromijn@… added
  • Component changed from contrib.admin to 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 Changed 5 months ago by KJ

  • 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 5 months ago by unaizalakain

  • Cc unai@… added

Patch LGTM

comment:35 Changed 5 months ago by vzima

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 5 months ago by vzima

  • 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 5 months ago by vzima

  • Cc vlastimil@… added

comment:38 follow-up: Changed 5 months 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 5 months ago by loic84 (previous) (diff)

comment:39 in reply to: ↑ 38 Changed 5 months ago by vzima

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 months ago by Gwildor

  • Cc Gwildor added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from ashchristopher to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.