From 66d12d1ababa8f062857ee5eb43276493720bf16 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 10 Jun 2015 15:45:20 -0600 Subject: [PATCH] [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. --- django/contrib/sessions/backends/cache.py | 6 ++++-- django/contrib/sessions/backends/cached_db.py | 4 ++-- django/contrib/sessions/backends/db.py | 5 +++-- django/contrib/sessions/backends/file.py | 5 +++-- docs/releases/1.4.21.txt | 21 +++++++++++++++++++ docs/releases/1.7.9.txt | 21 +++++++++++++++++++ docs/releases/1.8.3.txt | 21 +++++++++++++++++++ tests/sessions_tests/tests.py | 20 ++++++++++++++++++ 8 files changed, 95 insertions(+), 8 deletions(-) diff --git a/django/contrib/sessions/backends/cache.py b/django/contrib/sessions/backends/cache.py index 38b6112f51..5bc4dc1f8d 100644 --- a/django/contrib/sessions/backends/cache.py +++ b/django/contrib/sessions/backends/cache.py @@ -27,7 +27,7 @@ class SessionStore(SessionBase): session_data = None if session_data is not None: return session_data - self.create() + self._session_key = None return {} def create(self): @@ -49,6 +49,8 @@ class SessionStore(SessionBase): "It is likely that the cache is unavailable.") def save(self, must_create=False): + if self.session_key is None: + return self.create() if must_create: func = self._cache.add else: @@ -60,7 +62,7 @@ class SessionStore(SessionBase): raise CreateError 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): if session_key is None: diff --git a/django/contrib/sessions/backends/cached_db.py b/django/contrib/sessions/backends/cached_db.py index 0ba12f553f..435806595d 100644 --- a/django/contrib/sessions/backends/cached_db.py +++ b/django/contrib/sessions/backends/cached_db.py @@ -51,12 +51,12 @@ class SessionStore(DBStore): logger = logging.getLogger('django.security.%s' % e.__class__.__name__) logger.warning(force_text(e)) - self.create() + self._session_key = None data = {} return data 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 super(SessionStore, self).exists(session_key) diff --git a/django/contrib/sessions/backends/db.py b/django/contrib/sessions/backends/db.py index 450459108e..2d859d3b83 100644 --- a/django/contrib/sessions/backends/db.py +++ b/django/contrib/sessions/backends/db.py @@ -26,7 +26,7 @@ class SessionStore(SessionBase): logger = logging.getLogger('django.security.%s' % e.__class__.__name__) logger.warning(force_text(e)) - self.create() + self._session_key = None return {} def exists(self, session_key): @@ -43,7 +43,6 @@ class SessionStore(SessionBase): # Key wasn't unique. Try again. continue self.modified = True - self._session_cache = {} return def save(self, must_create=False): @@ -53,6 +52,8 @@ class SessionStore(SessionBase): create a *new* entry (as opposed to possibly updating an existing entry). """ + if self.session_key is None: + return self.create() obj = Session( session_key=self._get_or_create_session_key(), session_data=self.encode(self._get_session(no_load=must_create)), diff --git a/django/contrib/sessions/backends/file.py b/django/contrib/sessions/backends/file.py index 10d163acc4..41469c4a26 100644 --- a/django/contrib/sessions/backends/file.py +++ b/django/contrib/sessions/backends/file.py @@ -97,7 +97,7 @@ class SessionStore(SessionBase): self.delete() self.create() except (IOError, SuspiciousOperation): - self.create() + self._session_key = None return session_data def create(self): @@ -108,10 +108,11 @@ class SessionStore(SessionBase): except CreateError: continue self.modified = True - self._session_cache = {} return def save(self, must_create=False): + if self.session_key is None: + return self.create() # Get the session data now, before we start messing # with the file it is stored within. session_data = self._get_session(no_load=must_create) diff --git a/docs/releases/1.4.21.txt b/docs/releases/1.4.21.txt index 6ff4c6d115..da69b26564 100644 --- a/docs/releases/1.4.21.txt +++ b/docs/releases/1.4.21.txt @@ -5,3 +5,24 @@ Django 1.4.21 release notes *July 8, 2015* 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. diff --git a/docs/releases/1.7.9.txt b/docs/releases/1.7.9.txt index a2101ad6f9..f402a4cdd5 100644 --- a/docs/releases/1.7.9.txt +++ b/docs/releases/1.7.9.txt @@ -6,6 +6,27 @@ Django 1.7.9 release notes 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 ======== diff --git a/docs/releases/1.8.3.txt b/docs/releases/1.8.3.txt index aa6e46a99c..c2283048a5 100644 --- a/docs/releases/1.8.3.txt +++ b/docs/releases/1.8.3.txt @@ -11,6 +11,27 @@ Also, ``django.utils.deprecation.RemovedInDjango20Warning`` was renamed to 1.11 (LTS), 2.0 (drops Python 2 support). For backwards compatibility, ``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 ======== diff --git a/tests/sessions_tests/tests.py b/tests/sessions_tests/tests.py index 78f7a07155..a1aab37273 100644 --- a/tests/sessions_tests/tests.py +++ b/tests/sessions_tests/tests.py @@ -175,6 +175,11 @@ class SessionTestsMixin(object): self.assertNotEqual(self.session.session_key, prev_key) 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): # Submitting an invalid session key (either by guessing, or if the db has # removed the key) results in a new key being generated. @@ -313,6 +318,21 @@ class SessionTestsMixin(object): self.session.delete(old_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):