CVE-2019-18933: Fix insecure account creation via social authentication.

A bug in Zulip's new user signup process meant that users who
registered their account using social authentication (e.g. GitHub or
Google SSO) in an organization that also allows password
authentication could have their personal API key stolen by an
unprivileged attacker, allowing nearly full access to the user's
account.

Zulip versions between 1.7.0 and 2.0.6 were affected.

This commit fixes the original bug and also contains a database
migration to fix any users with corrupt `password` fields in the
database as a result of the bug.

Out of an abundance of caution (and to protect the users of any
installations that delay applying this commit), the migration also
resets the API keys of any users where Zulip's logs cannot prove the
user's API key was not previously stolen via this bug.  Resetting
those API keys will be inconvenient for users:

* Users of the Zulip mobile and terminal apps whose API keys are reset
  will be logged out and need to login again.
* Users using their personal API keys for any other reason will need
  to re-fetch their personal API key.

We discovered this bug internally and don't believe it was disclosed
prior to our publishing it through this commit.  Because the algorithm
for determining which users might have been affected is very
conservative, many users who were never at risk will have their API
keys reset by this migration.

To avoid this on self-hosted installations that have always used
e.g. LDAP authentication, we skip resetting API keys on installations
that don't have password authentication enabled.  System
administrators on installations that used to have email authentication
enabled, but no longer do, should temporarily enable EmailAuthBackend
before applying this migration.

The migration also records which users had their passwords or API keys
reset in the usual RealmAuditLog table.

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",

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),
+    ]

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]

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"

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:

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,

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: