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/.
This commit is contained in:
Mateusz Mandera 2021-11-29 16:20:59 +01:00 committed by Alex Vandiver
parent a1cd660147
commit a014ef75a3
4 changed files with 56 additions and 30 deletions

View File

@ -170,9 +170,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(
@ -182,7 +182,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"),
}

View File

@ -855,7 +855,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.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
@ -2014,8 +2014,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
@ -2032,8 +2031,17 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
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
@ -2124,7 +2132,9 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
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(

View File

@ -1,6 +1,6 @@
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
@ -94,10 +94,36 @@ if settings.BILLING_ENABLED:
@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,
@ -117,14 +143,7 @@ def check_prereg_key_and_redirect(
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
@ -139,15 +158,12 @@ def accounts_register(
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

View File

@ -128,9 +128,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 (
@ -559,8 +559,8 @@ i18n_urls = [
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>",