Skip to content
Permalink
Browse files

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.
  • Loading branch information
mateuszmandera authored and timabbott committed Nov 18, 2019
1 parent 16ea89a commit 0c2cc41d2e40807baa5ee2c72987ebfb64ea2eb6
@@ -416,6 +416,7 @@
'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",
@@ -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),
]
@@ -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]
@@ -212,6 +212,26 @@ def test_email_auth_backend(self) -> None:
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"
@@ -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:
@@ -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,
@@ -212,6 +212,11 @@ def authenticate(self, *, username: str, password: str,
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:

0 comments on commit 0c2cc41

Please sign in to comment.
You can’t perform that action at this time.