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.
This commit is contained in:
Carl Meyer 2015-06-10 15:45:20 -06:00 committed by Tim Graham
parent 125eaa19b2
commit df049ed77a
8 changed files with 95 additions and 8 deletions

View File

@ -27,7 +27,7 @@ class SessionStore(SessionBase):
session_data = None session_data = None
if session_data is not None: if session_data is not None:
return session_data return session_data
self.create() self._session_key = None
return {} return {}
def create(self): def create(self):
@ -49,6 +49,8 @@ class SessionStore(SessionBase):
"It is likely that the cache is unavailable.") "It is likely that the cache is unavailable.")
def save(self, must_create=False): def save(self, must_create=False):
if self.session_key is None:
return self.create()
if must_create: if must_create:
func = self._cache.add func = self._cache.add
else: else:
@ -60,7 +62,7 @@ class SessionStore(SessionBase):
raise CreateError raise CreateError
def exists(self, session_key): def exists(self, session_key):
return (KEY_PREFIX + session_key) in self._cache return session_key and (KEY_PREFIX + session_key) in self._cache
def delete(self, session_key=None): def delete(self, session_key=None):
if session_key is None: if session_key is None:

View File

@ -51,12 +51,12 @@ class SessionStore(DBStore):
logger = logging.getLogger('django.security.%s' % logger = logging.getLogger('django.security.%s' %
e.__class__.__name__) e.__class__.__name__)
logger.warning(force_text(e)) logger.warning(force_text(e))
self.create() self._session_key = None
data = {} data = {}
return data return data
def exists(self, session_key): def exists(self, session_key):
if (KEY_PREFIX + session_key) in self._cache: if session_key and (KEY_PREFIX + session_key) in self._cache:
return True return True
return super(SessionStore, self).exists(session_key) return super(SessionStore, self).exists(session_key)

View File

@ -26,7 +26,7 @@ class SessionStore(SessionBase):
logger = logging.getLogger('django.security.%s' % logger = logging.getLogger('django.security.%s' %
e.__class__.__name__) e.__class__.__name__)
logger.warning(force_text(e)) logger.warning(force_text(e))
self.create() self._session_key = None
return {} return {}
def exists(self, session_key): def exists(self, session_key):
@ -43,7 +43,6 @@ class SessionStore(SessionBase):
# Key wasn't unique. Try again. # Key wasn't unique. Try again.
continue continue
self.modified = True self.modified = True
self._session_cache = {}
return return
def save(self, must_create=False): def save(self, must_create=False):
@ -53,6 +52,8 @@ class SessionStore(SessionBase):
create a *new* entry (as opposed to possibly updating an existing create a *new* entry (as opposed to possibly updating an existing
entry). entry).
""" """
if self.session_key is None:
return self.create()
obj = Session( obj = Session(
session_key=self._get_or_create_session_key(), session_key=self._get_or_create_session_key(),
session_data=self.encode(self._get_session(no_load=must_create)), session_data=self.encode(self._get_session(no_load=must_create)),

View File

@ -97,7 +97,7 @@ class SessionStore(SessionBase):
self.delete() self.delete()
self.create() self.create()
except (IOError, SuspiciousOperation): except (IOError, SuspiciousOperation):
self.create() self._session_key = None
return session_data return session_data
def create(self): def create(self):
@ -108,10 +108,11 @@ class SessionStore(SessionBase):
except CreateError: except CreateError:
continue continue
self.modified = True self.modified = True
self._session_cache = {}
return return
def save(self, must_create=False): def save(self, must_create=False):
if self.session_key is None:
return self.create()
# Get the session data now, before we start messing # Get the session data now, before we start messing
# with the file it is stored within. # with the file it is stored within.
session_data = self._get_session(no_load=must_create) session_data = self._get_session(no_load=must_create)

View File

@ -5,3 +5,24 @@ Django 1.4.21 release notes
*July 8, 2015* *July 8, 2015*
Django 1.4.21 fixes several security issues in 1.4.20. Django 1.4.21 fixes several security issues in 1.4.20.
Denial-of-service possibility by filling session store
======================================================
In previous versions of Django, the session backends created a new empty record
in the session storage anytime ``request.session`` was accessed and there was a
session key provided in the request cookies that didn't already have a session
record. This could allow an attacker to easily create many new session records
simply by sending repeated requests with unknown session keys, potentially
filling up the session store or causing other users' session records to be
evicted.
The built-in session backends now create a session record only if the session
is actually modified; empty session records are not created. Thus this
potential DoS is now only possible if the site chooses to expose a
session-modifying view to anonymous users.
As each built-in session backend was fixed separately (rather than a fix in the
core sessions framework), maintainers of third-party session backends should
check whether the same vulnerability is present in their backend and correct
it if so.

View File

@ -6,6 +6,27 @@ Django 1.7.9 release notes
Django 1.7.9 fixes several security issues and bugs in 1.7.8. Django 1.7.9 fixes several security issues and bugs in 1.7.8.
Denial-of-service possibility by filling session store
======================================================
In previous versions of Django, the session backends created a new empty record
in the session storage anytime ``request.session`` was accessed and there was a
session key provided in the request cookies that didn't already have a session
record. This could allow an attacker to easily create many new session records
simply by sending repeated requests with unknown session keys, potentially
filling up the session store or causing other users' session records to be
evicted.
The built-in session backends now create a session record only if the session
is actually modified; empty session records are not created. Thus this
potential DoS is now only possible if the site chooses to expose a
session-modifying view to anonymous users.
As each built-in session backend was fixed separately (rather than a fix in the
core sessions framework), maintainers of third-party session backends should
check whether the same vulnerability is present in their backend and correct
it if so.
Bugfixes Bugfixes
======== ========

View File

@ -11,6 +11,27 @@ Also, ``django.utils.deprecation.RemovedInDjango20Warning`` was renamed to
1.11 (LTS), 2.0 (drops Python 2 support). For backwards compatibility, 1.11 (LTS), 2.0 (drops Python 2 support). For backwards compatibility,
``RemovedInDjango20Warning`` remains as an importable alias. ``RemovedInDjango20Warning`` remains as an importable alias.
Denial-of-service possibility by filling session store
======================================================
In previous versions of Django, the session backends created a new empty record
in the session storage anytime ``request.session`` was accessed and there was a
session key provided in the request cookies that didn't already have a session
record. This could allow an attacker to easily create many new session records
simply by sending repeated requests with unknown session keys, potentially
filling up the session store or causing other users' session records to be
evicted.
The built-in session backends now create a session record only if the session
is actually modified; empty session records are not created. Thus this
potential DoS is now only possible if the site chooses to expose a
session-modifying view to anonymous users.
As each built-in session backend was fixed separately (rather than a fix in the
core sessions framework), maintainers of third-party session backends should
check whether the same vulnerability is present in their backend and correct
it if so.
Bugfixes Bugfixes
======== ========

View File

@ -178,6 +178,11 @@ class SessionTestsMixin(object):
self.assertNotEqual(self.session.session_key, prev_key) self.assertNotEqual(self.session.session_key, prev_key)
self.assertEqual(list(self.session.items()), prev_data) self.assertEqual(list(self.session.items()), prev_data)
def test_save_doesnt_clear_data(self):
self.session['a'] = 'b'
self.session.save()
self.assertEqual(self.session['a'], 'b')
def test_invalid_key(self): def test_invalid_key(self):
# Submitting an invalid session key (either by guessing, or if the db has # Submitting an invalid session key (either by guessing, or if the db has
# removed the key) results in a new key being generated. # removed the key) results in a new key being generated.
@ -331,6 +336,21 @@ class SessionTestsMixin(object):
self.session.delete(old_session_key) self.session.delete(old_session_key)
self.session.delete(new_session_key) self.session.delete(new_session_key)
def test_session_load_does_not_create_record(self):
"""
Loading an unknown session key does not create a session record.
Creating session records on load is a DOS vulnerability.
"""
if self.backend is CookieSession:
raise unittest.SkipTest("Cookie backend doesn't have an external store to create records in.")
session = self.backend('someunknownkey')
session.load()
self.assertFalse(session.exists(session.session_key))
# provided unknown key was cycled, not reused
self.assertNotEqual(session.session_key, 'someunknownkey')
class DatabaseSessionTests(SessionTestsMixin, TestCase): class DatabaseSessionTests(SessionTestsMixin, TestCase):