From fc85d7d613e2386ebd7d464674db971045470e40 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Mon, 8 Jan 2024 22:19:08 +0100 Subject: [PATCH] auth: Add hardening authenticate(use_dummy_backend=True) in do_login. As explained in the comment, this is to prevent bugs where some strange combination of codepaths could end up calling do_login without basic validation of e.g. the subdomain. The usefulness of this will be extended with the upcoming commit to add the ability to configure custom code to wrap authenticate() calls in. This will help ensure that some codepaths don't slip by the mechanism, ending up logging in a user without the chance for the custom wrapper to run its code. (cherry picked from commit 72bea3433e3fcccf28d3928727230dd517b00700) --- zerver/decorator.py | 29 ++++++++++-- zerver/tests/test_auth_backends.py | 76 +++++++++++++++++++++++++----- zerver/tests/test_signup.py | 5 +- 3 files changed, 92 insertions(+), 18 deletions(-) diff --git a/zerver/decorator.py b/zerver/decorator.py index 7da88ab351..1d16dce4d4 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -19,7 +19,7 @@ from urllib.parse import urlsplit import django_otp from django.conf import settings -from django.contrib.auth import REDIRECT_FIELD_NAME +from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate from django.contrib.auth import login as django_login from django.contrib.auth.decorators import user_passes_test as django_user_passes_test from django.contrib.auth.models import AbstractBaseUser, AnonymousUser @@ -36,6 +36,7 @@ from django_otp import user_has_device from two_factor.utils import default_device from typing_extensions import Concatenate, ParamSpec +from zerver.context_processors import get_valid_realm_from_request from zerver.lib.exceptions import ( AccessDeniedError, AnomalousWebhookPayloadError, @@ -463,12 +464,30 @@ def do_login(request: HttpRequest, user_profile: UserProfile) -> None: """Creates a session, logging in the user, using the Django method, and also adds helpful data needed by our server logs. """ - django_login(request, user_profile) - RequestNotes.get_notes(request).requester_for_logs = user_profile.format_requester_for_logs() - process_client(request, user_profile, is_browser_view=True) + + # As a hardening measure, pass the user_profile through the dummy backend, + # which does the minimal validation that the user is allowed to log in. + # This, and stronger validation, should have already been done by the + # caller, so we raise an AssertionError if this doesn't work as expected. + # This is to prevent misuse of this function, as it would pose a major + # security issue. + realm = get_valid_realm_from_request(request) + validated_user_profile = authenticate( + request=request, username=user_profile.delivery_email, realm=realm, use_dummy_backend=True + ) + if validated_user_profile is None or validated_user_profile != user_profile: + raise AssertionError("do_login called for a user_profile that shouldn't be able to log in") + + assert isinstance(validated_user_profile, UserProfile) + + django_login(request, validated_user_profile) + RequestNotes.get_notes( + request + ).requester_for_logs = validated_user_profile.format_requester_for_logs() + process_client(request, validated_user_profile, is_browser_view=True) if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: # Log in with two factor authentication as well. - do_two_factor_login(request, user_profile) + do_two_factor_login(request, validated_user_profile) def log_view_func( diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index e82ff9305d..f69887dd78 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -5424,7 +5424,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): def test_login_success(self) -> None: user_profile = self.example_user("hamlet") email = user_profile.delivery_email - with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): + with self.settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ): result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) self.assertEqual(result.status_code, 302) self.assert_logged_in_user_id(user_profile.id) @@ -5433,7 +5438,10 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): username = "hamlet" user_profile = self.example_user("hamlet") with self.settings( - AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",), + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ), SSO_APPEND_DOMAIN="zulip.com", ): result = self.client_get("/accounts/login/sso/", REMOTE_USER=username) @@ -5443,7 +5451,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): def test_login_case_insensitive(self) -> None: user_profile = self.example_user("hamlet") email_upper = user_profile.delivery_email.upper() - with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): + with self.settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ): result = self.client_get("/accounts/login/sso/", REMOTE_USER=email_upper) self.assertEqual(result.status_code, 302) self.assert_logged_in_user_id(user_profile.id) @@ -5462,7 +5475,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): def test_login_failure_due_to_nonexisting_user(self) -> None: email = "nonexisting@zulip.com" - with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): + with self.settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ): result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) self.assertEqual(result.status_code, 200) self.assert_logged_in_user_id(None) @@ -5470,13 +5488,21 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): def test_login_failure_due_to_invalid_email(self) -> None: email = "hamlet" - with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): + with self.settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ): result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) self.assert_json_error_contains(result, "Enter a valid email address.", 400) def test_login_failure_due_to_missing_field(self) -> None: with self.settings( - AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",) + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) ), self.assertLogs("django.request", level="ERROR") as m: result = self.client_get("/accounts/login/sso/") self.assertEqual(result.status_code, 500) @@ -5488,7 +5514,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): def test_login_failure_due_to_wrong_subdomain(self) -> None: email = self.example_email("hamlet") - with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): + with self.settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ): with mock.patch("zerver.views.auth.get_subdomain", return_value="acme"): result = self.client_get( "http://testserver:9080/accounts/login/sso/", REMOTE_USER=email @@ -5499,7 +5530,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): def test_login_failure_due_to_empty_subdomain(self) -> None: email = self.example_email("hamlet") - with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): + with self.settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ): with mock.patch("zerver.views.auth.get_subdomain", return_value=""): result = self.client_get( "http://testserver:9080/accounts/login/sso/", REMOTE_USER=email @@ -5513,14 +5549,22 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): email = user_profile.delivery_email with mock.patch("zerver.views.auth.get_subdomain", return_value="zulip"): with self.settings( - AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",) + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) ): result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) self.assertEqual(result.status_code, 302) self.assert_logged_in_user_id(user_profile.id) @override_settings(SEND_LOGIN_EMAILS=True) - @override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)) + @override_settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ) def test_login_mobile_flow_otp_success_email(self) -> None: user_profile = self.example_user("hamlet") email = user_profile.delivery_email @@ -5568,7 +5612,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): @override_settings(SEND_LOGIN_EMAILS=True) @override_settings(SSO_APPEND_DOMAIN="zulip.com") - @override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)) + @override_settings( + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) + ) def test_login_mobile_flow_otp_success_username(self) -> None: user_profile = self.example_user("hamlet") email = user_profile.delivery_email @@ -5689,7 +5738,10 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase): user_profile = self.example_user("hamlet") email = user_profile.delivery_email with self.settings( - AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",) + AUTHENTICATION_BACKENDS=( + "zproject.backends.ZulipRemoteUserBackend", + "zproject.backends.ZulipDummyBackend", + ) ): result = self.client_get("/accounts/login/sso/", {"next": next}, REMOTE_USER=email) return result diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 891b3167a4..5a16be39fd 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -931,7 +931,7 @@ class LoginTest(ZulipTestCase): # seem to be any O(N) behavior. Some of the cache hits are related # to sending messages, such as getting the welcome bot, looking up # the alert words for a realm, etc. - with self.assert_database_query_count(104), self.assert_memcached_count(18): + with self.assert_database_query_count(105), self.assert_memcached_count(18): with self.captureOnCommitCallbacks(execute=True): self.register(self.nonreg_email("test"), "test") @@ -3762,6 +3762,7 @@ class UserSignUpTest(ZulipTestCase): AUTHENTICATION_BACKENDS=( "zproject.backends.ZulipLDAPAuthBackend", "zproject.backends.EmailAuthBackend", + "zproject.backends.ZulipDummyBackend", ) ) def test_ldap_invite_user_as_admin(self) -> None: @@ -3773,6 +3774,7 @@ class UserSignUpTest(ZulipTestCase): AUTHENTICATION_BACKENDS=( "zproject.backends.ZulipLDAPAuthBackend", "zproject.backends.EmailAuthBackend", + "zproject.backends.ZulipDummyBackend", ) ) def test_ldap_invite_user_as_guest(self) -> None: @@ -3784,6 +3786,7 @@ class UserSignUpTest(ZulipTestCase): AUTHENTICATION_BACKENDS=( "zproject.backends.ZulipLDAPAuthBackend", "zproject.backends.EmailAuthBackend", + "zproject.backends.ZulipDummyBackend", ) ) def test_ldap_invite_streams(self) -> None: