From 8d2d64c10006801b35f287e92e69da01d0b447cd Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Fri, 12 Jun 2020 16:19:17 +0200 Subject: [PATCH] CVE-2020-14215: Fix validation in PreregistrationUser queries. The most import change here is the one in maybe_send_to_registration codepath, as the insufficient validation there could lead to fetching an expired PreregistrationUser that was invited as an administrator admin even years ago, leading to this registration ending up in the new user being a realm administrator. Combined with the buggy migration in 0198_preregistrationuser_invited_as.py, this led to users incorrectly joining as organizations administrators by accident. But even without that bug, this issue could have allowed a user who was invited as an administrator but then had that invitation expire and then joined via social authentication incorrectly join as an organization administrator. The second change is in ConfirmationEmailWorker, where this wasn't a security problem, but if the server was stopped for long enough, with some invites to send out email for in the queue, then after starting it up again, the queue worker would send out emails for invites that had already expired. --- zerver/lib/actions.py | 17 ++++++++--------- zerver/models.py | 9 +++++++++ zerver/tests/test_auth_backends.py | 23 +++++++++++++++++++++++ zerver/views/auth.py | 13 ++++++++++--- zerver/worker/queue_processors.py | 10 +++++++--- 5 files changed, 57 insertions(+), 15 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 82715eaf9e..489a7936d6 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -200,6 +200,7 @@ from zerver.models import ( active_user_ids, custom_profile_fields_for_realm, email_to_username, + filter_to_valid_prereg_users, get_active_streams, get_bot_dicts_in_realm, get_bot_services, @@ -5129,17 +5130,14 @@ def do_invite_users(user_profile: UserProfile, notify_invites_changed(user_profile) def do_get_user_invites(user_profile: UserProfile) -> List[Dict[str, Any]]: - days_to_activate = settings.INVITATION_LINK_VALIDITY_DAYS - active_value = confirmation_settings.STATUS_ACTIVE - revoked_value = confirmation_settings.STATUS_REVOKED - lowest_datetime = timezone_now() - datetime.timedelta(days=days_to_activate) - base_query = PreregistrationUser.objects.exclude(status__in=[active_value, revoked_value]).filter( - invited_at__gte=lowest_datetime) - if user_profile.is_realm_admin: - prereg_users = base_query.filter(referred_by__realm=user_profile.realm) + prereg_users = filter_to_valid_prereg_users( + PreregistrationUser.objects.filter(referred_by__realm=user_profile.realm) + ) else: - prereg_users = base_query.filter(referred_by=user_profile) + prereg_users = filter_to_valid_prereg_users( + PreregistrationUser.objects.filter(referred_by=user_profile) + ) invites = [] @@ -5151,6 +5149,7 @@ def do_get_user_invites(user_profile: UserProfile) -> List[Dict[str, Any]]: invited_as=invitee.invited_as, is_multiuse=False)) + lowest_datetime = timezone_now() - datetime.timedelta(days=settings.INVITATION_LINK_VALIDITY_DAYS) multiuse_confirmation_objs = Confirmation.objects.filter(realm=user_profile.realm, type=Confirmation.MULTIUSE_INVITE, date_sent__gte=lowest_datetime) diff --git a/zerver/models.py b/zerver/models.py index a0f9294f0a..67da706443 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -35,6 +35,7 @@ from django.db.models.signals import post_delete, post_save from django.utils.timezone import now as timezone_now from django.utils.translation import ugettext_lazy as _ +from confirmation import settings as confirmation_settings from zerver.lib import cache from zerver.lib.cache import ( active_non_guest_user_ids_cache_key, @@ -1357,6 +1358,14 @@ class PreregistrationUser(models.Model): ) invited_as: int = models.PositiveSmallIntegerField(default=INVITE_AS['MEMBER']) +def filter_to_valid_prereg_users(query: QuerySet) -> QuerySet: + days_to_activate = settings.INVITATION_LINK_VALIDITY_DAYS + active_value = confirmation_settings.STATUS_ACTIVE + revoked_value = confirmation_settings.STATUS_REVOKED + lowest_datetime = timezone_now() - datetime.timedelta(days=days_to_activate) + return query.exclude(status__in=[active_value, revoked_value]).filter( + invited_at__gte=lowest_datetime) + class MultiuseInvite(models.Model): referred_by = models.ForeignKey(UserProfile, on_delete=CASCADE) # Optional[UserProfile] streams: Manager = models.ManyToManyField('Stream') diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 6d1c973b9e..d48ee8d2ee 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -1415,6 +1415,29 @@ class SocialAuthBase(DesktopFlowTestingLib, ZulipTestCase): self.assertEqual(result.status_code, 302) self.assertIn('login', result.url) + @override_settings(TERMS_OF_SERVICE=None) + def test_social_auth_invited_as_admin_but_expired(self) -> None: + iago = self.example_user("iago") + email = self.nonreg_email("alice") + name = 'Alice Jones' + + do_invite_users(iago, [email], [], invite_as=PreregistrationUser.INVITE_AS['REALM_ADMIN']) + expired_date = timezone_now() - datetime.timedelta(days=settings.INVITATION_LINK_VALIDITY_DAYS + 1) + PreregistrationUser.objects.filter(email=email).update(invited_at=expired_date) + + subdomain = 'zulip' + realm = get_realm("zulip") + account_data_dict = self.get_account_data_dict(email=email, name=name) + result = self.social_auth_test(account_data_dict, + expect_choose_email_screen=True, + subdomain=subdomain, is_signup=True) + self.stage_two_of_registration(result, realm, subdomain, email, name, name, + self.BACKEND_CLASS.full_name_validated) + + # The invitation is expired, so the user should be created as normal member only. + created_user = get_user_by_delivery_email(email, realm) + self.assertEqual(created_user.role, UserProfile.ROLE_MEMBER) + class SAMLAuthBackendTest(SocialAuthBase): __unittest_skip__ = False diff --git a/zerver/views/auth.py b/zerver/views/auth.py index c73243d934..4ee1b3dc98 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -50,7 +50,14 @@ from zerver.lib.user_agent import parse_user_agent from zerver.lib.users import get_api_key from zerver.lib.utils import has_api_key_format from zerver.lib.validator import validate_login_email -from zerver.models import PreregistrationUser, Realm, UserProfile, get_realm, remote_user_to_email +from zerver.models import ( + PreregistrationUser, + Realm, + UserProfile, + filter_to_valid_prereg_users, + get_realm, + remote_user_to_email, +) from zerver.signals import email_on_new_login from zproject.backends import ( AUTH_BACKEND_NAME_MAP, @@ -159,8 +166,8 @@ def maybe_send_to_registration(request: HttpRequest, email: str, full_name: str= # creation or confirm-continue-registration depending on # is_signup. try: - prereg_user = PreregistrationUser.objects.filter( - email__iexact=email, realm=realm).latest("invited_at") + prereg_user = filter_to_valid_prereg_users(PreregistrationUser.objects.filter( + email__iexact=email, realm=realm)).latest("invited_at") # password_required and full_name data passed here as argument should take precedence # over the defaults with which the existing PreregistrationUser that we've just fetched diff --git a/zerver/worker/queue_processors.py b/zerver/worker/queue_processors.py index 66c51d4399..64ca9eff38 100644 --- a/zerver/worker/queue_processors.py +++ b/zerver/worker/queue_processors.py @@ -89,6 +89,7 @@ from zerver.models import ( RealmAuditLog, UserMessage, UserProfile, + filter_to_valid_prereg_users, flush_per_request_caches, get_bot_services, get_client, @@ -324,10 +325,13 @@ class ConfirmationEmailWorker(QueueProcessingWorker): if "email" in data: # When upgrading from a version up through 1.7.1, there may be # existing items in the queue with `email` instead of `prereg_id`. - invitee = PreregistrationUser.objects.filter( - email__iexact=data["email"].strip()).latest("invited_at") + invitee = filter_to_valid_prereg_users( + PreregistrationUser.objects.filter(email__iexact=data["email"].strip()) + ).latest("invited_at") else: - invitee = PreregistrationUser.objects.filter(id=data["prereg_id"]).first() + invitee = filter_to_valid_prereg_users( + PreregistrationUser.objects.filter(id=data["prereg_id"]) + ).first() if invitee is None: # The invitation could have been revoked return