From 575f59f9bc7c59a5e41a081d1f5f55fc859c5012 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Wed, 5 Aug 2015 17:44:48 -0400 Subject: [PATCH] [1.4.x] Fixed DoS possiblity in contrib.auth.views.logout() Refs #20936 -- When logging out/ending a session, don't create a new, empty session. Previously, when logging out, the existing session was overwritten by a new sessionid instead of deleting the session altogether. This behavior added overhead by creating a new session record in whichever backend was in use: db, cache, etc. This extra session is unnecessary at the time since no session data is meant to be preserved when explicitly logging out. Backport of 393c0e24223c701edeb8ce7dc9d0f852f0c081ad, 088579638b160f3716dc81d194be70c72743593f, and 2dee853ed4def42b7ef1b3b472b395055543cc00 from master Thanks Florian Apolloner and Carl Meyer for review. This is a security fix. --- django/contrib/sessions/backends/base.py | 9 ++- django/contrib/sessions/backends/cached_db.py | 2 +- django/contrib/sessions/middleware.py | 46 +++++++----- django/contrib/sessions/tests.py | 70 +++++++++++++++++++ docs/releases/1.4.22.txt | 18 +++++ docs/topics/http/sessions.txt | 13 ++-- 6 files changed, 133 insertions(+), 25 deletions(-) diff --git a/django/contrib/sessions/backends/base.py b/django/contrib/sessions/backends/base.py index 5a637e24d2..08e65e965b 100644 --- a/django/contrib/sessions/backends/base.py +++ b/django/contrib/sessions/backends/base.py @@ -128,6 +128,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." # Todo: move to 0-9a-z charset in 1.5 @@ -230,7 +237,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 e15f88358c..b145fd145a 100644 --- a/django/contrib/sessions/backends/cached_db.py +++ b/django/contrib/sessions/backends/cached_db.py @@ -58,4 +58,4 @@ class SessionStore(DBStore): """ self.clear() self.delete(self.session_key) - self.create() + self._session_key = None diff --git a/django/contrib/sessions/middleware.py b/django/contrib/sessions/middleware.py index 68cb77f7e1..256a845d7b 100644 --- a/django/contrib/sessions/middleware.py +++ b/django/contrib/sessions/middleware.py @@ -14,30 +14,38 @@ 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. - 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. + 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 98271f4b0e..2b269437aa 100644 --- a/django/contrib/sessions/tests.py +++ b/django/contrib/sessions/tests.py @@ -150,6 +150,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) @@ -432,6 +433,75 @@ class SessionMiddlewareTests(unittest.TestCase): self.assertNotIn('httponly', str(response.cookies[settings.SESSION_COOKIE_NAME])) + 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: %s=; expires=Thu, 01-Jan-1970 00:00:00 GMT; ' + 'Max-Age=0; Path=/' % 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: %s=; Domain=.example.local; expires=Thu, ' + '01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/' % ( + 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/topics/http/sessions.txt b/docs/topics/http/sessions.txt index bb9d73af97..2849582cd3 100644 --- a/docs/topics/http/sessions.txt +++ b/docs/topics/http/sessions.txt @@ -197,12 +197,17 @@ 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.4.22 + + 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 was a denial-of-service vulnerability. + .. method:: set_test_cookie Sets a test cookie to determine whether the user's browser supports