Skip to content
Permalink
Browse files Browse the repository at this point in the history
CVE-2022-24751: Clear sessions outside of the transaction.
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.
  • Loading branch information
alexmv committed Mar 15, 2022
1 parent 7650b5a commit 62ba8e4
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 1 deletion.
2 changes: 1 addition & 1 deletion zerver/lib/actions.py
Expand Up @@ -1414,7 +1414,6 @@ def do_deactivate_user(

change_user_is_active(user_profile, False)

delete_user_sessions(user_profile)
clear_scheduled_emails(user_profile.id)
revoke_invites_generated_by_user(user_profile)

Expand All @@ -1441,6 +1440,7 @@ def do_deactivate_user(
if settings.BILLING_ENABLED:
update_license_ledger_if_needed(user_profile.realm, event_time)

delete_user_sessions(user_profile)
event = dict(
type="realm_user",
op="remove",
Expand Down
19 changes: 19 additions & 0 deletions zerver/tests/test_users.py
Expand Up @@ -6,6 +6,7 @@
import orjson
from django.conf import settings
from django.contrib.contenttypes.models import ContentType
from django.contrib.sessions.models import Session
from django.core.exceptions import ValidationError
from django.test import override_settings
from django.utils.timezone import now as timezone_now
Expand Down Expand Up @@ -1570,6 +1571,24 @@ def test_revoke_invites(self) -> None:
).expiry_date
)

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:
user = self.example_user("hamlet")
send_future_email(
Expand Down

0 comments on commit 62ba8e4

Please sign in to comment.