CVE-2021-43791: Insufficient Session Expiration (CWE-613)
value: Zulip is an open source group chat application that combines real-time chat with threaded conversations. In affected versions expiration dates on the confirmation objects associated with email invitations were not enforced properly in the new account registration flow. A confirmation link takes a user to the check_prereg_key_and_redirect endpoint, before getting redirected to POST to /accounts/register/. The problem was that validation was happening in the check_prereg_key_and_redirect part and not in /accounts/register/ - meaning that one could submit an expired confirmation key and be able to register. The issue is fixed in Zulip 4.8. There are no known workarounds and users are advised to upgrade as soon as possible.
CWE description: According to WASC, "Insufficient Session Expiration is when a web site permits an attacker to reuse old session credentials or session IDs for authorization."
CVSS v3 base score: 5.3
Published: 2021-12-02T01:15Z
Repository: https://github.com/zulip/zulip
Language: Python
Files changed: confirmation/models.py, zerver/tests/test_signup.py, zerver/views/registration.py, zproject/urls.py
Commit date: 2021-12-01 23:14:04 +0000
Commit message: CVE-2021-43791: Validate confirmation keys in /accounts/register/ codepath.

A confirmation link takes a user to the check_prereg_key_and_redirect
endpoint, before getting redirected to POST to /accounts/register/. The
problem was that validation was happening in the check_prereg_key_and_redirect
part and not in /accounts/register/ - meaning that one could submit an
expired confirmation key and be able to register.

We fix this by moving validation into /accouts/register/.

Diff: diff --git a/confirmation/models.py b/confirmation/models.py
index 4081e80261c..16402014881 100644
--- a/confirmation/models.py
+++ b/confirmation/models.py
@@ -168,23 +168,23 @@ class ConfirmationType:
         self.url_name = url_name
         self.validity_in_days = validity_in_days
 
 
 _properties = {
-    Confirmation.USER_REGISTRATION: ConfirmationType("check_prereg_key_and_redirect"),
+    Confirmation.USER_REGISTRATION: ConfirmationType("get_prereg_key_and_redirect"),
     Confirmation.INVITATION: ConfirmationType(
-        "check_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS
+        "get_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS
     ),
     Confirmation.EMAIL_CHANGE: ConfirmationType("confirm_email_change"),
     Confirmation.UNSUBSCRIBE: ConfirmationType(
         "unsubscribe",
         validity_in_days=1000000,  # should never expire
     ),
     Confirmation.MULTIUSE_INVITE: ConfirmationType(
         "join", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS
     ),
-    Confirmation.REALM_CREATION: ConfirmationType("check_prereg_key_and_redirect"),
+    Confirmation.REALM_CREATION: ConfirmationType("get_prereg_key_and_redirect"),
     Confirmation.REALM_REACTIVATION: ConfirmationType("realm_reactivation"),
 }
 
 
 def one_click_unsubscribe_link(user_profile: UserProfile, email_type: str) -> str:
diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py
index 1970d3d3d9a..6e9d220fc9a 100644
--- a/zerver/tests/test_signup.py
+++ b/zerver/tests/test_signup.py
@@ -853,11 +853,11 @@ class LoginTest(ZulipTestCase):
         ContentType.objects.clear_cache()
 
         with queries_captured() as queries, cache_tries_captured() as cache_tries:
             self.register(self.nonreg_email("test"), "test")
         # Ensure the number of queries we make is not O(streams)
-        self.assert_length(queries, 89)
+        self.assert_length(queries, 91)
 
         # We can probably avoid a couple cache hits here, but there doesn't
         # seem to be any O(N) behavior.  Some of the cache hits are related
         # to sending messages, such as getting the welcome bot, looking up
         # the alert words for a realm, etc.
@@ -2012,12 +2012,11 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         self.assertEqual(cm.exception.error_type, ConfirmationKeyException.DOES_NOT_EXIST)
 
         # Verify that using the wrong type doesn't work in the main confirm code path
         email_change_url = create_confirmation_link(prereg_user, Confirmation.EMAIL_CHANGE)
         email_change_key = email_change_url.split("/")[-1]
-        url = "/accounts/do_confirm/" + email_change_key
-        result = self.client_get(url)
+        result = self.client_post("/accounts/register/", {"key": email_change_key})
         self.assertEqual(result.status_code, 404)
         self.assert_in_response(
             "Whoops. We couldn't find your confirmation link in the system.", result
         )
 
@@ -2030,12 +2029,21 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         )
         date_sent = timezone_now() - datetime.timedelta(weeks=3)
         with patch("confirmation.models.timezone_now", return_value=date_sent):
             url = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION)
 
-        target_url = "/" + url.split("/", 3)[3]
-        result = self.client_get(target_url)
+        key = url.split("/")[-1]
+        confirmation_link_path = "/" + url.split("/", 3)[3]
+        # Both the confirmation link and submitting the key to the registration endpoint
+        # directly will return the appropriate error.
+        result = self.client_get(confirmation_link_path)
+        self.assertEqual(result.status_code, 404)
+        self.assert_in_response(
+            "Whoops. The confirmation link has expired or been deactivated.", result
+        )
+
+        result = self.client_post("/accounts/register/", {"key": key})
         self.assertEqual(result.status_code, 404)
         self.assert_in_response(
             "Whoops. The confirmation link has expired or been deactivated.", result
         )
 
@@ -2122,11 +2130,13 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         url = "/accounts/register/"
         response = self.client_post(
             url, {"key": registration_key, "from_confirmation": 1, "full_nme": "alice"}
         )
         self.assertEqual(response.status_code, 404)
-        self.assert_in_response("The registration link has expired or is not valid.", response)
+        self.assert_in_response(
+            "Whoops. We couldn't find your confirmation link in the system.", response
+        )
 
         registration_key = confirmation_link.split("/")[-1]
         response = self.client_post(
             url, {"key": registration_key, "from_confirmation": 1, "full_nme": "alice"}
         )
diff --git a/zerver/views/registration.py b/zerver/views/registration.py
index 22ab3c13f88..29c374cab1a 100644
--- a/zerver/views/registration.py
+++ b/zerver/views/registration.py
@@ -1,8 +1,8 @@
 import logging
 import urllib
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, List, Optional, Union
 from urllib.parse import urlencode
 
 from django.conf import settings
 from django.contrib.auth import authenticate, get_backends
 from django.core import validators
@@ -92,14 +92,40 @@ if settings.BILLING_ENABLED:
     from corporate.lib.registration import check_spare_licenses_available_for_registering_new_user
     from corporate.lib.stripe import LicenseLimitError
 
 
 @has_request_variables
-def check_prereg_key_and_redirect(
+def get_prereg_key_and_redirect(
     request: HttpRequest, confirmation_key: str, full_name: Optional[str] = REQ(default=None)
 ) -> HttpResponse:
-    confirmation = Confirmation.objects.filter(confirmation_key=confirmation_key).first()
+    key_check_result = check_prereg_key(request, confirmation_key)
+    if isinstance(key_check_result, HttpResponse):
+        return key_check_result
+    # confirm_preregistrationuser.html just extracts the confirmation_key
+    # (and GET parameters) and redirects to /accounts/register, so that the
+    # user can enter their information on a cleaner URL.
+    return render(
+        request,
+        "confirmation/confirm_preregistrationuser.html",
+        context={"key": confirmation_key, "full_name": full_name},
+    )
+
+
+def check_prereg_key(
+    request: HttpRequest, confirmation_key: str
+) -> Union[Confirmation, HttpResponse]:
+    """
+    Checks if the Confirmation key is valid, returning the Confirmation object in case of success
+    and an appropriate error page otherwise.
+    """
+    try:
+        confirmation: Optional[Confirmation] = Confirmation.objects.get(
+            confirmation_key=confirmation_key
+        )
+    except Confirmation.DoesNotExist:
+        confirmation = None
+
     if confirmation is None or confirmation.type not in [
         Confirmation.USER_REGISTRATION,
         Confirmation.INVITATION,
         Confirmation.REALM_CREATION,
     ]:
@@ -115,18 +141,11 @@ def check_prereg_key_and_redirect(
     try:
         get_object_from_key(confirmation_key, confirmation.type, activate_object=False)
     except ConfirmationKeyException as exception:
         return render_confirmation_key_error(request, exception)
 
-    # confirm_preregistrationuser.html just extracts the confirmation_key
-    # (and GET parameters) and redirects to /accounts/register, so that the
-    # user can enter their information on a cleaner URL.
-    return render(
-        request,
-        "confirmation/confirm_preregistrationuser.html",
-        context={"key": confirmation_key, "full_name": full_name},
-    )
+    return confirmation
 
 
 @require_post
 @has_request_variables
 def accounts_register(
@@ -137,19 +156,16 @@ def accounts_register(
     form_full_name: Optional[str] = REQ("full_name", default=None),
     source_realm_id: Optional[int] = REQ(
         default=None, converter=to_converted_or_fallback(to_non_negative_int, None)
     ),
 ) -> HttpResponse:
-    try:
-        confirmation = Confirmation.objects.get(confirmation_key=key)
-    except Confirmation.DoesNotExist:
-        return render(request, "zerver/confirmation_link_expired_error.html", status=404)
+    key_check_result = check_prereg_key(request, key)
+    if isinstance(key_check_result, HttpResponse):
+        return key_check_result
 
-    prereg_user = confirmation.content_object
+    prereg_user = key_check_result.content_object
     assert prereg_user is not None
-    if prereg_user.status == confirmation_settings.STATUS_REVOKED:
-        return render(request, "zerver/confirmation_link_expired_error.html", status=404)
     email = prereg_user.email
     realm_creation = prereg_user.realm_creation
     password_required = prereg_user.password_required
 
     role = prereg_user.invited_as
diff --git a/zproject/urls.py b/zproject/urls.py
index 780ae05f796..650da67f1f1 100644
--- a/zproject/urls.py
+++ b/zproject/urls.py
@@ -126,13 +126,13 @@ from zerver.views.realm_logo import delete_logo_backend, get_logo_backend, uploa
 from zerver.views.realm_playgrounds import add_realm_playground, delete_realm_playground
 from zerver.views.registration import (
     accounts_home,
     accounts_home_from_multiuse_invite,
     accounts_register,
-    check_prereg_key_and_redirect,
     create_realm,
     find_account,
+    get_prereg_key_and_redirect,
     realm_redirect,
 )
 from zerver.views.report import (
     report_csp_violations,
     report_error,
@@ -557,12 +557,12 @@ i18n_urls = [
         name="new_realm_send_confirm",
     ),
     path("accounts/register/", accounts_register, name="accounts_register"),
     path(
         "accounts/do_confirm/<confirmation_key>",
-        check_prereg_key_and_redirect,
-        name="check_prereg_key_and_redirect",
+        get_prereg_key_and_redirect,
+        name="get_prereg_key_and_redirect",
     ),
     path(
         "accounts/confirm_new_email/<confirmation_key>",
         confirm_email_change,
         name="confirm_email_change",

Vulnerable code:     Confirmation.USER_REGISTRATION: ConfirmationType("check_prereg_key_and_redirect"),
        "check_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS
    Confirmation.REALM_CREATION: ConfirmationType("check_prereg_key_and_redirect"),
        self.assert_length(queries, 89)
        url = "/accounts/do_confirm/" + email_change_key
        result = self.client_get(url)
        target_url = "/" + url.split("/", 3)[3]
        result = self.client_get(target_url)
        self.assert_in_response("The registration link has expired or is not valid.", response)
from typing import Any, Dict, List, Optional
def check_prereg_key_and_redirect(
    confirmation = Confirmation.objects.filter(confirmation_key=confirmation_key).first()
    # confirm_preregistrationuser.html just extracts the confirmation_key
    # (and GET parameters) and redirects to /accounts/register, so that the
    # user can enter their information on a cleaner URL.
    return render(
        request,
        "confirmation/confirm_preregistrationuser.html",
        context={"key": confirmation_key, "full_name": full_name},
    )
    try:
        confirmation = Confirmation.objects.get(confirmation_key=key)
    except Confirmation.DoesNotExist:
        return render(request, "zerver/confirmation_link_expired_error.html", status=404)
    prereg_user = confirmation.content_object
    if prereg_user.status == confirmation_settings.STATUS_REVOKED:
        return render(request, "zerver/confirmation_link_expired_error.html", status=404)
    check_prereg_key_and_redirect,
        check_prereg_key_and_redirect,
        name="check_prereg_key_and_redirect",
Fixed code:     Confirmation.USER_REGISTRATION: ConfirmationType("get_prereg_key_and_redirect"),
        "get_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS
    Confirmation.REALM_CREATION: ConfirmationType("get_prereg_key_and_redirect"),
        self.assert_length(queries, 91)
        result = self.client_post("/accounts/register/", {"key": email_change_key})
        key = url.split("/")[-1]
        confirmation_link_path = "/" + url.split("/", 3)[3]
        # Both the confirmation link and submitting the key to the registration endpoint
        # directly will return the appropriate error.
        result = self.client_get(confirmation_link_path)
        self.assertEqual(result.status_code, 404)
        self.assert_in_response(
            "Whoops. The confirmation link has expired or been deactivated.", result
        )

        result = self.client_post("/accounts/register/", {"key": key})
        self.assert_in_response(
            "Whoops. We couldn't find your confirmation link in the system.", response
        )
from typing import Any, Dict, List, Optional, Union
def get_prereg_key_and_redirect(
    key_check_result = check_prereg_key(request, confirmation_key)
    if isinstance(key_check_result, HttpResponse):
        return key_check_result
    # confirm_preregistrationuser.html just extracts the confirmation_key
    # (and GET parameters) and redirects to /accounts/register, so that the
    # user can enter their information on a cleaner URL.
    return render(
        request,
        "confirmation/confirm_preregistrationuser.html",
        context={"key": confirmation_key, "full_name": full_name},
    )


def check_prereg_key(
    request: HttpRequest, confirmation_key: str
) -> Union[Confirmation, HttpResponse]:
    """
    Checks if the Confirmation key is valid, returning the Confirmation object in case of success
    and an appropriate error page otherwise.
    """
    try:
        confirmation: Optional[Confirmation] = Confirmation.objects.get(
            confirmation_key=confirmation_key
        )
    except Confirmation.DoesNotExist:
        confirmation = None

    return confirmation
    key_check_result = check_prereg_key(request, key)
    if isinstance(key_check_result, HttpResponse):
        return key_check_result
    prereg_user = key_check_result.content_object
    get_prereg_key_and_redirect,
        get_prereg_key_and_redirect,
        name="get_prereg_key_and_redirect",