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