8000 [1.4.x] Fixed DoS possiblity in contrib.auth.views.logout() · django/django@575f59f · GitHub
[go: up one dir, main page]
Skip to content

Commit 575f59f

Browse files
timgrahamtimgraham
committed
[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 393c0e2, 0885796, and 2dee853 from master Thanks Florian Apolloner and Carl Meyer for review. This is a security fix.
1 parent 8b0d639 commit 575f59f

File tree

6 files changed

+133
-25
lines changed

6 files changed

+133
-25
lines changed

‎django/contrib/sessions/backends/base.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,13 @@ def clear(self):
128128
self.accessed = True
129129
self.modified = True
130130

131+
def is_empty(self):
132+
"Returns True when there is no session_key and the session is empty"
133+
try:
134+
return not bool(self._session_key) and not self._session_cache
135+
except AttributeError:
136+
return True
137+
131138
def _get_new_session_key(self):
132139
"Returns session key that isn't being used."
133140
# Todo: move to 0-9a-z charset in 1.5
@@ -230,7 +237,7 @@ def flush(self):
230237
"""
231238
self.clear()
232239
self.delete()
233-
self.create()
240+
self._session_key = None
234241

235242
def cycle_key(self):
236243
"""

‎django/contrib/sessions/backends/cached_db.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,4 +58,4 @@ def flush(self):
5858
"""
5959
self.clear()
6060
self.delete(self.session_key)
61-
self.create()
61+
self._session_key = None

‎django/contrib/sessions/middleware.py‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,30 +14,38 @@ def process_request(self, request):
1414
def process_response(self, request, response):
1515
"""
1616
If request.session was modified, or if the configuration is to save the
17-
session every time, save the changes and set a session cookie.
17+
session every time, save the changes and set a session cookie or delete
18+
the session cookie if the session has been emptied.
1819
"""
1920
try:
2021
accessed = request.session.accessed
2122
modified = request.session.modified
23+
empty = request.session.is_empty()
2224
except AttributeError:
2325
pass
2426
else:
25-
if accessed:
26-
patch_vary_headers(response, ('Cookie',))
27-
if modified or settings.SESSION_SAVE_EVERY_REQUEST:
28-
if request.session.get_expire_at_browser_close():
29-
max_age = None
30-
expires = None
31-
else:
32-
max_age = request.session.get_expiry_age()
33-
expires_time = time.time() + max_age
34-
expires = cookie_date(expires_time)
35-
# Save the session data and refresh the client cookie.
36-
request.session.save()
37-
response.set_cookie(settings.SESSION_COOKIE_NAME,
38-
request.session.session_key, max_age=max_age,
39-
expires=expires, domain=settings.SESSION_COOKIE_DOMAIN,
40-
path=settings.SESSION_COOKIE_PATH,
41-
secure=settings.SESSION_COOKIE_SECURE or None,
42-
httponly=settings.SESSION_COOKIE_HTTPONLY or None)
27+
# First check if we need to delete this cookie.
28+
# The session should be deleted only if the session is entirely empty
29+
if settings.SESSION_COOKIE_NAME in request.COOKIES and empty:
30+
response.delete_cookie(settings.SESSION_COOKIE_NAME,
31+
domain=settings.SESSION_COOKIE_DOMAIN)
32+
else:
33+
if accessed:
34+
patch_vary_headers(response, ('Cookie',))
35+
if (modified or settings.SESSION_SAVE_EVERY_REQUEST) and not empty:
36+
if request.session.get_expire_at_browser_close():
37+
max_age = None
38+
expires = None
39+
else:
40+
max_age = request.session.get_expiry_age()
41+
expires_time = time.time() + max_age
42+
expires = cookie_date(expires_time)
43+
# Save the session data and refresh the client cookie.
44+
request.session.save()
45+
response.set_cookie(settings.SESSION_COOKIE_NAME,
46+
request.session.session_key, max_age=max_age,
47+
expires=expires, domain=settings.SESSION_COOKIE_DOMAIN,
48+
path=settings.SESSION_COOKIE_PATH,
49+
secure=settings.SESSION_COOKIE_SECURE or None,
50+
httponly=settings.SESSION_COOKIE_HTTPONLY or None)
4351
return response

‎django/contrib/sessions/tests.py‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ def test_flush(self):
150150
self.session.flush()
151151
self.assertFalse(self.session.exists(prev_key))
152152
self.assertNotEqual(self.session.session_key, prev_key)
153+
self.assertIsNone(self.session.session_key)
153154
self.assertTrue(self.session.modified)
154155
self.assertTrue(self.session.accessed)
155156

@@ -432,6 +433,75 @@ def test_no_httponly_session_cookie(self):
432433
self.assertNotIn('httponly',
433434
str(response.cookies[settings.SESSION_COOKIE_NAME]))
434435

436+
def test_session_delete_on_end(self):
437+
request = RequestFactory().get('/')
438+
response = HttpResponse('Session test')
439+
middleware = SessionMiddleware()
440+
441+
# Before deleting, there has to be an existing cookie
442+
request.COOKIES[settings.SESSION_COOKIE_NAME] = 'abc'
443+
444+
# Simulate a request that ends the session
445+
middleware.process_request(request)
446+
request.session.flush()
447+
448+
# Handle the response through the middleware
449+
response = middleware.process_response(request, response)
450+
451+
# Check that the cookie was deleted, not recreated.
452+
# A deleted cookie header looks like:
453+
# Set-Cookie: sessionid=; expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/
454+
self.assertEqual(
455+
'Set-Cookie: %s=; expires=Thu, 01-Jan-1970 00:00:00 GMT; '
456+
'Max-Age=0; Path=/' % settings.SESSION_COOKIE_NAME,
457+
str(response.cookies[settings.SESSION_COOKIE_NAME])
458+
)
459+
460+
@override_settings(SESSION_COOKIE_DOMAIN='.example.local')
461+
def test_session_delete_on_end_with_custom_domain(self):
462+
request = RequestFactory().get('/')
463+
response = HttpResponse('Session test')
464+
middleware = SessionMiddleware()
465+
466+
# Before deleting, there has to be an existing cookie
467+
request.COOKIES[settings.SESSION_COOKIE_NAME] = 'abc'
468+
469+
# Simulate a request that ends the session
470+
middleware.process_request(request)
471+
request.session.flush()
472+
473+
# Handle the response through the middleware
474+
response = middleware.process_response(request, response)
475+
476+
# Check that the cookie was deleted, not recreated.
477+
# A deleted cookie header with a custom domain looks like:
478+
# Set-Cookie: sessionid=; Domain=.example.local;
479+
# expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/
480+
self.assertEqual(
481+
'Set-Cookie: %s=; Domain=.example.local; expires=Thu, '
482+
'01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/' % (
483+
settings.SESSION_COOKIE_NAME,
484+
),
485+
str(response.cookies[settings.SESSION_COOKIE_NAME])
486+
)
487+
488+
def test_flush_empty_without_session_cookie_doesnt_set_cookie(self):
489+
request = RequestFactory().get('/')
490+
response = HttpResponse('Session test')
491+
middleware = SessionMiddleware()
492+
493+
# Simulate a request that ends the session
494+
middleware.process_request(request)
495+
request.session.flush()
496+
497+
# Handle the response through the middleware
498+
response = middleware.process_response(request, response)
499+
500+
# A cookie should not be set.
501+
self.assertEqual(response.cookies, {})
502+
# The session is accessed so "Vary: Cookie" should be set.
503+
self.assertEqual(response['Vary'], 'Cookie')
504+
435505

436506
class CookieSessionTests(SessionTestsMixin, TestCase):
437507

‎docs/releases/1.4.22.txt‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,21 @@ Django 1.4.22 fixes a security issue in 1.4.21.
99
It also fixes support with pip 7+ by disabling wheel support. Older versions
1010
of 1.4 would silently build a broken wheel when installed with those versions
1111
of pip.
12+
13+
Denial-of-service possibility in ``logout()`` view by filling session store
14+
===========================================================================
15+
16+
Previously, a session could be created when anonymously accessing the
17+
:func:`django.contrib.auth.views.logout` view (provided it wasn't decorated
18+
with :func:`~django.contrib.auth.decorators.login_required` as done in the
19+
admin). This could allow an attacker to easily create many new session records
20+
by sending repeated requests, potentially filling up the session store or
21+
causing other users' session records to be evicted.
22+
23+
The :class:`~django.contrib.sessions.middleware.SessionMiddleware` has been
24+
modified to no longer create empty session records.
25+
26+
Additionally, the ``contrib.sessions.backends.base.SessionBase.flush()`` and
27+
``cache_db.SessionStore.flush()`` methods have been modified to avoid creating
28+
a new empty session. Maintainers of third-party session backends should check
29+
if the same vulnerability is present in their backend and correct it if so.

‎docs/topics/http/sessions.txt‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -197,12 +197,17 @@ You can edit it multiple times.
197197

198198
.. method:: flush
199199

200-
Delete the current session data from the session and regenerate the
201-
session key value that is sent back to the user in the cookie. This is
202-
used if you want to ensure that the previous session data can't be
203-
accessed again from the user's browser (for example, the
200+
Deletes the current session data from the session and deletes the session
201+
cookie. This is used if you want to ensure that the previous session data
202+
can't be accessed again from the user's browser (for example, the
204203
:func:`django.contrib.auth.logout()` function calls it).
205204

205+
.. versionchanged:: 1.4.22
206+
207+
Deletion of the session cookie was added. Previously, the behavior
208+
was to regenerate the session key value that was sent back to the
209+
user in the cookie, but this was a denial-of-service vulnerability.
210+
206211
.. method:: set_test_cookie
207212

208213
Sets a test cookie to determine whether the user's browser supports

0 commit comments

Comments
 (0)
0