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(-) --- 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." # The random module is seeded when this Apache child is created. @@ -237,7 +244,7 @@ class SessionBase(object): """ self.clear() self.delete() - self.create() + self._session_key = None def cycle_key(self): """ --- a/django/contrib/sessions/backends/cached_db.py +++ b/django/contrib/sessions/backends/cached_db.py @@ -46,4 +46,4 @@ class SessionStore(DBStore): """ self.clear() self.delete(self.session_key) - self.create() + self._session_key = None --- a/django/contrib/sessions/middleware.py +++ b/django/contrib/sessions/middleware.py @@ -14,29 +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) + # 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) return response --- a/django/contrib/sessions/tests.py +++ b/django/contrib/sessions/tests.py @@ -7,6 +7,8 @@ r""" >>> from django.contrib.sessions.backends.file import SessionStore as FileSession >>> from django.contrib.sessions.backends.base import SessionBase >>> from django.contrib.sessions.models import Session +>>> from django.contrib.sessions.middleware import SessionMiddleware +>>> from django.http import HttpResponse, HttpRequest >>> db_session = DatabaseSession() >>> db_session.modified @@ -34,6 +36,8 @@ True >>> db_session.flush() >>> db_session.exists(prev_key) False +>>> db_session._session_key is None +True >>> db_session.session_key == prev_key False >>> db_session.modified, db_session.accessed @@ -137,6 +141,8 @@ True >>> file_session.flush() >>> file_session.exists(prev_key) False +>>> file_session._session_key is None +True >>> file_session.session_key == prev_key False >>> file_session.modified, file_session.accessed @@ -442,6 +448,38 @@ True True >>> settings.SESSION_EXPIRE_AT_BROWSER_CLOSE = original_expire_at_browser_close + +# Middleware tests +# Backport of SessionMiddlewareTests.test_session_delete_on_end(): +>>> request = HttpRequest() +>>> 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=/ +>>> '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]) +True + +# Backport of +# SessionMiddlewareTests.test_flush_empty_without_session_cookie_doesnt_set_cookie() +>>> request = HttpRequest() +>>> 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) +>>> response['Vary'] = 'Cookie' +>>> len(response.cookies) +0 """ if __name__ == '__main__': --- a/django/contrib/sessions/backends/db.py +++ b/django/contrib/sessions/backends/db.py @@ -51,7 +51,7 @@ class SessionStore(SessionBase): create a *new* entry (as opposed to possibly updating an existing entry). """ - if self.session_key is None: + if self._session_key is None: return self.create() obj = Session( session_key = self.session_key, --- a/django/contrib/sessions/backends/file.py +++ b/django/contrib/sessions/backends/file.py @@ -74,7 +74,7 @@ class SessionStore(SessionBase): return def save(self, must_create=False): - if self.session_key is None: + 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.