From 06c2161f7eb4c2550a3b69ec33d27d42b58a5c9c Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Mon, 18 Nov 2019 08:11:03 +0100 Subject: [PATCH] auth: Use zxcvbn to ensure password strength on server side. For a long time, we've been only doing the zxcvbn password strength checks on the browser, which is helpful, but means users could through hackery (or a bug in the frontend validation code) manage to set a too-weak password. We fix this by running our password strength validation on the backend as well, using python-zxcvbn. In theory, a bug in python-zxcvbn could result in it producing a different opinion than the frontend version; if so, it'd be a pretty bad bug in the library, and hopefully we'd hear about it from users, report upstream, and get it fixed that way. Alternatively, we can switch to shelling out to node like we do for KaTeX. Fixes #6880. --- requirements/common.in | 3 ++ requirements/dev.txt | 2 + requirements/prod.txt | 2 + version.py | 2 +- zerver/forms.py | 21 ++++++++- zerver/models.py | 14 ++++++ zerver/tests/test_auth_backends.py | 27 ++++++++++-- zerver/tests/test_settings.py | 21 +++++++++ zerver/tests/test_signup.py | 68 ++++++++++++++++++++++++------ zerver/tests/test_users.py | 5 +++ zerver/views/user_settings.py | 4 +- zerver/views/users.py | 6 ++- zproject/backends.py | 19 +++++++++ 13 files changed, 175 insertions(+), 19 deletions(-) diff --git a/requirements/common.in b/requirements/common.in index 737840cf5a..a7a8765f0e 100644 --- a/requirements/common.in +++ b/requirements/common.in @@ -186,3 +186,6 @@ python3-saml # Use SameSite cookies in legacy Django (remove with Django 2.1) django-cookies-samesite + +# For server-side enforcement of password strength +zxcvbn diff --git a/requirements/dev.txt b/requirements/dev.txt index a29e69aca9..650a8deed6 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -980,6 +980,8 @@ https://github.com/zulip/python-zulip-api/archive/804501610b6a205334e71b4e441fca --hash=sha256:270cebf4554a3a2fb1c95190168c952ef97081541ed2be0a83f00fb48e29928a https://github.com/zulip/python-zulip-api/archive/804501610b6a205334e71b4e441fca60acf650da.zip#egg=zulip_bots==0.6.1+git&subdirectory=zulip_bots \ --hash=sha256:270cebf4554a3a2fb1c95190168c952ef97081541ed2be0a83f00fb48e29928a +zxcvbn==4.4.28 \ + --hash=sha256:151bd816817e645e9064c354b13544f85137ea3320ca3be1fb6873ea75ef7dc1 # The following packages are considered to be unsafe in a requirements file: pip==19.3.1 \ diff --git a/requirements/prod.txt b/requirements/prod.txt index 42b22e3715..d24251ca80 100644 --- a/requirements/prod.txt +++ b/requirements/prod.txt @@ -575,6 +575,8 @@ https://github.com/zulip/python-zulip-api/archive/804501610b6a205334e71b4e441fca --hash=sha256:270cebf4554a3a2fb1c95190168c952ef97081541ed2be0a83f00fb48e29928a https://github.com/zulip/python-zulip-api/archive/804501610b6a205334e71b4e441fca60acf650da.zip#egg=zulip_bots==0.6.1+git&subdirectory=zulip_bots \ --hash=sha256:270cebf4554a3a2fb1c95190168c952ef97081541ed2be0a83f00fb48e29928a +zxcvbn==4.4.28 \ + --hash=sha256:151bd816817e645e9064c354b13544f85137ea3320ca3be1fb6873ea75ef7dc1 # The following packages are considered to be unsafe in a requirements file: pip==19.3.1 \ diff --git a/version.py b/version.py index 8524f6d815..6fab645c7c 100644 --- a/version.py +++ b/version.py @@ -26,4 +26,4 @@ LATEST_RELEASE_ANNOUNCEMENT = "https://blog.zulip.org/2019/03/01/zulip-2-0-relea # historical commits sharing the same major version, in which case a # minor version bump suffices. -PROVISION_VERSION = '66.0' +PROVISION_VERSION = '66.1' diff --git a/zerver/forms.py b/zerver/forms.py index 70cf8a956c..2dc30ff601 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -25,7 +25,7 @@ from zerver.models import Realm, get_user_by_delivery_email, UserProfile, get_re email_to_domain, \ email_allowed_for_realm, DisposableEmailError, DomainNotAllowedForRealmError, \ EmailContainsPlusError -from zproject.backends import email_auth_enabled, email_belongs_to_ldap +from zproject.backends import email_auth_enabled, email_belongs_to_ldap, check_password_strength import logging import re @@ -44,6 +44,7 @@ WRONG_SUBDOMAIN_ERROR = "Your Zulip account is not a member of the " + \ "Please contact your organization administrator with any questions." DEACTIVATED_ACCOUNT_ERROR = u"Your account is no longer active. " + \ u"Please contact your organization administrator to reactivate it." +PASSWORD_TOO_WEAK_ERROR = u"The password is too weak." def email_is_not_mit_mailing_list(email: str) -> None: """Prevent MIT mailing lists from signing up for Zulip""" @@ -108,6 +109,15 @@ class RegistrationForm(forms.Form): except JsonableError as e: raise ValidationError(e.msg) + def clean_password(self) -> str: + password = self.cleaned_data['password'] + if self.fields['password'].required and not check_password_strength(password): + # The frontend code tries to stop the user from submitting the form with a weak password, + # but if the user bypasses that protection, this error code path will run. + raise ValidationError(mark_safe(PASSWORD_TOO_WEAK_ERROR)) + + return password + def clean_realm_subdomain(self) -> str: if not self.realm_creation: # This field is only used if realm_creation @@ -179,6 +189,15 @@ class RealmCreationForm(forms.Form): email_is_not_disposable]) class LoggingSetPasswordForm(SetPasswordForm): + def clean_new_password1(self) -> str: + new_password = self.cleaned_data['new_password1'] + if not check_password_strength(new_password): + # The frontend code tries to stop the user from submitting the form with a weak password, + # but if the user bypasses that protection, this error code path will run. + raise ValidationError(PASSWORD_TOO_WEAK_ERROR) + + return new_password + def save(self, commit: bool=True) -> UserProfile: do_change_password(self.user, self.cleaned_data['new_password1'], commit=commit) diff --git a/zerver/models.py b/zerver/models.py index 38f189015b..3bf328a25c 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1138,6 +1138,20 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): else: return -1 + def set_password(self, password: Optional[str]) -> None: + if password is None: + self.set_unusable_password() + return + + from zproject.backends import check_password_strength + if not check_password_strength(password): + raise PasswordTooWeakError + + super().set_password(password) + +class PasswordTooWeakError(Exception): + pass + class UserGroup(models.Model): name = models.CharField(max_length=100) members = models.ManyToManyField(UserProfile, through='UserGroupMembership') diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index a4d970dede..c02c3ed69a 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -45,7 +45,7 @@ from zerver.lib.test_classes import ( from zerver.models import \ get_realm, email_to_username, CustomProfileField, CustomProfileFieldValue, \ UserProfile, PreregistrationUser, Realm, RealmDomain, get_user, MultiuseInvite, \ - clear_supported_auth_backends_cache + clear_supported_auth_backends_cache, PasswordTooWeakError from zerver.signals import JUST_CREATED_THRESHOLD from confirmation.models import Confirmation, create_confirmation_link @@ -58,7 +58,7 @@ from zproject.backends import ZulipDummyBackend, EmailAuthBackend, \ ZulipLDAPConfigurationError, ZulipLDAPExceptionNoMatchingLDAPUser, ZulipLDAPExceptionOutsideDomain, \ ZulipLDAPException, query_ldap, sync_user_from_ldap, SocialAuthMixin, \ PopulateUserLDAPError, SAMLAuthBackend, saml_auth_enabled, email_belongs_to_ldap, \ - get_social_backend_dicts, AzureADAuthBackend + get_social_backend_dicts, AzureADAuthBackend, check_password_strength from zerver.views.auth import (maybe_send_to_registration, _subdomain_token_salt) @@ -226,7 +226,12 @@ class AuthBackendTest(ZulipTestCase): # Now do the same test with the empty string as the password. password = "" - user_profile.set_password(password) + with self.assertRaises(PasswordTooWeakError): + # UserProfile.set_password protects against setting an empty password. + user_profile.set_password(password) + # We do want to force an empty password for this test, so we bypass the protection + # by using Django's version of this method. + super(UserProfile, user_profile).set_password(password) user_profile.save() self.assertIsNone(EmailAuthBackend().authenticate(username=self.example_email('hamlet'), password=password, @@ -475,6 +480,20 @@ class AuthBackendTest(ZulipTestCase): httpretty.disable() httpretty.reset() +class CheckPasswordStrengthTest(ZulipTestCase): + def test_check_password_strength(self) -> None: + with self.settings(PASSWORD_MIN_LENGTH=0, PASSWORD_MIN_GUESSES=0): + # Never allow empty password. + self.assertFalse(check_password_strength('')) + + with self.settings(PASSWORD_MIN_LENGTH=6, PASSWORD_MIN_GUESSES=1000): + self.assertFalse(check_password_strength('')) + self.assertFalse(check_password_strength('short')) + # Long enough, but too easy: + self.assertFalse(check_password_strength('longer')) + # Good password: + self.assertTrue(check_password_strength('f657gdGGk9')) + class SocialAuthBase(ZulipTestCase): """This is a base class for testing social-auth backends. These methods are often overriden by subclasses: @@ -832,6 +851,8 @@ class SocialAuthBase(ZulipTestCase): self.assert_logged_in_user_id(user_profile.id) self.assertEqual(user_profile.full_name, expected_final_name) + self.assertFalse(user_profile.has_usable_password()) + @override_settings(TERMS_OF_SERVICE=None) def test_social_auth_registration(self) -> None: """If the user doesn't exist yet, social auth can be used to register an account""" diff --git a/zerver/tests/test_settings.py b/zerver/tests/test_settings.py index 4178b44ffc..af789828cd 100644 --- a/zerver/tests/test_settings.py +++ b/zerver/tests/test_settings.py @@ -77,6 +77,27 @@ class ChangeSettingsTest(ZulipTestCase): user_profile = self.example_user('hamlet') self.assert_logged_in_user_id(user_profile.id) + def test_password_change_check_strength(self) -> None: + self.login(self.example_email("hamlet")) + with self.settings(PASSWORD_MIN_LENGTH=3, PASSWORD_MIN_GUESSES=1000): + json_result = self.client_patch( + "/json/settings", + dict( + full_name='Foo Bar', + old_password=initial_password(self.example_email("hamlet")), + new_password='easy', + )) + self.assert_json_error(json_result, "New password is too weak!") + + json_result = self.client_patch( + "/json/settings", + dict( + full_name='Foo Bar', + old_password=initial_password(self.example_email("hamlet")), + new_password='f657gdGGk9', + )) + self.assert_json_success(json_result) + def test_illegal_name_changes(self) -> None: user = self.example_user('hamlet') email = user.email diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 647fc0882c..19bfd06cb1 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -243,21 +243,28 @@ class PasswordResetTest(ZulipTestCase): self.assertEqual(result.status_code, 200) # Reset your password - result = self.client_post(password_reset_url, - {'new_password1': 'new_password', - 'new_password2': 'new_password'}) + with self.settings(PASSWORD_MIN_LENGTH=3, PASSWORD_MIN_GUESSES=1000): + # Verify weak passwords don't work. + result = self.client_post(password_reset_url, + {'new_password1': 'easy', + 'new_password2': 'easy'}) + self.assert_in_response("The password is too weak.", + result) - # password reset succeeded - self.assertEqual(result.status_code, 302) - self.assertTrue(result["Location"].endswith("/password/done/")) + result = self.client_post(password_reset_url, + {'new_password1': 'f657gdGGk9', + 'new_password2': 'f657gdGGk9'}) + # password reset succeeded + self.assertEqual(result.status_code, 302) + self.assertTrue(result["Location"].endswith("/password/done/")) - # log back in with new password - self.login(email, password='new_password') - user_profile = self.example_user('hamlet') - self.assert_logged_in_user_id(user_profile.id) + # log back in with new password + self.login(email, password='f657gdGGk9') + user_profile = self.example_user('hamlet') + self.assert_logged_in_user_id(user_profile.id) - # make sure old password no longer works - self.login(email, password=old_password, fails=True) + # make sure old password no longer works + self.login(email, password=old_password, fails=True) def test_password_reset_for_non_existent_user(self) -> None: email = 'nonexisting@mars.com' @@ -2238,6 +2245,43 @@ class UserSignUpTest(InviteUserBase): 'from_confirmation': '1'}) self.assert_in_success_response(["We just need you to do one last thing."], result) + def test_signup_with_weak_password(self) -> None: + """ + Check if signing up without a full name redirects to a registration + form. + """ + email = "newguy@zulip.com" + + result = self.client_post('/accounts/home/', {'email': email}) + self.assertEqual(result.status_code, 302) + self.assertTrue(result["Location"].endswith( + "/accounts/send_confirm/%s" % (email,))) + result = self.client_get(result["Location"]) + self.assert_in_response("Check your email so we can get started.", result) + + # Visit the confirmation link. + confirmation_url = self.get_confirmation_url_from_outbox(email) + result = self.client_get(confirmation_url) + self.assertEqual(result.status_code, 200) + + with self.settings(PASSWORD_MIN_LENGTH=6, PASSWORD_MIN_GUESSES=1000): + result = self.client_post( + '/accounts/register/', + {'password': 'easy', + 'key': find_key_by_email(email), + 'terms': True, + 'full_name': "New Guy", + 'from_confirmation': '1'}) + self.assert_in_success_response(["We just need you to do one last thing."], result) + + result = self.submit_reg_form_for_user(email, + 'easy', + full_name="New Guy") + self.assert_in_success_response(["The password is too weak."], result) + with self.assertRaises(UserProfile.DoesNotExist): + # Account wasn't created. + get_user(email, get_realm("zulip")) + def test_signup_with_default_stream_group(self) -> None: # Check if user is subscribed to the streams of default # stream group as well as default streams. diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 26a9a55150..2288b12fd9 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -603,6 +603,11 @@ class AdminCreateUserTest(ZulipTestCase): full_name='Romeo Montague', short_name='Romeo', ) + # Check can't use a bad password with zxcvbn enabled + with self.settings(PASSWORD_MIN_LENGTH=6, PASSWORD_MIN_GUESSES=1000): + result = self.client_post("/json/users", valid_params) + self.assert_json_error(result, "The password is too weak.") + result = self.client_post("/json/users", valid_params) self.assert_json_success(result) diff --git a/zerver/views/user_settings.py b/zerver/views/user_settings.py index ac836332f1..5899cf0f31 100644 --- a/zerver/views/user_settings.py +++ b/zerver/views/user_settings.py @@ -24,7 +24,7 @@ from zerver.lib.timezone import get_all_timezones from zerver.models import UserProfile, name_changes_disabled, avatar_changes_disabled from confirmation.models import get_object_from_key, render_confirmation_key_error, \ ConfirmationKeyException, Confirmation -from zproject.backends import email_belongs_to_ldap +from zproject.backends import email_belongs_to_ldap, check_password_strength AVATAR_CHANGES_DISABLED_ERROR = _("Avatar changes are disabled in this organization.") @@ -71,6 +71,8 @@ def json_change_settings(request: HttpRequest, user_profile: UserProfile, if not authenticate(username=user_profile.delivery_email, password=old_password, realm=user_profile.realm, return_data=return_data): return json_error(_("Wrong password!")) + if not check_password_strength(new_password): + return json_error(_("New password is too weak!")) do_change_password(user_profile, new_password) # In Django 1.10, password changes invalidates sessions, see # https://docs.djangoproject.com/en/1.10/topics/auth/default/#session-invalidation-on-password-change diff --git a/zerver/views/users.py b/zerver/views/users.py index fd0c5663de..9af3f4bf35 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -8,7 +8,7 @@ from django.shortcuts import redirect, render from django.conf import settings from zerver.decorator import require_realm_admin, require_member_or_admin -from zerver.forms import CreateUserForm +from zerver.forms import CreateUserForm, PASSWORD_TOO_WEAK_ERROR from zerver.lib.events import get_raw_user_data from zerver.lib.actions import do_change_avatar_fields, do_change_bot_owner, \ do_change_is_admin, do_change_default_all_public_streams, \ @@ -38,6 +38,7 @@ from zerver.models import UserProfile, Stream, Message, email_allowed_for_realm, DomainNotAllowedForRealmError, DisposableEmailError, get_user_profile_by_id_in_realm, \ EmailContainsPlusError, get_user_by_id_in_realm_including_cross_realm, Realm, \ InvalidFakeEmailDomain +from zproject.backends import check_password_strength def deactivate_user_backend(request: HttpRequest, user_profile: UserProfile, user_id: int) -> HttpResponse: @@ -450,6 +451,9 @@ def create_user_backend(request: HttpRequest, user_profile: UserProfile, except UserProfile.DoesNotExist: pass + if not check_password_strength(password): + return json_error(PASSWORD_TOO_WEAK_ERROR) + do_create_user(email, password, realm, full_name, short_name) return json_success() diff --git a/zproject/backends.py b/zproject/backends.py index 7ad56f2abc..e83ae19d39 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -18,6 +18,7 @@ import magic import ujson from typing import Any, Dict, List, Optional, Set, Tuple, Union from typing_extensions import TypedDict +from zxcvbn import zxcvbn from django_auth_ldap.backend import LDAPBackend, LDAPReverseEmailSearch, \ _LDAPUser, ldap_error @@ -193,6 +194,24 @@ class ZulipDummyBackend(ZulipAuthMixin): return common_get_active_user(username, realm, return_data) return None +def check_password_strength(password: str) -> bool: + """ + Returns True if the password is strong enough, + False otherwise. + """ + if len(password) < settings.PASSWORD_MIN_LENGTH: + return False + + if password == '': + # zxcvbn throws an exception when passed the empty string, so + # we need a special case for the empty string password here. + return False + + if int(zxcvbn(password)['guesses']) < settings.PASSWORD_MIN_GUESSES: + return False + + return True + class EmailAuthBackend(ZulipAuthMixin): """ Email+Password Authentication Backend (the default).