Code

Opened 7 years ago

Closed 7 years ago

#4531 closed (fixed)

SessionId collision - session takeover by accident

Reported by: Frank Tegtmeyer <fte@…> Owned by: adrian
Component: Contrib apps Version: master
Severity: Keywords: sessionid session patch
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

I just had an accidental session takeover at a clients web site.

My session settings:

SESSION_COOKIE_SECURE = True
SESSION_EXPIRE_AT_BROWSER_CLOSE = True
SESSION_COOKIE_AGE = 70000

Environment:
Python 2.5, Django SVN Revision: 5320, OpenBSD 4.1, lighttpd with FastCGI

After checking the generation of the sessionid I found that there may be
the following reasons (in combination):

  • very low traffic site (at the moment)
  • I had a very short session before logging in again, I used the logout link (admin interface).
  • no deletion of the session cookie when logging out (not sure about that but it would explain the behaviour)
  • five django processes, each having its own seeded random module
  • exclusive use of fixed data or determined data for the sessionid generation

I think I reused my old sessionid by still having the cookie. Between logging
out and logging in again another user got the same sessionid (because it
was not in the database anymore). So I got an authenticated session from the
other user.

A patch is provided, maybe even the remote IP should be included in feeding md5.
The session cookie should also be deleted when using a logout link.

Regards, Frank

Attachments (1)

sessionid_patch (1.1 KB) - added by Frank Tegtmeyer <fte@…> 7 years ago.
Patch for safer sessionid generation

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by Frank Tegtmeyer <fte@…>

Patch for safer sessionid generation

comment:1 Changed 7 years ago by Frank Tegtmeyer <fte@…>

  • Keywords patch added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from SessionId collision - session takeover by accident to [patch] SessionId collision - session takeover by accident

comment:2 follow-up: Changed 7 years ago by mtredinnick

  • Patch needs improvement set
  • Summary changed from [patch] SessionId collision - session takeover by accident to SessionId collision - session takeover by accident
  • Triage Stage changed from Unreviewed to Accepted

The os.getpid() change in the patch reduces randomness (the range of pid values is smaller than the range 0 - maxint, plus pid will be the same for processes in multi-threaded servers on some systems), so that part should go out. Adding time.time() seems like a good idea; it's similar to how we fixed another clash when the pseudo-random seeding was too close together.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by Frank Tegtmeyer <fte@…>

Replying to mtredinnick:

The os.getpid() change in the patch reduces randomness

No, it doesn't (reasoning further down).

(the range of pid values is smaller than the range 0 - maxint,

Yes, this is true. But the current doubled random() call is in fact so randon like a single call. So getpid() does add randomness in comparison to the current code.

plus pid will be the same for processes in multi-threaded servers on some systems),

For this case it doesn't matter because then there are multiple calls to random() which will give different results.

You have either

  • randomness from getpid() - or if this is inside one process:
  • randomness by random() and always
  • randomness by time()


so that part should go out.

time() could be enough randomness, but adding the pid will increase that additionally.

Regards, Frank

comment:4 in reply to: ↑ 3 Changed 7 years ago by Frank Tegtmeyer <fte@…>

Replying to Frank Tegtmeyer <fte@fte.to>:

the current doubled random() call is in fact so randon like a single call

I think this point is not obvious to everybody, so here some additional explanation:

The pseudo random number generator is a completely deterministic thing. So if
you call it once you get a value. If you call it once again, you get another value
but this value is dependent on the first. So you have no additional randomness here.
A generator like the one used by random() always produces the same sequence of numbers. That's also the main reason for the id duplication.

The random module documentation has a nice sentence in it that says it all:

However, being completely deterministic, it is not suitable for all purposes, and is completely unsuitable for cryptographic purposes.

Hope, that makes it clearer.

Regards, Frank

comment:5 Changed 7 years ago by mtredinnick

Yeah, I understood what you were saying. You're correct. Oversight on my part the first time.

comment:6 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 7 years ago by mtredinnick

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

(In [5470]) Fixed #4531 -- Added a bit more randomness to session idents. Thanks, Frank
Tegtmeyer.

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.