Skip to content
Permalink
Browse files Browse the repository at this point in the history
CVE-2021-43791: Validate confirmation keys in /accounts/register/ cod…
…epath.

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/.
  • Loading branch information
mateuszmandera authored and alexmv committed Dec 1, 2021
1 parent a1cd660 commit a014ef7
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 30 deletions.
6 changes: 3 additions & 3 deletions confirmation/models.py
Expand Up @@ -170,9 +170,9 @@ def __init__(


_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(
Expand All @@ -182,7 +182,7 @@ def __init__(
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"),
}

Expand Down
22 changes: 16 additions & 6 deletions zerver/tests/test_signup.py
Expand Up @@ -855,7 +855,7 @@ def test_register(self) -> None:
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
Expand Down Expand Up @@ -2014,8 +2014,7 @@ def test_confirmation_key_of_wrong_type(self) -> None:
# 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
Expand All @@ -2032,8 +2031,17 @@ def test_confirmation_expired(self) -> None:
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
Expand Down Expand Up @@ -2124,7 +2132,9 @@ def test_confirmation_obj_not_exist_error(self) -> None:
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(
Expand Down
52 changes: 34 additions & 18 deletions zerver/views/registration.py
@@ -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
Expand Down Expand Up @@ -94,10 +94,36 @@


@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,
Expand All @@ -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
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions zproject/urls.py
Expand Up @@ -128,9 +128,9 @@
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 (
Expand Down Expand Up @@ -559,8 +559,8 @@
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>",
Expand Down

0 comments on commit a014ef7

Please sign in to comment.