#6941 closed (fixed)
Session key reuse creates minor security flaw.
Reported by: | Jay Hargis | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | contrib.sessions | Version: | dev |
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: | no | UI/UX: | no |
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)
Change History (19)
comment:1 by , 17 years ago
comment:2 by , 17 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → 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 by , 17 years ago
Summary: | Session key reuse creates security flaw. → 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)
follow-up: 5 comment:4 by , 17 years ago
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...
- 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.
- Patch current code to clear sessions on login when the resumed session was created by another user.
- 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 by , 17 years ago
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 by , 17 years ago
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:8 by , 17 years ago
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 by , 17 years ago
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.
by , 17 years ago
Attachment: | clear_session_on_logout_and_login.diff added |
---|
A simpler patch that depends on #7515
comment:10 by , 17 years ago
Needs documentation: | set |
---|
comment:12 by , 16 years ago
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!
by , 16 years ago
Attachment: | clear_session_on_logout_and_login.2.diff added |
---|
Patch updated to changes to #7515
by , 16 years ago
Attachment: | clear_session_on_logout_and_login2.diff added |
---|
Minor improvement to logout().
comment:13 by , 16 years ago
Owner: | changed from | to
---|
comment:14 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → 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.
Collin, jb0t, and I were talking in IRC about this...I think we agreed that the desired behavior would be that by default:
Is there anything that is not fixed with the above two statements?