diff options
| -rw-r--r-- | django/contrib/sessions/backends/base.py | 9 | ||||
| -rw-r--r-- | django/contrib/sessions/backends/cached_db.py | 2 | ||||
| -rw-r--r-- | django/contrib/sessions/middleware.py | 50 | ||||
| -rw-r--r-- | django/contrib/sessions/tests.py | 70 | ||||
| -rw-r--r-- | docs/releases/1.4.22.txt | 18 | ||||
| -rw-r--r-- | docs/releases/1.7.10.txt | 18 | ||||
| -rw-r--r-- | docs/topics/http/sessions.txt | 14 |
7 files changed, 154 insertions, 27 deletions
diff --git a/django/contrib/sessions/backends/base.py b/django/contrib/sessions/backends/base.py index a77a25bb31..c7819b220d 100644 --- a/django/contrib/sessions/backends/base.py +++ b/django/contrib/sessions/backends/base.py @@ -142,6 +142,13 @@ class SessionBase(object): self.accessed = True self.modified = True + def is_empty(self): + "Returns True when there is no session_key and the session is empty" + try: + return not bool(self._session_key) and not self._session_cache + except AttributeError: + return True + def _get_new_session_key(self): "Returns session key that isn't being used." while True: @@ -268,7 +275,7 @@ class SessionBase(object): """ self.clear() self.delete() - self.create() + self._session_key = None def cycle_key(self): """ diff --git a/django/contrib/sessions/backends/cached_db.py b/django/contrib/sessions/backends/cached_db.py index 5cc6f79b5a..5a7664b9b1 100644 --- a/django/contrib/sessions/backends/cached_db.py +++ b/django/contrib/sessions/backends/cached_db.py @@ -79,7 +79,7 @@ class SessionStore(DBStore): """ self.clear() self.delete(self.session_key) - self.create() + self._session_key = None # At bottom to avoid circular import diff --git a/django/contrib/sessions/middleware.py b/django/contrib/sessions/middleware.py index 211ef57d45..54d80f7f33 100644 --- a/django/contrib/sessions/middleware.py +++ b/django/contrib/sessions/middleware.py @@ -18,32 +18,40 @@ class SessionMiddleware(object): def process_response(self, request, response): """ If request.session was modified, or if the configuration is to save the - session every time, save the changes and set a session cookie. + session every time, save the changes and set a session cookie or delete + the session cookie if the session has been emptied. """ try: accessed = request.session.accessed modified = request.session.modified + empty = request.session.is_empty() except AttributeError: pass else: - if accessed: - patch_vary_headers(response, ('Cookie',)) - if modified or settings.SESSION_SAVE_EVERY_REQUEST: - if request.session.get_expire_at_browser_close(): - max_age = None - expires = None - else: - max_age = request.session.get_expiry_age() - expires_time = time.time() + max_age - expires = cookie_date(expires_time) - # Save the session data and refresh the client cookie. - # Skip session save for 500 responses, refs #3881. - if response.status_code != 500: - request.session.save() - response.set_cookie(settings.SESSION_COOKIE_NAME, - request.session.session_key, max_age=max_age, - expires=expires, domain=settings.SESSION_COOKIE_DOMAIN, - path=settings.SESSION_COOKIE_PATH, - secure=settings.SESSION_COOKIE_SECURE or None, - httponly=settings.SESSION_COOKIE_HTTPONLY or None) + # First check if we need to delete this cookie. + # The session should be deleted only if the session is entirely empty + if settings.SESSION_COOKIE_NAME in request.COOKIES and empty: + response.delete_cookie(settings.SESSION_COOKIE_NAME, + domain=settings.SESSION_COOKIE_DOMAIN) + else: + if accessed: + patch_vary_headers(response, ('Cookie',)) + if (modified or settings.SESSION_SAVE_EVERY_REQUEST) and not empty: + if request.session.get_expire_at_browser_close(): + max_age = None + expires = None + else: + max_age = request.session.get_expiry_age() + expires_time = time.time() + max_age + expires = cookie_date(expires_time) + # Save the session data and refresh the client cookie. + # Skip session save for 500 responses, refs #3881. + if response.status_code != 500: + request.session.save() + response.set_cookie(settings.SESSION_COOKIE_NAME, + request.session.session_key, max_age=max_age, + expires=expires, domain=settings.SESSION_COOKIE_DOMAIN, + path=settings.SESSION_COOKIE_PATH, + secure=settings.SESSION_COOKIE_SECURE or None, + httponly=settings.SESSION_COOKIE_HTTPONLY or None) return response diff --git a/django/contrib/sessions/tests.py b/django/contrib/sessions/tests.py index 6e042c7cc6..578b6f4134 100644 --- a/django/contrib/sessions/tests.py +++ b/django/contrib/sessions/tests.py @@ -159,6 +159,7 @@ class SessionTestsMixin(object): self.session.flush() self.assertFalse(self.session.exists(prev_key)) self.assertNotEqual(self.session.session_key, prev_key) + self.assertIsNone(self.session.session_key) self.assertTrue(self.session.modified) self.assertTrue(self.session.accessed) @@ -589,6 +590,75 @@ class SessionMiddlewareTests(unittest.TestCase): # Check that the value wasn't saved above. self.assertNotIn('hello', request.session.load()) + def test_session_delete_on_end(self): + request = RequestFactory().get('/') + response = HttpResponse('Session test') + middleware = SessionMiddleware() + + # Before deleting, there has to be an existing cookie + request.COOKIES[settings.SESSION_COOKIE_NAME] = 'abc' + + # Simulate a request that ends the session + middleware.process_request(request) + request.session.flush() + + # Handle the response through the middleware + response = middleware.process_response(request, response) + + # Check that the cookie was deleted, not recreated. + # A deleted cookie header looks like: + # Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/ + self.assertEqual( + 'Set-Cookie: {0}=; expires=Thu, 01-Jan-1970 00:00:00 GMT; ' + 'Max-Age=0; Path=/'.format(settings.SESSION_COOKIE_NAME), + str(response.cookies[settings.SESSION_COOKIE_NAME]) + ) + + @override_settings(SESSION_COOKIE_DOMAIN='.example.local') + def test_session_delete_on_end_with_custom_domain(self): + request = RequestFactory().get('/') + response = HttpResponse('Session test') + middleware = SessionMiddleware() + + # Before deleting, there has to be an existing cookie + request.COOKIES[settings.SESSION_COOKIE_NAME] = 'abc' + + # Simulate a request that ends the session + middleware.process_request(request) + request.session.flush() + + # Handle the response through the middleware + response = middleware.process_response(request, response) + + # Check that the cookie was deleted, not recreated. + # A deleted cookie header with a custom domain looks like: + # Set-Cookie: sessionid=; Domain=.example.local; + # expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/ + self.assertEqual( + 'Set-Cookie: {}=; Domain=.example.local; expires=Thu, ' + '01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/'.format( + settings.SESSION_COOKIE_NAME, + ), + str(response.cookies[settings.SESSION_COOKIE_NAME]) + ) + + def test_flush_empty_without_session_cookie_doesnt_set_cookie(self): + request = RequestFactory().get('/') + response = HttpResponse('Session test') + middleware = SessionMiddleware() + + # Simulate a request that ends the session + middleware.process_request(request) + request.session.flush() + + # Handle the response through the middleware + response = middleware.process_response(request, response) + + # A cookie should not be set. + self.assertEqual(response.cookies, {}) + # The session is accessed so "Vary: Cookie" should be set. + self.assertEqual(response['Vary'], 'Cookie') + class CookieSessionTests(SessionTestsMixin, TestCase): diff --git a/docs/releases/1.4.22.txt b/docs/releases/1.4.22.txt index d8ce24bc68..9f8177440f 100644 --- a/docs/releases/1.4.22.txt +++ b/docs/releases/1.4.22.txt @@ -9,3 +9,21 @@ Django 1.4.22 fixes a security issue in 1.4.21. It also fixes support with pip 7+ by disabling wheel support. Older versions of 1.4 would silently build a broken wheel when installed with those versions of pip. + +Denial-of-service possibility in ``logout()`` view by filling session store +=========================================================================== + +Previously, a session could be created when anonymously accessing the +:func:`django.contrib.auth.views.logout` view (provided it wasn't decorated +with :func:`~django.contrib.auth.decorators.login_required` as done in the +admin). This could allow an attacker to easily create many new session records +by sending repeated requests, potentially filling up the session store or +causing other users' session records to be evicted. + +The :class:`~django.contrib.sessions.middleware.SessionMiddleware` has been +modified to no longer create empty session records. + +Additionally, the ``contrib.sessions.backends.base.SessionBase.flush()`` and +``cache_db.SessionStore.flush()`` methods have been modified to avoid creating +a new empty session. Maintainers of third-party session backends should check +if the same vulnerability is present in their backend and correct it if so. diff --git a/docs/releases/1.7.10.txt b/docs/releases/1.7.10.txt index 76457bccbd..38af4a42ce 100644 --- a/docs/releases/1.7.10.txt +++ b/docs/releases/1.7.10.txt @@ -5,3 +5,21 @@ Django 1.7.10 release notes *August 18, 2015* Django 1.7.10 fixes a security issue in 1.7.9. + +Denial-of-service possibility in ``logout()`` view by filling session store +=========================================================================== + +Previously, a session could be created when anonymously accessing the +:func:`django.contrib.auth.views.logout` view (provided it wasn't decorated +with :func:`~django.contrib.auth.decorators.login_required` as done in the +admin). This could allow an attacker to easily create many new session records +by sending repeated requests, potentially filling up the session store or +causing other users' session records to be evicted. + +The :class:`~django.contrib.sessions.middleware.SessionMiddleware` has been +modified to no longer create empty session records. + +Additionally, the ``contrib.sessions.backends.base.SessionBase.flush()`` and +``cache_db.SessionStore.flush()`` methods have been modified to avoid creating +a new empty session. Maintainers of third-party session backends should check +if the same vulnerability is present in their backend and correct it if so. diff --git a/docs/topics/http/sessions.txt b/docs/topics/http/sessions.txt index 85431b5b1c..f261a27f24 100644 --- a/docs/topics/http/sessions.txt +++ b/docs/topics/http/sessions.txt @@ -226,12 +226,18 @@ You can edit it multiple times. .. method:: flush() - Delete the current session data from the session and regenerate the - session key value that is sent back to the user in the cookie. This is - used if you want to ensure that the previous session data can't be - accessed again from the user's browser (for example, the + Deletes the current session data from the session and deletes the session + cookie. This is used if you want to ensure that the previous session data + can't be accessed again from the user's browser (for example, the :func:`django.contrib.auth.logout()` function calls it). + .. versionchanged:: 1.7.10 + + Deletion of the session cookie was added. Previously, the behavior + was to regenerate the session key value that was sent back to the + user in the cookie, but this could be a denial-of-service + vulnerability. + .. method:: set_test_cookie() Sets a test cookie to determine whether the user's browser supports |