Silently replaced session_key

[George Sakkis]

Is this a bug or a feature ?

>>> from django.contrib.sessions.backends.db import SessionStore
>>> s = SessionStore(session_key='secret!!!11')
>>> s.session_key
'secret!!!1!1'
>>> 'foo' in s
False
>>> s.session_key
'7f9aa956cb169b1f89a3a5b384cafc1b'

George

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

[Jacob Kaplan-Moss]

On Tue, May 4, 2010 at 3:11 PM, George Sakkis <george...@gmail.com> wrote:
> Is this a bug or a feature ?

Take a look at the source (django/contrib/sessions/backends/db.py;
line 16 - the load() function). If the session key doesn't exist in
the database, a new session key will be generated. This prevents users
from being able to "choose" arbitrary session keys, so that's the
correct thing to do.

Jacob

[George Sakkis]

On May 4, 11:05 pm, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:

> On Tue, May 4, 2010 at 3:11 PM, George Sakkis <george.sak...@gmail.com> wrote:
> > Is this a bug or a feature ?
>
> Take a look at the source (django/contrib/sessions/backends/db.py;
> line 16 - the load() function). If the session key doesn't exist in
> the database, a new session key will be generated. This prevents users
> from being able to "choose" arbitrary session keys, so that's the
> correct thing to do.

Ok, the rationale makes sense (as a default, overridable choice at
least) but the API could be less surprising, e.g. raise an exception
when a non auto-generated key is passed. Also it turns out that it
doesn't really prevent setting a key explicitly, it just makes it more
cumbersome:

session_key = 'secret!!1!'
s = SessionStore(session_key)
s['foo'] = 'bar'
s.session_key = session_key # I *really* mean session_key dammit
s.save()

This creates two entries, one with a random key and one with
session_key but only the latter's session_data are updated.

The following avoids creating the random key in the first place:

s = SessionStore()
if not s.exists(session_key):
s['foo'] = 'bar'
s.session_key = session_key
s.save()

I'm not sure if these are unintended implementation side effects but
they seem incongruent.

George

[Tom Evans]

If you allow users to present new session ids without replacing them,
then you open up an attack vector for a session fixation attack. The
default behaviour is default for a reason.

Cheers

Tom

[George Sakkis]

On May 5, 12:25 pm, Tom Evans <tevans...@googlemail.com> wrote:

I'm repeating myself here but if the intention is to really disallow
user-provided ids. it can be done more clearly: raise an exception if
the key does not exist and make the session_key property read-only.
Now it seems like a bug that you can sort of work around by setting
the key just before saving.

By the way, this does not apply to all backends; file SessionStore for
example uses passed ids as is. Whatever the desired behavior is, it
should apply to all backends, so the relevant logic should move to
SessionBase.

George

[Jeremy Dunck]

On Wed, May 5, 2010 at 2:45 PM, George Sakkis <george...@gmail.com> wrote:
...

> I'm repeating myself here but if the intention is to really disallow
> user-provided ids. it can be done more clearly: raise an exception if
> the key does not exist and make the session_key property read-only.
> Now it seems like a bug that you can sort of work around by setting
> the key just before saving.

Allowing an attacker to predictably raise exceptions might be bad.

> By the way, this does not apply to all backends; file SessionStore for
> example uses passed ids as is. Whatever the desired behavior is, it
> should apply to all backends, so the relevant logic should  move to
> SessionBase.

I filed a ticket for this: http://code.djangoproject.com/ticket/13478

[Matthew Roy]

How so? An exception here will be caught by the app or become a 500. That's better than possibly using a chosen session key due to miscoding.

Matthew

On May 5, 2010 4:20 PM, "Jeremy Dunck" <jdu...@gmail.com> wrote:

On Wed, May 5, 2010 at 2:45 PM, George Sakkis <george...@gmail.com> wrote:
...

> I'm repeating myself here but if the intention is to really disallow

> user-provided ids. it can b...

Allowing an attacker to predictably raise exceptions might be bad.

> By the way, this does not apply to all backends; file SessionStore for

> example uses passed ids ...

I filed a ticket for this: http://code.djangoproject.com/ticket/13478

--
You received this message because you are subscribed to the Google Groups "Django developers" g...

[Ulrich Petri]

Am 05.05.2010 um 21:45 schrieb George Sakkis:

>
> I'm repeating myself here but if the intention is to really disallow
> user-provided ids. it can be done more clearly: raise an exception if
> the key does not exist and make the session_key property read-only.
> Now it seems like a bug that you can sort of work around by setting
> the key just before saving.
>

Depending on the backend it might be difficult to determine what
exactly constitutes a non-existing key.
A visitor could for example have a stale session cookie in their
browser that has already been evicted from your storage.
In such cases the user would be presented with an exception - not very
friendly.

Ulrich

[George Sakkis]

On May 5, 10:20 pm, Jeremy Dunck <jdu...@gmail.com> wrote:

> On Wed, May 5, 2010 at 2:45 PM, George Sakkis <george.sak...@gmail.com> wrote:
>
> ...
>
> > I'm repeating myself here but if the intention is to really disallow
> > user-provided ids. it can be done more clearly: raise an exception if
> > the key does not exist and make the session_key property read-only.
> > Now it seems like a bug that you can sort of work around by setting
> > the key just before saving.
>
> Allowing an attacker to predictably raise exceptions might be bad.

The exception would not propagate of course all the way up the stack,
it would be caught by a caller that knows how to handle it (e.g.
middleware). In this way SessionStore backends would be responsible
only for the low level storage details, not the security policy. On
second thought though this is probably impractical since load() is
called implicitly the first time a dict-like method is called, so the
exception could be raised almost anywhere the session is first
accessed.

George