Django

Code

Ticket #7515 (closed: fixed)

Opened 3 months ago

Last modified 4 weeks ago

Add .clear() and .destroy() to session objects

Reported by: mrts Assigned to: mtredinnick
Milestone: 1.0 beta Component: django.contrib.sessions
Version: SVN Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

Reliable session clearing is required to avoid fragile (another thread may delete a key and cause an exception in the loop given in the snippet) and cumbersome tricks like http://www.djangosnippets.org/snippets/681/ .

See also http://groups.google.com/group/django-developers/browse_thread/thread/fbcfa88c997d1bb3 .

Note that this doesn't reset the session key, only the data associated with the key. Other frameworks tend to reset the key as well. I believe this is redundant, but feel free to correct me. The reasoning behind that claim is as follows: 

session key -> sensitive data
clear session, but don't reset the key
session key -> no data
=> therefore no sensitive information leaked, as the key per se is public, only the data set behind it is private

Attachments

session_clear.diff (1.0 kB) - added by mrts on 06/20/08 13:25:31.
session_clear-with_docs.diff (1.5 kB) - added by anonymous on 06/20/08 13:36:31.
session_clear-with_docs2.diff (1.5 kB) - added by mrts on 06/20/08 13:46:56.
Rephrased docs as per suggestions on the IRC.
clear_and_destroy.diff (8.5 kB) - added by mrts on 07/30/08 08:28:34.
Exception-safe destroy() added to request.session, with tests and docs
clear_and_destroy2.diff (8.4 kB) - added by mrts on 07/31/08 12:25:01.
Remove redundant .save()s from tests and update to r8158

Change History

06/20/08 13:25:31 changed by mrts

  • attachment session_clear.diff added.

06/20/08 13:26:33 changed by jacob

  • status changed from new to assigned.
  • needs_better_patch changed.
  • needs_tests changed.
  • owner changed from nobody to jacob.
  • needs_docs changed.
  • stage changed from Unreviewed to Accepted.

06/20/08 13:27:02 changed by telenieko

  • needs_docs set to 1.

new method, new docs ;)

06/20/08 13:36:07 changed by mrts

The "proof" behind the claim should read as follows (where -> designates mapping and '=>' implication): 

session key X -> sensitive data for user A
clear session, but don't reset the key
session key X -> no data
reuse session key for user B
session key X -> sensitive data for user B
=> therefore no sensitive information leaked, as the key per se is public, only the data set behind it is private

Attaching docs as per Jacob's request.

06/20/08 13:36:31 changed by anonymous

  • attachment session_clear-with_docs.diff added.

06/20/08 13:41:50 changed by telenieko

  • needs_docs deleted.

06/20/08 13:46:56 changed by mrts

  • attachment session_clear-with_docs2.diff added.

Rephrased docs as per suggestions on the IRC.

06/21/08 06:08:20 changed by Simon Greenhill

  • stage changed from Accepted to Ready for checkin.

06/26/08 07:05:49 changed by mrts

  • stage changed from Ready for checkin to Design decision needed.

One could argue that not resetting the session key is not exception safe:

  • session key X -> sensitive data for user A
  • clear session, but don't reset the key
  • session changed, empty session needs to be cleared from db
  • db connection has died, exception, session.save() fails
  • old key remains in browser, will be reused for user B
  • db comes online again
  • user B visits site and gets access to user A's sensitive data as the cookie and session key have not changed

So, I'd propose leaving clear() as is and adding reset() or destroy() that

  • generates a new key
  • calls clear() in try/catch
  • assigns the new key to the new session in finally and re-raises the exception that was thrown
  • assures that the old cookie is removed from browser and perhaps a new one sent regardless of exceptions thrown

Obviously, if B has maliciously stolen A's session this doesn't help. It's only good for the common inadvertent case.

07/30/08 08:28:34 changed by mrts

  • attachment clear_and_destroy.diff added.

Exception-safe destroy() added to request.session, with tests and docs

07/30/08 08:39:17 changed by mrts

  • summary changed from Add .clear() to session objects to Add .clear() and .destroy() to session objects.

07/31/08 12:25:01 changed by mrts

  • attachment clear_and_destroy2.diff added.

Remove redundant .save()s from tests and update to r8158

08/08/08 16:04:18 changed by jacob

  • stage changed from Design decision needed to Accepted.

08/09/08 10:54:51 changed by jacob

  • owner changed from jacob to mtredinnick.
  • status changed from assigned to new.

08/13/08 22:57:30 changed by mtredinnick

(In [8341]) Added a clear() method to sessions. Patch from mrts. Refs #7515.

08/13/08 22:57:47 changed by mtredinnick

(In [8342]) Implemented a flush() method on sessions that cleans out the session and regenerates the key. Used to ensure the caller gets a fresh session at logout, for example.

Based on a patch from mrts. Refs #7515.

08/13/08 23:02:43 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

I'm not going to worry about the attempt at exception-proofing mentioned in comment 6. That only trades one race condition for some other ones. The random numbers could collide. The network between the server and the browser could fail to deliver the message. The user could turn off their browser or computer before the reply gets back, etc. There's no way to completely guarantee that the updated cookie will reach their browser, so there's always a low-probability chance of failure.

Add/Change #7515 (Add .clear() and .destroy() to session objects)




Change Properties
Action