Opened 17 years ago

Closed 17 years ago

#3507 closed (worksforme)

sessions race condition

Reported by: jimmy@… Owned by: Adrian Holovaty
Component: Contrib apps Version: dev
Severity: Keywords: sessions save
Cc: tom@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Regarding this piece of code in django/contrib/sessions/models.py:

        while 1:
            session_key = md5.new(str(random.randint(0, sys.maxint - 1)) + str(random.randint(0, sys.maxint - 1)) + settings.SECRET_KEY).hexdigest()
            try:
                self.get(session_key=session_key)
            except self.model.DoesNotExist:
                break
        return session_key

There is a very very small chance that a race condition exists between finding a uniq session, and saving it; which would result in one user ending up with a session owned by someone else. I know the chance is very small, but I do worry about it. Maybe it would be possible to also include remote_addr into the to be hashed string?

I also want to add that it would be nice to make a configuration option to make it impossible to use a session from another remote_addr. I might be to paranoid.

Change History (5)

comment:1 by James Bennett, 17 years ago

I'm not certain that it's something worth worrying about, given the extreme improbability -- unless I'm misreading, it would require two users to get the same pair of random numbers in the same order within an extremely short time of one another. And if we're going to worry about that probability -- my rough calculation is that such a collision will happen roughly once in every 4.6 quintillion key generations -- then we're stuck with no solution; there are also infinitesimally small chances that two users behind the same NAT would get the same pair of random numbers (in which case REMOTE_ADDR is no good) or would get the same datetime stamp from different server processes (in which case using a timestamp as salt is no good), etc.

Or is there something I'm missing here?

comment:2 by jimmy@…, 17 years ago

Keywords: save added

If your calculation is correct, I won't have to worry about it indeed.

I blame my lack of knowledge on the exact working of the PRNG in use, it's used seed, ... because it will be very dependant on that.

Your statement on remote_addr is also correct, another one would be the chanse that md5 collisions occur while still having a random value, ...

I however believe that it would be better to make sure that in the case of a new session, save will only try and perform an INSERT, and never try the SELECT/UPDATE pair as is being used.

Maybe the flaw is not exactly in the session implementation, but in save() in general; because for whatever model we use save(), I think we can never be sure that the values will be INSERT'ed, and not UPDATE'ed by concurrent requests instead of failing and giving a chance to rollback (last time I checked).

I might for example add a Domain(model) to my Profile(model) only if it does not yet exist, concurrently with someone else, even when using sessions, both requests will perform a SELECT, one performs a SELECT and INSERT, the other one will block untill the first one is commited, if that happens, it will perform a SELECT and UPDATE, and both sessions receive an 'OK' while only one has the domain in it's profile. To make it worse, it could be possible that they both send a signal to some daemon to process the domain addition, and things go boom. Am I correct on this?

If this is correct, it would be nice to be able to force certain behaviour and take advantage in the session implementation for example.

eg:

def save(..., new_obj=False, existing_obj=False):

I thank you already for your time spend.

comment:3 by anonymous, 17 years ago

I also noticed this piece of code a few weeks ago and thought the same thing.

However, this is not the only piece of code that has a race condition. There's e.g. get_or_create() where a race condition is very probable.

See my post to the django-developers mailing list for details.

comment:4 by Thomas Steinacher <tom@…>, 17 years ago

Cc: tom@… added

(the previous comment was by me)

comment:5 by James Bennett, 17 years ago

Resolution: worksforme
Status: newclosed

Closing this "worksforme" for the various reasons outlined above.

Thomas, as pointed out in that thread, the solution for get_or_create in critical sections is to use transaction isolation.

Note: See TracTickets for help on using tickets.
Back to Top