paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-06 15:03:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

CVE-2022-24751: Clear sessions outside of the transaction.

Browse Source 
Clearing the sessions inside the transaction makes Zulip vulnerable to
a narrow window where the deleted session has not yet been committed,
but has been removed from the memcached cache.  During this window, a
request with the session-id which has just been deleted can
successfully re-fill the memcached cache, as the in-database delete is
not yet committed, and thus not yet visible.  After the delete
transaction commits, the cache will be left with a cached session,
which allows further site access until it expires (after
SESSION_COOKIE_AGE seconds), is ejected from the cache due to memory
pressure, or the server is upgraded.

Move the session deletion outside of the transaction.

Because the testsuite runs inside of a transaction, it is impossible
to test this is CI; the testsuite uses the non-caching
`django.contrib.sessions.backends.db` backend, regardless.  The test
added in this commit thus does not fail before this commit; it is
merely a base expression that the session should be deleted somehow,
and does not exercise the assert added in the previous commit.
This commit is contained in:
Alex Vandiver
2022-03-09 02:03:42 +00:00
parent c28d1169c3
commit e6eace307e
2 changed files with 20 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
zerver/lib/actions.py
View File

@@ -1170,7 +1170,6 @@ def do_deactivate_user(
change_user_is_active(user_profile, False) change_user_is_active(user_profile, False)
delete_user_sessions(user_profile)
clear_scheduled_emails(user_profile.id) clear_scheduled_emails(user_profile.id)
event_time = timezone_now() event_time = timezone_now()
@@ -1196,6 +1195,7 @@ def do_deactivate_user(
if settings.BILLING_ENABLED: if settings.BILLING_ENABLED:
update_license_ledger_if_needed(user_profile.realm, event_time) update_license_ledger_if_needed(user_profile.realm, event_time)
delete_user_sessions(user_profile)
event = dict( event = dict(
type="realm_user", type="realm_user",
op="remove", op="remove",

19
zerver/tests/test_users.py
View File

@@ -6,6 +6,7 @@ from unittest import mock
import orjson import orjson
from django.conf import settings from django.conf import settings
from django.contrib.contenttypes.models import ContentType from django.contrib.contenttypes.models import ContentType
from django.contrib.sessions.models import Session
from django.core.exceptions import ValidationError from django.core.exceptions import ValidationError
from django.test import override_settings from django.test import override_settings
from django.utils.timezone import now as timezone_now from django.utils.timezone import now as timezone_now
@@ -1393,6 +1394,24 @@ class ActivateTest(ZulipTestCase):
) )
self.assert_json_error(result, "Insufficient permission") self.assert_json_error(result, "Insufficient permission")
def test_clear_sessions(self) -> None:
user = self.example_user("hamlet")
self.login_user(user)
session_key = self.client.session.session_key
self.assertTrue(session_key)
result = self.client_get("/json/users")
self.assert_json_success(result)
self.assertEqual(Session.objects.filter(pk=session_key).count(), 1)
do_deactivate_user(user, acting_user=None)
self.assertEqual(Session.objects.filter(pk=session_key).count(), 0)
result = self.client_get("/json/users")
self.assert_json_error(
result, "Not logged in: API authentication or user session required", 401
)
def test_clear_scheduled_jobs(self) -> None: def test_clear_scheduled_jobs(self) -> None:
user = self.example_user("hamlet") user = self.example_user("hamlet")
send_future_email( send_future_email(