diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index d0a23f6a24..0baadeff0a 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -416,6 +416,7 @@ python_rules = RuleList( 'zerver/migrations/0060_move_avatars_to_be_uid_based.py', 'zerver/migrations/0104_fix_unreads.py', 'zerver/migrations/0206_stream_rendered_description.py', + 'zerver/migrations/0209_user_profile_no_empty_password.py', 'pgroonga/migrations/0002_html_escape_subject.py', ]), 'description': "Don't import models or other code in migrations; see docs/subsystems/schema-migrations.md", diff --git a/zerver/migrations/0209_user_profile_no_empty_password.py b/zerver/migrations/0209_user_profile_no_empty_password.py new file mode 100644 index 0000000000..457014f501 --- /dev/null +++ b/zerver/migrations/0209_user_profile_no_empty_password.py @@ -0,0 +1,234 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.24 on 2019-10-16 22:48 +from __future__ import unicode_literals + +from django.conf import settings +from django.contrib.auth import get_backends +from django.db import migrations +from django.db.backends.postgresql_psycopg2.schema import DatabaseSchemaEditor +from django.db.migrations.state import StateApps +from django.contrib.auth.hashers import check_password, make_password +from django.utils.timezone import now as timezone_now + +from zerver.lib.cache import cache_delete, user_profile_by_api_key_cache_key +from zerver.lib.queue import queue_json_publish +from zerver.lib.utils import generate_api_key +from zproject.backends import EmailAuthBackend + +from typing import Any, Set, Union + +import ujson + +def ensure_no_empty_passwords(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: + """With CVE-2019-18933, it was possible for certain users created + using social login (e.g. Google/GitHub auth) to have the empty + string as their password in the Zulip database, rather than + Django's "unusable password" (i.e. no password at all). This was a + serious security issue for organizations with both password and + Google/GitHub authentication enabled. + + Combined with the code changes to prevent new users from entering + this buggy state, this migration sets the intended "no password" + state for any users who are in this buggy state, as had been + intended. + + While this bug was discovered by our own development team and we + believe it hasn't been exploited in the wild, out of an abundance + of caution, this migration also resets the personal API keys for + all users where Zulip's database-level logging cannot **prove** + that user's current personal API key was never accessed using this + bug. + + There are a few ways this can be proven: (1) the user's password + has never been changed and is not the empty string, + or (2) the user's personal API key has changed since that user last + changed their password (which is not ''). Both constitute proof + because this bug cannot be used to gain the access required to change + or reset a user's password. + + Resetting those API keys has the effect of logging many users out + of the Zulip mobile and terminal apps unnecessarily (e.g. because + the user changed their password at any point in the past, even + though the user never was affected by the bug), but we're + comfortable with that cost for ensuring that this bug is + completely fixed. + + To avoid this inconvenience for self-hosted servers which don't + even have EmailAuthBackend enabled, we skip resetting any API keys + if the server doesn't have EmailAuthBackend configured. + """ + + UserProfile = apps.get_model('zerver', 'UserProfile') + RealmAuditLog = apps.get_model('zerver', 'RealmAuditLog') + + # Because we're backporting this migration to the Zulip 2.0.x + # series, we've given it migration number 0209, which is a + # duplicate with an existing migration already merged into Zulip + # master. Migration 0247_realmauditlog_event_type_to_int.py + # changes the format of RealmAuditLog.event_type, so we need the + # following conditional block to determine what values to use when + # searching for the relevant events in that log. + event_type_class = RealmAuditLog._meta.get_field('event_type').get_internal_type() + if event_type_class == 'CharField': + USER_PASSWORD_CHANGED = 'user_password_changed' # type: Union[int, str] + USER_API_KEY_CHANGED = 'user_api_key_changed' # type: Union[int, str] + else: + USER_PASSWORD_CHANGED = 122 + USER_API_KEY_CHANGED = 127 + + # First, we do some bulk queries to collect data we'll find useful + # in the loop over all users below. + + # Users who changed their password at any time since account + # creation. These users could theoretically have started with an + # empty password, but set a password later via the password reset + # flow. If their API key has changed since they changed their + # password, we can prove their current API key cannot have been + # exposed; we store those users in + # password_change_user_ids_no_reset_needed. + password_change_user_ids = set(RealmAuditLog.objects.filter( + event_type=USER_PASSWORD_CHANGED).values_list("modified_user_id", flat=True)) + password_change_user_ids_api_key_reset_needed = set() # type: Set[int] + password_change_user_ids_no_reset_needed = set() # type: Set[int] + + for user_id in password_change_user_ids: + # Here, we check the timing for users who have changed + # their password. + + # We check if the user changed their API key since their first password change. + query = RealmAuditLog.objects.filter( + modified_user=user_id, event_type__in=[USER_PASSWORD_CHANGED, + USER_API_KEY_CHANGED] + ).order_by("event_time") + + earliest_password_change = query.filter(event_type=USER_PASSWORD_CHANGED).first() + # Since these users are in password_change_user_ids, this must not be None. + assert earliest_password_change is not None + + latest_api_key_change = query.filter(event_type=USER_API_KEY_CHANGED).last() + if latest_api_key_change is None: + # This user has never changed their API key. As a + # result, even though it's very likely this user never + # had an empty password, they have changed their + # password, and we have no record of the password's + # original hash, so we can't prove the user's API key + # was never affected. We schedule this user's API key + # to be reset. + password_change_user_ids_api_key_reset_needed.add(user_id) + elif earliest_password_change.event_time <= latest_api_key_change.event_time: + # This user has changed their password before + # generating their current personal API key, so we can + # prove their current personal API key could not have + # been exposed by this bug. + password_change_user_ids_no_reset_needed.add(user_id) + else: + password_change_user_ids_api_key_reset_needed.add(user_id) + + if password_change_user_ids_no_reset_needed and settings.PRODUCTION: + # We record in this log file users whose current API key was + # generated after a real password was set, so there's no need + # to reset their API key, but because they've changed their + # password, we don't know whether or not they originally had a + # buggy password. + # + # In theory, this list can be recalculated using the above + # algorithm modified to only look at events before the time + # this migration was installed, but it's helpful to log it as well. + with open("/var/log/zulip/0209_password_migration.log", "w") as log_file: + line = "No reset needed, but changed password: {}\n" + log_file.write(line.format(password_change_user_ids_no_reset_needed)) + + AFFECTED_USER_TYPE_EMPTY_PASSWORD = 'empty_password' + AFFECTED_USER_TYPE_CHANGED_PASSWORD = 'changed_password' + MIGRATION_ID = '0209_user_profile_no_empty_password' + + def write_realm_audit_log_entry(user_profile: Any, + event_time: Any, event_type: Any, + affected_user_type: str) -> None: + RealmAuditLog.objects.create( + realm=user_profile.realm, + modified_user=user_profile, + event_type=event_type, + event_time=event_time, + extra_data=ujson.dumps({ + 'migration_id': MIGRATION_ID, + 'affected_user_type': affected_user_type, + }) + ) + + # If Zulip's built-in password authentication is not enabled on + # the server level, then we plan to skip resetting any users' API + # keys, since the bug requires EmailAuthBackend. + email_auth_enabled = any(isinstance(backend, EmailAuthBackend) + for backend in get_backends()) + + # A quick note: This query could in theory exclude users with + # is_active=False, is_bot=True, or realm__deactivated=True here to + # accessing only active human users in non-deactivated realms. + # But it's better to just be thorough; users can be reactivated, + # and e.g. a server admin could manually edit the database to + # change a bot into a human user if they really wanted to. And + # there's essentially no harm in rewriting state for a deactivated + # account. + for user_profile in UserProfile.objects.all(): + event_time = timezone_now() + if check_password('', user_profile.password): + # This user currently has the empty string as their password. + + # Change their password and record that we did so. + user_profile.password = make_password(None) + update_fields = ["password"] + write_realm_audit_log_entry(user_profile, event_time, + USER_PASSWORD_CHANGED, + AFFECTED_USER_TYPE_EMPTY_PASSWORD) + + if email_auth_enabled and not user_profile.is_bot: + # As explained above, if the built-in password authentication + # is enabled, reset the API keys. We can skip bot accounts here, + # because the `password` attribute on a bot user is useless. + reset_user_api_key(user_profile) + update_fields.append("api_key") + + event_time = timezone_now() + write_realm_audit_log_entry(user_profile, event_time, + USER_API_KEY_CHANGED, + AFFECTED_USER_TYPE_EMPTY_PASSWORD) + + user_profile.save(update_fields=update_fields) + continue + + elif email_auth_enabled and \ + user_profile.id in password_change_user_ids_api_key_reset_needed: + # For these users, we just need to reset the API key. + reset_user_api_key(user_profile) + user_profile.save(update_fields=["api_key"]) + + write_realm_audit_log_entry(user_profile, event_time, + USER_API_KEY_CHANGED, + AFFECTED_USER_TYPE_CHANGED_PASSWORD) + +def reset_user_api_key(user_profile: Any) -> None: + old_api_key = user_profile.api_key + user_profile.api_key = generate_api_key() + cache_delete(user_profile_by_api_key_cache_key(old_api_key)) + + # Like with any API key change, we need to clear any server-side + # state for sending push notifications to mobile app clients that + # could have been registered with the old API key. Fortunately, + # we can just write to the queue processor that handles sending + # those notices to the push notifications bouncer service. + event = {'type': 'clear_push_device_tokens', + 'user_profile_id': user_profile.id} + queue_json_publish("deferred_work", event) + +class Migration(migrations.Migration): + atomic = False + + dependencies = [ + ('zerver', '0208_add_realm_night_logo_fields'), + ] + + operations = [ + migrations.RunPython(ensure_no_empty_passwords, + reverse_code=migrations.RunPython.noop), + ] diff --git a/zerver/migrations/0254_merge_0209_0253.py b/zerver/migrations/0254_merge_0209_0253.py new file mode 100644 index 0000000000..915aadd95f --- /dev/null +++ b/zerver/migrations/0254_merge_0209_0253.py @@ -0,0 +1,16 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.26 on 2019-11-21 01:47 +from __future__ import unicode_literals + +from django.db import migrations +from typing import Any, List + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0253_userprofile_wildcard_mentions_notify'), + ('zerver', '0209_user_profile_no_empty_password'), + ] + + operations = [ + ] # type: List[Any] diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 032ef8b015..a4d970dede 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -212,6 +212,26 @@ class AuthBackendTest(ZulipTestCase): realm=get_realm('zephyr'), return_data=dict())) + def test_email_auth_backend_empty_password(self) -> None: + user_profile = self.example_user('hamlet') + password = "testpassword" + user_profile.set_password(password) + user_profile.save() + + # First, verify authentication works with the a nonempty + # password so we know we've set up the test correctly. + self.assertIsNotNone(EmailAuthBackend().authenticate(username=self.example_email('hamlet'), + password=password, + realm=get_realm("zulip"))) + + # Now do the same test with the empty string as the password. + password = "" + user_profile.set_password(password) + user_profile.save() + self.assertIsNone(EmailAuthBackend().authenticate(username=self.example_email('hamlet'), + password=password, + realm=get_realm("zulip"))) + def test_email_auth_backend_disabled_password_auth(self) -> None: user_profile = self.example_user('hamlet') password = "testpassword" diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 71eee0dac3..543ffe9231 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -199,10 +199,14 @@ def accounts_register(request: HttpRequest) -> HttpResponse: form['password'].field.required = False if form.is_valid(): - if password_auth_enabled(realm): + if password_auth_enabled(realm) and form['password'].field.required: password = form.cleaned_data['password'] else: - # SSO users don't need no passwords + # If the user wasn't prompted for a password when + # completing the authentication form (because they're + # signing up with SSO and no password is required), set + # the password field to `None` (Which causes Django to + # create an unusable password). password = None if realm_creation: diff --git a/zerver/views/users.py b/zerver/views/users.py index 4fd63d701c..fd0c5663de 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -328,7 +328,7 @@ def add_bot_backend( if bot_type in (UserProfile.INCOMING_WEBHOOK_BOT, UserProfile.EMBEDDED_BOT) and service_name: check_valid_bot_config(bot_type, service_name, config_data) - bot_profile = do_create_user(email=email, password='', + bot_profile = do_create_user(email=email, password=None, realm=user_profile.realm, full_name=full_name, short_name=short_name, bot_type=bot_type, diff --git a/zproject/backends.py b/zproject/backends.py index 7448431a4a..7ad56f2abc 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -212,6 +212,11 @@ class EmailAuthBackend(ZulipAuthMixin): if return_data is not None: return_data['email_auth_disabled'] = True return None + if password == "": + # Never allow an empty password. This is defensive code; + # a user having password "" should only be possible + # through a bug somewhere else. + return None user_profile = common_get_active_user(username, realm, return_data=return_data) if user_profile is None: