From 10b7da0a156f58ced13e4baba15108eb8723d303 Mon Sep 17 00:00:00 2001 From: Matthias Runge Date: Fri, 17 Jul 2015 10:54:54 +0200 Subject: [PATCH] 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 is a cherry-pick to stable/1.6.x --- django/contrib/sessions/backends/cache.py | 6 ++++-- django/contrib/sessions/backends/cached_db.py | 14 ++++++++------ django/contrib/sessions/backends/db.py | 6 ++++-- django/contrib/sessions/backends/file.py | 6 ++++-- django/contrib/sessions/tests.py | 21 +++++++++++++++++++++ 5 files changed, 41 insertions(+), 12 deletions(-) diff --git a/django/contrib/sessions/backends/cache.py b/django/contrib/sessions/backends/cache.py index 596042f..87df6be 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 be22c1f..8f2cf81 100644 --- a/django/contrib/sessions/backends/cached_db.py +++ b/django/contrib/sessions/backends/cached_db.py @@ -4,6 +4,7 @@ Cached, database-backed sessions. import logging +from django.conf import settings from django.contrib.sessions.backends.db import SessionStore as DBStore from django.core.cache import cache from django.core.exceptions import SuspiciousOperation @@ -19,6 +20,7 @@ class SessionStore(DBStore): """ def __init__(self, session_key=None): + self._cache = cache super(SessionStore, self).__init__(session_key) @property @@ -27,7 +29,7 @@ class SessionStore(DBStore): def load(self): try: - data = cache.get(self.cache_key, None) + data = self._cache.get(self.cache_key, None) except Exception: # Some backends (e.g. memcache) raise an exception on invalid # cache keys. If this happens, reset the session. See #17810. @@ -42,25 +44,25 @@ class SessionStore(DBStore): expire_date__gt=timezone.now() ) data = self.decode(s.session_data) - cache.set(self.cache_key, data, + self._cache.set(self.cache_key, data, self.get_expiry_age(expiry=s.expire_date)) except (Session.DoesNotExist, SuspiciousOperation) as e: if isinstance(e, SuspiciousOperation): 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 cache: + if session_key and (KEY_PREFIX + session_key) in self._cache: return True return super(SessionStore, self).exists(session_key) def save(self, must_create=False): super(SessionStore, self).save(must_create) - cache.set(self.cache_key, self._session, self.get_expiry_age()) + self._cache.set(self.cache_key, self._session, self.get_expiry_age()) def delete(self, session_key=None): super(SessionStore, self).delete(session_key) @@ -68,7 +70,7 @@ class SessionStore(DBStore): if self.session_key is None: return session_key = self.session_key - cache.delete(KEY_PREFIX + session_key) + self._cache.delete(KEY_PREFIX + session_key) def flush(self): """ diff --git a/django/contrib/sessions/backends/db.py b/django/contrib/sessions/backends/db.py index 7be99c3..3e6cdf9 100644 --- a/django/contrib/sessions/backends/db.py +++ b/django/contrib/sessions/backends/db.py @@ -6,6 +6,7 @@ from django.db import IntegrityError, transaction, router from django.utils import timezone from django.utils.encoding import force_text + class SessionStore(SessionBase): """ Implements database session store. @@ -25,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): @@ -42,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): @@ -52,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 f47aa2d..f886bcd 100644 --- a/django/contrib/sessions/backends/file.py +++ b/django/contrib/sessions/backends/file.py @@ -13,6 +13,7 @@ from django.utils.encoding import force_text from django.contrib.sessions.exceptions import InvalidSessionKey + class SessionStore(SessionBase): """ Implements a file based session store. @@ -95,7 +96,7 @@ class SessionStore(SessionBase): self.delete() self.create() except (IOError, SuspiciousOperation): - self.create() + self._session_key = None return session_data def create(self): @@ -106,10 +107,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/django/contrib/sessions/tests.py b/django/contrib/sessions/tests.py index ff0a70c..6a04f24 100644 --- a/django/contrib/sessions/tests.py +++ b/django/contrib/sessions/tests.py @@ -160,6 +160,11 @@ class SessionTestsMixin(object): self.assertTrue(self.session.modified) self.assertTrue(self.session.accessed) + def test_save_doesnt_clear_data(self): + self.session['a'] = 'b' + self.session.save() + self.assertEqual(self.session['a'], 'b') + def test_cycle(self): self.session['a'], self.session['b'] = 'c', 'd' self.session.save() @@ -305,6 +310,22 @@ 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): -- 2.4.3