CVE-2022-24751: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: Zulip is an open source group chat application. Starting with version 4.0 and prior to version 4.11, Zulip is vulnerable to a race condition during account deactivation, where a simultaneous access by the user being deactivated may, in rare cases, allow continued access by the deactivated user. A patch is available in version 4.11 on the 4.x branch and version 5.0-rc1 on the 5.x branch. Upgrading to a fixed version will, as a side effect, deactivate any cached sessions that may have been leaked through this bug. There are currently no known workarounds.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 7.4
Published: 2022-03-16T14:15Z
Repository: https://github.com/zulip/zulip
Language: Python
Files changed: zerver/lib/actions.py, zerver/tests/test_users.py
Commit date: 2022-03-15 13:52:15 -0700
Commit message: 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.

Diff: diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py
index e8b29e98603..2b6087deac9 100644
--- a/zerver/lib/actions.py
+++ b/zerver/lib/actions.py
@@ -1412,11 +1412,10 @@ def do_deactivate_user(
             user_profile.is_mirror_dummy = True
             user_profile.save(update_fields=["is_mirror_dummy"])
 
         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)
 
         event_time = timezone_now()
         RealmAuditLog.objects.create(
@@ -1439,10 +1438,11 @@ def do_deactivate_user(
             increment=-1,
         )
         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",
         person=dict(user_id=user_profile.id, full_name=user_profile.full_name),
     )
diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py
index 18fa1412384..a249337eb16 100644
--- a/zerver/tests/test_users.py
+++ b/zerver/tests/test_users.py
@@ -4,10 +4,11 @@ from typing import Any, Dict, Iterable, List, Mapping, Optional, TypeVar, Union
 from unittest import mock
 
 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
 
 from confirmation.models import Confirmation
@@ -1568,10 +1569,28 @@ class ActivateTest(ZulipTestCase):
             Confirmation.objects.get(
                 confirmation_key=desdemona_never_expire_multiuse_key
             ).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(
             "zerver/emails/followup_day1",
             user.realm,

Vulnerable code:         delete_user_sessions(user_profile)
Fixed code:     delete_user_sessions(user_profile)
from django.contrib.sessions.models import Session
    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
        )