Commit 2f548534 authored by Tim Graham's avatar Tim Graham
Browse files

[1.7.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 393c0e24,
08857963, and
2dee853e from master

Thanks Florian Apolloner and Carl Meyer for review.

This is a security fix.
parent 95af8946

django/contrib/sessions/backends/base.py

+8 −1
Original line number Diff line number Diff line
@@ -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):

        """

django/contrib/sessions/backends/cached_db.py

+1 −1
Original line number Diff line number Diff line
@@ -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

django/contrib/sessions/middleware.py

+29 −21
Original line number Diff line number Diff line
@@ -18,17 +18,25 @@ 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:

            # 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:

                if (modified or settings.SESSION_SAVE_EVERY_REQUEST) and not empty:

                    if request.session.get_expire_at_browser_close():

                        max_age = None

                        expires = None

django/contrib/sessions/tests.py

+70 −0
Original line number Diff line number Diff line
@@ -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):

docs/releases/1.4.22.txt

+18 −0
Original line number Diff line number Diff line
@@ -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.