[Django] #19324: invalid session keys cause unnecessary empty records in django_session table
Django
table
----------------------------------+--------------------
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
db session store calls self.create when no record is found for the session
key, which causes an empty record inserted. Is this necessary? This gives
chance to user to fill the session table with empty records by sending
invalid session keys.
is it more appropriate to set session_key to be None in this case?
current implementation:
{{{
def load(self):
try:
s = Session.objects.get(
session_key=self.session_key,
expire_date__gt=timezone.now()
)
return self.decode(s.session_data)
except (Session.DoesNotExist, SuspiciousOperation):
self.create()
return {}
}}}
suggested implementation:
{{{
def load(self):
try:
s = Session.objects.get(
session_key=self.session_key,
expire_date__gt=timezone.now()
)
return self.decode(s.session_data)
except (Session.DoesNotExist, SuspiciousOperation):
self.session_key = None
return {}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19324>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.4
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* needs_better_patch: => 0
* stage: Unreviewed => Design decision needed
* needs_tests: => 0
* needs_docs: => 0
Comment:
You probably meant: `self._session_key = None`.
I don't immediately see how this could allow session fixation attacks —
but that doesn't prove anything :)
As is, this change causes two test failures:
{{{
Creating test database for alias 'default'...
Creating test database for alias 'other'...
........................................................................................x................................................F.............................F..........................................
======================================================================
FAIL: test_save (django.contrib.sessions.tests.DatabaseSessionTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/aaugustin/Documents/dev/django/django/contrib/sessions/tests.py",
line 143, in test_save
self.assertTrue(self.session.exists(self.session.session_key))
AssertionError: False is not true
======================================================================
FAIL: test_save
(django.contrib.sessions.tests.DatabaseSessionWithTimeZoneTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/aaugustin/Documents/dev/django/django/contrib/sessions/tests.py",
line 143, in test_save
self.assertTrue(self.session.exists(self.session.session_key))
AssertionError: False is not true
----------------------------------------------------------------------
Ran 210 tests in 0.353s
FAILED (failures=2, expected failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...
}}}
This could probably be resolved in `save()`, though.
----
In fact, this change would cause `save()` to be called instead of
`create()`. Currently the roles of these two functions overlap: `save()`
even has a `must_create` argument! See also #18344.
To sum up, the behavior described exists, but it has a very low impact,
and even with the proposed change it's easy to cause the cache to fill up.
I suspect this ticket should be closed in favor of a ticket describing a
refactoring of the sessions API to eliminate the redundancy between
`save()` and `create()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:1>
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Design decision needed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:2>
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.4
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"66d12d1ababa8f062857ee5eb43276493720bf16" 66d12d1a]:
{{{
#!CommitTicketReference repository=""
revision="66d12d1ababa8f062857ee5eb43276493720bf16"
[1.8.x] Fixed #19324 -- Avoided creating a session record when loading the
session.
The session record is now only created if/when the session is modified.
This
prevents a potential DoS via creation of many empty session records.
This is a security fix; disclosure to follow shortly.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:3>
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.4
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"df049ed77a4db67e45db5679bfc76a85d2a26680" df049ed]:
{{{
#!CommitTicketReference repository=""
revision="df049ed77a4db67e45db5679bfc76a85d2a26680"
Fixed #19324 -- Avoided creating a session record when loading the
session.
The session record is now only created if/when the session is modified.
This
prevents a potential DoS via creation of many empty session records.
This is a security fix; disclosure to follow shortly.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:4>
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.4
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"1828f4341ec53a8684112d24031b767eba557663" 1828f43]:
{{{
#!CommitTicketReference repository=""
revision="1828f4341ec53a8684112d24031b767eba557663"
[1.7.x] Fixed #19324 -- Avoided creating a session record when loading the
session.
The session record is now only created if/when the session is modified.
This
prevents a potential DoS via creation of many empty session records.
This is a security fix; disclosure to follow shortly.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:6>
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.4
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2e47f3e401c29bc2ba5ab794d483cb0820855fb9" 2e47f3e]:
{{{
#!CommitTicketReference repository=""
revision="2e47f3e401c29bc2ba5ab794d483cb0820855fb9"
[1.4.x] Fixed #19324 -- Avoided creating a session record when loading the
session.
The session record is now only created if/when the session is modified.
This
prevents a potential DoS via creation of many empty session records.
This is a security fix; disclosure to follow shortly.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:5>
Django
table
Reporter: liangrubo@… | Owner: nobody
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.4
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by lamby):
Now the issue has been disclosed, could the logic be very briefly
documented in the code itself? Stripped of context of this security issue,
not immediately obvious why — for example — only if session_key is None we
want to call .create(), etc. etc.
--
Ticket URL: <https://code.djangoproject.com/ticket/19324#comment:7>