Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#6941 closed (fixed)

Session key reuse creates minor security flaw.

Reported by: jb0t Owned by: mtredinnick
Component: contrib.sessions Version: master
Severity: Keywords: session, session key, duplicate
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

When you store data in a user session, then logout and login (or simply just login again) you end up reusing the existing session key saved in the browser cookie. This exposes any user who can authenticate to session data that does not belong to them. Public terminal scenario would be most likely cause for concern.

Attachments (4)

6941_notests.diff (1.7 KB) - added by axiak 6 years ago.
An initial patch. No tests yet.
clear_session_on_logout_and_login.diff (1.3 KB) - added by mrts 6 years ago.
A simpler patch that depends on #7515
clear_session_on_logout_and_login.2.diff (1.3 KB) - added by mrts 6 years ago.
Patch updated to changes to #7515
clear_session_on_logout_and_login2.diff (1.5 KB) - added by mrts 6 years ago.
Minor improvement to logout().

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by axiak

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

Collin, jb0t, and I were talking in IRC about this...I think we agreed that the desired behavior would be that by default:

  1. If a user logs out, the session is destroyed.
  2. If a user is logging in and there is presently a different user in the session, a new session is created.

Is there anything that is not fixed with the above two statements?

Changed 6 years ago by axiak

An initial patch. No tests yet.

comment:2 Changed 6 years ago by axiak

  • Has patch set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

I think some form of this is necessary. I know that some functionality is changing, so a core dev will probably have to weigh in on this one.

comment:3 Changed 6 years ago by jacob

  • Summary changed from Session key reuse creates security flaw. to Session key reuse creates minor security flaw.

My thoughts, from a quick (lightly-edited) chat on #django-dev:

[4:15pm] axiak: basically if a person logs in as a different user the session key is unchanged
[4:15pm] axiak: so if there any session-specific data other than "who is logged in", it doesn't go away
[4:16pm] jacobkm: yeah, that's annoying, but hardly a security problem. session data isn't in any way secure, anyway.
[4:16pm] jacobkm: Calling it a security problem is complaining that someone who breaks into your unlocked, doorless house can steal things.
[4:16pm] axiak: okay...well it seems there are great pains to detect tampering with session data...it's kind of inconsistent to leave this hole there
[4:17pm] jacobkm: Also, there's the question of weather the session is tied to the browser or to the user -- we're a bit muddled there currently.
[4:17pm] axiak: yeah, I noticed when writing the patch
[4:17pm] jacobkm: I think your patch is probably correct 'cause we should have anon. sessions anyway, but I'd hardly call it a security problem.

IOW, this should go in, but I don't think it's a security problem per se (hence the slight change to the title)

comment:4 follow-up: Changed 6 years ago by jb0t

First, to be clear this has nothing to do with session high jacking.

In my mind, anytime what I would deem 'normal function' creates a scenario where one user has there session data muddled with another session that is not theirs, it is a security issue.

While the information may or may not be useless, the fact that it was not created by the subsequent user who authenticated and then resumed a session that they did not start is a problem. In response to who owns a session, it follows a simple order. Anon user to authenticated user to logout to re authenticated same user and resumed session OR authenticated new user and new session. So ownership first belongs to the browser, and THEN belongs to a user.

The app I am building will be running in a kiosk scenario where numerous people will walk up to the same machine and browser and login in and out. As it stands, all of them will be sharing the same session.

I don't see how this is just 'annoying'. Also, it's not at all an unlocked door. If it was, it should be called session_knob and not session_key.. and function exactly as a key. Can a key be copied easily, yes. But the house has a door, and it is certainly locked.

I do concede that this is arguably an application specific problem. But if your site is popular, and people login from, say a library, you would be vulnerable to the same problems if you stored session data that was specific to the user logged in at the time the data was stored in the session. Acknowledging that this situation exists means...

  1. Documenting the behavior so it's known. Then requiring application authors to deal with it, hopefully with some guidance so that they do not bastardize the framework in the process.
  1. Patch current code to clear sessions on login when the resumed session was created by another user.
  1. Create an option in settings to disable session resuming.

I can understand why someone might think this is trivial if they have not encountered the problem and start going over in their mind a scenario where it would cause a problem. So I will describe mine. 3 models are involved, auth_user, organization, and user_organization. A user logs in, and we check to see if they belong to multiple organizations (user_organization). If they do, they are presented with the option to select which one they would like to work with. That selection is then stored in the session and used multiple times per page request. As is, I must re validate that the user actually belongs to that organization. Not re validating is half the reason for putting it in the session in the first place. Anyhow, next user logs in, and they have in session the selected organization which they may or may not belong to. Firstly, they didn't get to choose because it thinks they already have chosen, secondly it starts failing checks because when/if they don't belong to the organization. It just cascades in to a situation where a lot of unnecessary validation is needed simply because I cannot rely on the data in the session. Not only as fresh (which is a common issue), but even valid at all.

comment:5 in reply to: ↑ 4 Changed 6 years ago by axiak

Replying to jb0t:

[...]
I do concede that this is arguably an application specific problem. But if your site is popular, and people login from, say a library, you would be vulnerable to the same problems if you stored session data that was specific to the user logged in at the time the data was stored in the session. Acknowledging that this situation exists means...

Hey jb0t,

I have a quick thought here. Despite earlier confusion, Django *does* make a distinction between individual users and browser sessions. The user is called request.user while the session is called request.session. If you have any data that you want to specifically attach to *both*, then you need to attach it to both. To illustrate that, here's some quick pseudocode:

if request.session['organization_userid'] == request.user.id:
    organization = request.session.get('organization')
else:
    organization = None #: Set a new organization.

# To set an organization...
request.session['organization_userid'] = request.user.id
request.session['organization'] = chosen_organization

I *do* think that it'd be nice to have the session changed on the login by default, but Django currently is "correct" in the fact that the user and the session are what they say they are.

comment:6 Changed 6 years ago by ubernostrum

Given the very long history of sites with warnings about using them from public/shared terminals due to the way a cookie can persist past a logout, I agree with Jacob that this really isn't a security flaw; I do think it's worth dealing with this, though, but I'm not certain what the best way forward is. Should it simply delete the session on logout?

comment:7 Changed 6 years ago by anonymous

For the time being I am doing this..

http://www.djangosnippets.org/snippets/681/

comment:8 Changed 6 years ago by cgrady

Replying to ubernostrum:

I'm not certain what the best way forward is. Should it simply delete the session on logout?

I think it would also have to delete any existing session data when going from logged in user -> logged in user as well (anonymous -> logged in could remain the same though)

comment:9 Changed 6 years ago by jb0t

One thing I noticed on logout clearing the session data the way I am doing it (maybe theres a better way) is that any 'messages' are lost. Sometimes I like to redirect to login or logout with a message.

Changed 6 years ago by mrts

A simpler patch that depends on #7515

comment:10 Changed 6 years ago by anonymous

  • Needs documentation set

comment:11 Changed 6 years ago by mrts

  • milestone set to 1.0

This should be in scope for 1.0.

comment:12 Changed 6 years ago by Itai Shirav <ishirav@…>

Please fix this! Whether or not it's a security flaw doesn't matter. It can cause weird bugs that are very difficult to duplicate or understand. For example, in my application I have filters that control what the user wants to see. The selected filter is stored in the session. After switching users, the application tries to use the first user's filter in order to filter the second user's data - not good!

Changed 6 years ago by mrts

Patch updated to changes to #7515

Changed 6 years ago by mrts

Minor improvement to logout().

comment:13 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

comment:14 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [8343]) Fixed #6941 -- When logging a user out, or when logging in with an existing
session and a different user id to the current session owner, flush the session
data to avoid leakage. Logging in and moving from an anonymous user to a
validated user still keeps existing session data.

Backwards incompatible if you were assuming sessions persisted past logout.

comment:15 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.