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/.

confirmation/models.py

@@ -143,9 +143,9 @@ class ConfirmationType:
 
 
 _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(
@@ -155,7 +155,7 @@ _properties = {
     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"),
 }
 

zerver/tests/test_signup.py

@@ -773,7 +773,7 @@ class LoginTest(ZulipTestCase):
         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.assertEqual(len(queries), 70)
+        self.assertEqual(len(queries), 72)
 
         # 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
@@ -1876,8 +1876,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         # 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_post("/accounts/register/", {"key": email_change_key})
-        result = self.client_get(url)
         self.assertEqual(result.status_code, 404)
         self.assert_in_response(
             "Whoops. We couldn't find your confirmation link in the system.", result
@@ -1897,8 +1896,17 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         conf.date_sent -= datetime.timedelta(weeks=3)
         conf.save()
 
-        target_url = "/" + url.split("/", 3)[3]
+        key = url.split("/")[-1]
-        result = self.client_get(target_url)
+        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
@@ -1970,9 +1978,9 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         response = self.client_post(
             url, {"key": registration_key, "from_confirmation": 1, "full_nme": "alice"}
         )
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 404)
-        self.assert_in_success_response(
+        self.assert_in_response(
-            ["The registration link has expired or is not valid."], response
+            "Whoops. We couldn't find your confirmation link in the system.", response
         )
 
         registration_key = confirmation_link.split("/")[-1]
@@ -3569,10 +3577,8 @@ class UserSignUpTest(InviteUserBase):
             },
         )
         # Error page should be displayed
-        self.assert_in_success_response(
+        self.assert_in_response("The registration link has expired or is not valid.", result)
-            ["The registration link has expired or is not valid."], result
+        self.assertEqual(result.status_code, 404)
-        )
-        self.assertEqual(result.status_code, 200)
 
     def test_signup_with_multiple_default_stream_groups(self) -> None:
         # Check if user is subscribed to the streams of default

zerver/views/registration.py

@@ -1,6 +1,6 @@
 import logging
 import urllib
-from typing import Dict, List, Optional
+from typing import Dict, List, Optional, Union
 from urllib.parse import urlencode
 
 import pytz
@@ -87,8 +87,34 @@ from zproject.backends import (
 )
 
 
-def check_prereg_key_and_redirect(request: HttpRequest, confirmation_key: str) -> HttpResponse:
+def get_prereg_key_and_redirect(request: HttpRequest, confirmation_key: str) -> 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": request.GET.get("full_name", None)},
+    )
+
+
+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,
@@ -107,27 +133,19 @@ def check_prereg_key_and_redirect(request: HttpRequest, confirmation_key: str) -
     except ConfirmationKeyException as exception:
         return render_confirmation_key_error(request, exception)
 
-    # confirm_preregistrationuser.html just extracts the confirmation_key
+    return confirmation
-    # (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": request.GET.get("full_name", None)},
-    )
 
 
 @require_post
 def accounts_register(request: HttpRequest) -> HttpResponse:
-    try:
+    key = request.POST.get("key", default="")
-        key = request.POST.get("key", default="")
-        confirmation = Confirmation.objects.get(confirmation_key=key)
-    except Confirmation.DoesNotExist:
-        return render(request, "zerver/confirmation_link_expired_error.html")
 
-    prereg_user = confirmation.content_object
+    key_check_result = check_prereg_key(request, key)
-    if prereg_user.status == confirmation_settings.STATUS_REVOKED:
+    if isinstance(key_check_result, HttpResponse):
-        return render(request, "zerver/confirmation_link_expired_error.html")
+        return key_check_result
+
+    prereg_user = key_check_result.content_object
+    assert prereg_user is not None
     email = prereg_user.email
     realm_creation = prereg_user.realm_creation
     password_required = prereg_user.password_required

zproject/urls.py

@@ -126,9 +126,9 @@ 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 (
@@ -570,8 +570,8 @@ i18n_urls = [
     path("accounts/register/", accounts_register, name="accounts_register"),
     path(
         "accounts/do_confirm/<confirmation_key>",
-        check_prereg_key_and_redirect,
+        get_prereg_key_and_redirect,
-        name="check_prereg_key_and_redirect",
+        name="get_prereg_key_and_redirect",
     ),
     path(
         "accounts/confirm_new_email/<confirmation_key>",