From 551b38716479ccfc4e6a833e609c12a7bbef46de Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Mon, 29 Nov 2021 16:20:59 +0100 Subject: [PATCH] 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 | 6 ++-- zerver/tests/test_signup.py | 30 +++++++++++-------- zerver/views/registration.py | 56 ++++++++++++++++++++++++------------ zproject/urls.py | 6 ++-- 4 files changed, 61 insertions(+), 37 deletions(-) diff --git a/confirmation/models.py b/confirmation/models.py index 937821bf04..22aab10f7c 100644 --- a/confirmation/models.py +++ b/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"), } diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index bd53e482e4..2492cb3ced 100644 --- a/zerver/tests/test_signup.py +++ b/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_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 @@ -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] - 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 @@ -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.assert_in_success_response( - ["The registration link has expired or is not valid."], response + self.assertEqual(response.status_code, 404) + self.assert_in_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( - ["The registration link has expired or is not valid."], result - ) - self.assertEqual(result.status_code, 200) + self.assert_in_response("The registration link has expired or is not valid.", result) + self.assertEqual(result.status_code, 404) def test_signup_with_multiple_default_stream_groups(self) -> None: # Check if user is subscribed to the streams of default diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 18e0055fce..4d2b064291 100644 --- a/zerver/views/registration.py +++ b/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: - confirmation = Confirmation.objects.filter(confirmation_key=confirmation_key).first() +def get_prereg_key_and_redirect(request: HttpRequest, confirmation_key: str) -> HttpResponse: + 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 - # (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)}, - ) + return confirmation @require_post def accounts_register(request: HttpRequest) -> HttpResponse: - try: - 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") + key = request.POST.get("key", default="") - prereg_user = confirmation.content_object - if prereg_user.status == confirmation_settings.STATUS_REVOKED: - return render(request, "zerver/confirmation_link_expired_error.html") + 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 + assert prereg_user is not None email = prereg_user.email realm_creation = prereg_user.realm_creation password_required = prereg_user.password_required diff --git a/zproject/urls.py b/zproject/urls.py index a2064d337e..75c1a5dd60 100644 --- a/zproject/urls.py +++ b/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/", - 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/",