From b7b747567225ee584ebca7b00aa2166809181319 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Fri, 4 Sep 2020 19:02:13 -0700 Subject: [PATCH] python: Use standard secrets module to generate random tokens. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are three functional side effects: • Correct an insignificant but mathematically offensive bias toward repeated characters in generate_api_key introduced in commit 47b4283c4b4c70ecde4d3c8de871c90ee2506d87; its entropy is increased from 190.52864 bits to 190.53428 bits. • Use the base32 alphabet in confirmation.models.generate_key; its entropy is reduced from 124.07820 bits to the documented 120 bits, but now it uses 1 syscall instead of 24. • Use the base32 alphabet in get_bigbluebutton_url; its entropy is reduced from 51.69925 bits to 50 bits, but now it uses 1 syscall instead of 10. (The base32 alphabet is A-Z 2-7. We could probably replace all of these with plain secrets.token_urlsafe, since I expect most callers can handle the full urlsafe_b64 alphabet A-Z a-z 0-9 - _ without problems.) Signed-off-by: Anders Kaseorg --- confirmation/models.py | 7 +++---- corporate/lib/stripe.py | 4 ++-- scripts/setup/generate_secrets.py | 4 ++-- tools/semgrep.yml | 1 - zerver/data_import/slack.py | 5 +++-- zerver/lib/cache.py | 6 ++---- zerver/lib/email_mirror.py | 4 ++-- zerver/lib/import_realm.py | 5 +++-- zerver/lib/redis_utils.py | 5 ++--- zerver/lib/upload.py | 13 +++++-------- zerver/lib/utils.py | 16 ++++++---------- ..._missed_message_addresses_from_redis_to_db.py | 5 +++-- zerver/models.py | 5 +++-- zerver/tests/test_auth_backends.py | 6 +++--- zerver/tests/test_create_video_call.py | 2 +- zerver/tests/test_rate_limiter.py | 4 ++-- zerver/tests/test_redis_utils.py | 2 +- zerver/views/auth.py | 4 ++-- zerver/views/home.py | 5 +++-- zerver/views/video_calls.py | 5 +++-- 20 files changed, 51 insertions(+), 57 deletions(-) diff --git a/confirmation/models.py b/confirmation/models.py index 072c60932b..55faff3471 100644 --- a/confirmation/models.py +++ b/confirmation/models.py @@ -2,8 +2,8 @@ __revision__ = '$Id: models.py 28 2009-10-22 15:03:02Z jarek.zgoda $' import datetime -import string -from random import SystemRandom +import secrets +from base64 import b32encode from typing import Mapping, Optional, Union from urllib.parse import urljoin @@ -37,9 +37,8 @@ def render_confirmation_key_error(request: HttpRequest, exception: ConfirmationK return render(request, 'confirmation/link_does_not_exist.html') def generate_key() -> str: - generator = SystemRandom() # 24 characters * 5 bits of entropy/character = 120 bits of entropy - return ''.join(generator.choice(string.ascii_lowercase + string.digits) for _ in range(24)) + return b32encode(secrets.token_bytes(15)).decode().lower() ConfirmationObjT = Union[MultiuseInvite, PreregistrationUser, EmailChangeStatus] def get_object_from_key(confirmation_key: str, diff --git a/corporate/lib/stripe.py b/corporate/lib/stripe.py index db23a57436..b9dd9d104a 100644 --- a/corporate/lib/stripe.py +++ b/corporate/lib/stripe.py @@ -1,6 +1,7 @@ import logging import math import os +import secrets from datetime import datetime, timedelta from decimal import Decimal from functools import wraps @@ -25,7 +26,6 @@ from corporate.models import ( ) from zerver.lib.logging_util import log_to_file from zerver.lib.timestamp import datetime_to_timestamp, timestamp_to_datetime -from zerver.lib.utils import generate_random_token from zerver.models import Realm, RealmAuditLog, UserProfile, get_system_bot from zproject.config import get_secret @@ -54,7 +54,7 @@ def get_latest_seat_count(realm: Realm) -> int: return max(non_guests, math.ceil(guests / 5)) def sign_string(string: str) -> Tuple[str, str]: - salt = generate_random_token(64) + salt = secrets.token_hex(32) signer = Signer(salt=salt) return signer.sign(string), salt diff --git a/scripts/setup/generate_secrets.py b/scripts/setup/generate_secrets.py index e9e7204f8f..bd6a4179c1 100755 --- a/scripts/setup/generate_secrets.py +++ b/scripts/setup/generate_secrets.py @@ -40,9 +40,9 @@ def random_token() -> str: # of importing cryptography modules when necessary. # # This helps optimize noop provision performance. - from zerver.lib.utils import generate_random_token + import secrets - return generate_random_token(64) + return secrets.token_hex(32) def generate_django_secretkey() -> str: """Secret key generation taken from Django's startproject.py""" diff --git a/tools/semgrep.yml b/tools/semgrep.yml index 5084aa3959..8acd6b89fd 100644 --- a/tools/semgrep.yml +++ b/tools/semgrep.yml @@ -20,7 +20,6 @@ rules: - id: dont-import-models-in-migrations patterns: - pattern-not: from zerver.lib.redis_utils import get_redis_client - - pattern-not: from zerver.lib.utils import generate_random_token - pattern-not: from zerver.models import filter_pattern_validator - pattern-not: from zerver.models import filter_format_validator - pattern-not: from zerver.models import generate_email_token_for_stream diff --git a/zerver/data_import/slack.py b/zerver/data_import/slack.py index d1dc63641d..7780dbe244 100755 --- a/zerver/data_import/slack.py +++ b/zerver/data_import/slack.py @@ -1,6 +1,7 @@ import logging import os import random +import secrets import shutil import subprocess from collections import defaultdict @@ -39,7 +40,7 @@ from zerver.data_import.slack_message_conversion import ( ) from zerver.lib.emoji import name_to_codepoint from zerver.lib.export import MESSAGE_BATCH_CHUNK_SIZE -from zerver.lib.upload import random_name, resize_logo, sanitize_name +from zerver.lib.upload import resize_logo, sanitize_name from zerver.models import ( CustomProfileField, CustomProfileFieldValue, @@ -904,7 +905,7 @@ def get_attachment_path_and_content(fileinfo: ZerverFieldsT, realm_id: int) -> T 'SlackImportAttachment', # This is a special placeholder which should be kept # in sync with 'exports.py' function 'import_message_data' format(random.randint(0, 255), 'x'), - random_name(18), + secrets.token_urlsafe(18), sanitize_name(fileinfo['name']), ]) attachment_path = f'/user_uploads/{s3_path}' diff --git a/zerver/lib/cache.py b/zerver/lib/cache.py index 63029f46d5..56bfac40f5 100644 --- a/zerver/lib/cache.py +++ b/zerver/lib/cache.py @@ -1,10 +1,9 @@ # See https://zulip.readthedocs.io/en/latest/subsystems/caching.html for docs -import base64 import hashlib import logging import os -import random import re +import secrets import sys import time import traceback @@ -87,8 +86,7 @@ def get_or_create_key_prefix() -> str: filename = os.path.join(settings.DEPLOY_ROOT, "var", "remote_cache_prefix") try: fd = os.open(filename, os.O_CREAT | os.O_EXCL | os.O_RDWR, 0o444) - random_hash = hashlib.sha256(str(random.getrandbits(256)).encode('utf-8')).digest() - prefix = base64.b16encode(random_hash)[:32].decode('utf-8').lower() + ':' + prefix = secrets.token_hex(16) + ':' # This does close the underlying file with os.fdopen(fd, 'w') as f: f.write(prefix + "\n") diff --git a/zerver/lib/email_mirror.py b/zerver/lib/email_mirror.py index eb4b8dd926..c99c280923 100644 --- a/zerver/lib/email_mirror.py +++ b/zerver/lib/email_mirror.py @@ -1,5 +1,6 @@ import logging import re +import secrets from email.headerregistry import AddressHeader from email.message import EmailMessage from typing import Dict, List, Optional, Tuple @@ -25,7 +26,6 @@ from zerver.lib.queue import queue_json_publish from zerver.lib.rate_limiter import RateLimitedObject from zerver.lib.send_email import FromAddress from zerver.lib.upload import upload_message_file -from zerver.lib.utils import generate_random_token from zerver.models import ( Message, MissedMessageEmailAddress, @@ -96,7 +96,7 @@ def log_and_report(email_message: EmailMessage, error_message: str, to: Optional # Temporary missed message addresses def generate_missed_message_token() -> str: - return 'mm' + generate_random_token(32) + return 'mm' + secrets.token_hex(16) def is_missed_message_address(address: str) -> bool: try: diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 6bb3d9a74a..39f4132bef 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -1,6 +1,7 @@ import datetime import logging import os +import secrets import shutil from typing import Any, Dict, Iterable, List, Optional, Tuple @@ -30,7 +31,7 @@ from zerver.lib.parallel import run_parallel from zerver.lib.server_initialization import create_internal_realm, server_initialized from zerver.lib.streams import render_stream_description from zerver.lib.timestamp import datetime_to_timestamp -from zerver.lib.upload import BadImageError, guess_type, random_name, sanitize_name +from zerver.lib.upload import BadImageError, guess_type, sanitize_name from zerver.lib.utils import generate_api_key, process_list_in_batches from zerver.models import ( AlertWord, @@ -668,7 +669,7 @@ def import_uploads(realm: Realm, import_dir: Path, processes: int, processing_av # function 'upload_message_file' relative_path = "/".join([ str(record['realm_id']), - random_name(18), + secrets.token_urlsafe(18), sanitize_name(os.path.basename(record['path'])), ]) path_maps['attachment_path'][record['s3_path']] = relative_path diff --git a/zerver/lib/redis_utils.py b/zerver/lib/redis_utils.py index 0c2825385a..ab0233a01e 100644 --- a/zerver/lib/redis_utils.py +++ b/zerver/lib/redis_utils.py @@ -1,12 +1,11 @@ import re +import secrets from typing import Any, Dict, Mapping, Optional import orjson import redis from django.conf import settings -from zerver.lib.utils import generate_random_token - # Redis accepts keys up to 512MB in size, but there's no reason for us to use such size, # so we want to stay limited to 1024 characters. MAX_KEY_LENGTH = 1024 @@ -34,7 +33,7 @@ def put_dict_in_redis(redis_client: redis.StrictRedis, key_format: str, error_msg = "Requested key too long in put_dict_in_redis. Key format: %s, token length: %s" raise ZulipRedisKeyTooLongError(error_msg % (key_format, token_length)) if token is None: - token = generate_random_token(token_length) + token = secrets.token_hex(token_length // 2) key = key_format.format(token=token) with redis_client.pipeline() as pipeline: pipeline.set(key, orjson.dumps(data_to_store)) diff --git a/zerver/lib/upload.py b/zerver/lib/upload.py index 9a1eee1cae..f1a36425b9 100644 --- a/zerver/lib/upload.py +++ b/zerver/lib/upload.py @@ -5,6 +5,7 @@ import logging import os import random import re +import secrets import shutil import unicodedata import urllib @@ -30,7 +31,6 @@ from PIL.Image import DecompressionBombError from zerver.lib.avatar_hash import user_avatar_path from zerver.lib.exceptions import ErrorCode, JsonableError -from zerver.lib.utils import generate_random_token from zerver.models import Attachment, Message, Realm, RealmEmoji, UserProfile DEFAULT_AVATAR_SIZE = 100 @@ -94,9 +94,6 @@ def sanitize_name(value: str) -> str: assert value not in {'', '.', '..'} return mark_safe(value) -def random_name(bytes: int=60) -> str: - return base64.urlsafe_b64encode(os.urandom(bytes)).decode('utf-8') - class BadImageError(JsonableError): code = ErrorCode.BAD_IMAGE @@ -362,7 +359,7 @@ class S3UploadBackend(ZulipUploadBackend): target_realm = user_profile.realm s3_file_name = "/".join([ str(target_realm.id), - random_name(18), + secrets.token_urlsafe(18), sanitize_name(uploaded_file_name), ]) url = f"/user_uploads/{s3_file_name}" @@ -591,7 +588,7 @@ class S3UploadBackend(ZulipUploadBackend): def upload_export_tarball(self, realm: Optional[Realm], tarball_path: str, percent_callback: Optional[Callable[[Any], None]]=None) -> str: # We use the avatar bucket, because it's world-readable. - key = self.avatar_bucket.Object(os.path.join("exports", generate_random_token(32), + key = self.avatar_bucket.Object(os.path.join("exports", secrets.token_hex(16), os.path.basename(tarball_path))) key.upload_file(tarball_path, Callback=percent_callback) @@ -672,7 +669,7 @@ class LocalUploadBackend(ZulipUploadBackend): path = "/".join([ str(user_profile.realm_id), format(random.randint(0, 255), 'x'), - random_name(18), + secrets.token_urlsafe(18), sanitize_name(uploaded_file_name), ]) @@ -819,7 +816,7 @@ class LocalUploadBackend(ZulipUploadBackend): path = os.path.join( 'exports', str(realm.id), - random_name(18), + secrets.token_urlsafe(18), os.path.basename(tarball_path), ) abs_path = os.path.join(settings.LOCAL_UPLOADS_DIR, 'avatars', path) diff --git a/zerver/lib/utils.py b/zerver/lib/utils.py index e1fcacf98d..ff0fb414e8 100644 --- a/zerver/lib/utils.py +++ b/zerver/lib/utils.py @@ -1,10 +1,8 @@ -import base64 import hashlib import heapq import itertools -import os import re -import string +import secrets from itertools import zip_longest from time import sleep from typing import Any, Callable, Iterator, List, Optional, Sequence, Set, Tuple, TypeVar @@ -107,14 +105,12 @@ def log_statsd_event(name: str) -> None: event_name = f"events.{name}" statsd.incr(event_name) -def generate_random_token(length: int) -> str: - return str(base64.b16encode(os.urandom(length // 2)).decode('utf-8').lower()) - def generate_api_key() -> str: - choices = string.ascii_letters + string.digits - altchars = ''.join(choices[ord(os.urandom(1)) % 62] for _ in range(2)).encode("utf-8") - api_key = base64.b64encode(os.urandom(24), altchars=altchars).decode("utf-8") - return api_key + api_key = "" + while len(api_key) < 32: + # One iteration suffices 99.4992% of the time. + api_key += secrets.token_urlsafe(3 * 9).replace("_", "").replace("-", "") + return api_key[:32] def has_api_key_format(key: str) -> bool: return bool(re.fullmatch(r"([A-Za-z0-9]){32}", key)) diff --git a/zerver/migrations/0260_missed_message_addresses_from_redis_to_db.py b/zerver/migrations/0260_missed_message_addresses_from_redis_to_db.py index 78bb24026e..6b0c987360 100644 --- a/zerver/migrations/0260_missed_message_addresses_from_redis_to_db.py +++ b/zerver/migrations/0260_missed_message_addresses_from_redis_to_db.py @@ -1,3 +1,5 @@ +import secrets + from django.core.exceptions import ObjectDoesNotExist from django.db import migrations from django.db.backends.postgresql.schema import DatabaseSchemaEditor @@ -5,11 +7,10 @@ from django.db.migrations.state import StateApps # Imported to avoid needing to duplicate redis-related code. from zerver.lib.redis_utils import get_redis_client -from zerver.lib.utils import generate_random_token def generate_missed_message_token() -> str: - return 'mm' + generate_random_token(32) + return 'mm' + secrets.token_hex(16) def move_missed_message_addresses_to_database(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: redis_client = get_redis_client() diff --git a/zerver/models.py b/zerver/models.py index dda87c3d27..095277c2a0 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1,6 +1,7 @@ import ast import datetime import re +import secrets import time from collections import defaultdict from datetime import timedelta @@ -78,7 +79,7 @@ from zerver.lib.types import ( UserFieldElement, Validator, ) -from zerver.lib.utils import generate_random_token, make_safe_digest +from zerver.lib.utils import make_safe_digest from zerver.lib.validator import ( check_date, check_int, @@ -1464,7 +1465,7 @@ class PushDeviceToken(AbstractPushDeviceToken): unique_together = ("user", "kind", "token") def generate_email_token_for_stream() -> str: - return generate_random_token(32) + return secrets.token_hex(16) class Stream(models.Model): MAX_NAME_LENGTH = 60 diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 58474d4895..302538b5fd 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -3,6 +3,7 @@ import copy import datetime import json import re +import secrets import time import urllib from contextlib import contextmanager @@ -67,7 +68,6 @@ from zerver.lib.test_helpers import ( ) from zerver.lib.upload import MEDIUM_AVATAR_SIZE, resize_avatar from zerver.lib.users import get_all_api_keys -from zerver.lib.utils import generate_random_token from zerver.lib.validator import ( Validator, check_bool, @@ -549,7 +549,7 @@ class RateLimitAuthenticationTests(ZulipTestCase): # We have to mock RateLimitedAuthenticationByUsername.key to avoid key collisions # if tests run in parallel. original_key_method = RateLimitedAuthenticationByUsername.key - salt = generate_random_token(32) + salt = secrets.token_hex(16) def _mock_key(self: RateLimitedAuthenticationByUsername) -> str: return f"{salt}:{original_key_method(self)}" @@ -2991,7 +2991,7 @@ class GoogleAuthBackendTest(SocialAuthBase): 'redirect_to': '', } with mock.patch("logging.warning") as mock_warn: - token = generate_random_token(ExternalAuthResult.LOGIN_TOKEN_LENGTH) + token = secrets.token_hex(ExternalAuthResult.LOGIN_TOKEN_LENGTH // 2) result = self.get_log_into_subdomain(data, force_token=token) mock_warn.assert_called_once_with("log_into_subdomain: Invalid token given: %s", token) self.assertEqual(result.status_code, 400) diff --git a/zerver/tests/test_create_video_call.py b/zerver/tests/test_create_video_call.py index 28c0ef8f07..e83b28c98b 100644 --- a/zerver/tests/test_create_video_call.py +++ b/zerver/tests/test_create_video_call.py @@ -163,7 +163,7 @@ class TestVideoCall(ZulipTestCase): def test_create_bigbluebutton_link(self) -> None: with mock.patch('zerver.views.video_calls.random.randint', return_value="1"), mock.patch( - 'zerver.views.video_calls.random.SystemRandom.choice', return_value="A"): + 'secrets.token_bytes', return_value=b"\x00" * 7): response = self.client_get("/json/calls/bigbluebutton/create") self.assert_json_success(response) self.assertEqual(response.json()['url'], diff --git a/zerver/tests/test_rate_limiter.py b/zerver/tests/test_rate_limiter.py index 898af5fb37..7cc6fa71fc 100644 --- a/zerver/tests/test_rate_limiter.py +++ b/zerver/tests/test_rate_limiter.py @@ -1,3 +1,4 @@ +import secrets import time from typing import Dict, List, Tuple, Type from unittest import mock @@ -12,9 +13,8 @@ from zerver.lib.rate_limiter import ( remove_ratelimit_rule, ) from zerver.lib.test_classes import ZulipTestCase -from zerver.lib.utils import generate_random_token -RANDOM_KEY_PREFIX = generate_random_token(32) +RANDOM_KEY_PREFIX = secrets.token_hex(16) class RateLimitedTestObject(RateLimitedObject): def __init__(self, name: str, rules: List[Tuple[int, int]], diff --git a/zerver/tests/test_redis_utils.py b/zerver/tests/test_redis_utils.py index c69a4de40c..22a1ec1114 100644 --- a/zerver/tests/test_redis_utils.py +++ b/zerver/tests/test_redis_utils.py @@ -45,7 +45,7 @@ class RedisUtilsTest(ZulipTestCase): # Trying to put data under an overly long key should get stopped before even # generating the random token. - with mock.patch("zerver.lib.redis_utils.generate_random_token") as mock_generate: + with mock.patch("secrets.token_hex") as mock_generate: with self.assertRaises(ZulipRedisKeyTooLongError): put_dict_in_redis(self.redis_client, self.key_format, data, expiration_seconds=self.expiration_seconds, diff --git a/zerver/views/auth.py b/zerver/views/auth.py index eca2c9f557..01bb40e615 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -1,5 +1,5 @@ import logging -import os +import secrets import urllib from functools import wraps from typing import Any, Dict, List, Mapping, Optional, cast @@ -289,7 +289,7 @@ def finish_desktop_flow(request: HttpRequest, user_profile: UserProfile, result = ExternalAuthResult(user_profile=user_profile) token = result.store_data() key = bytes.fromhex(otp) - iv = os.urandom(12) + iv = secrets.token_bytes(12) desktop_data = (iv + AESGCM(key).encrypt(iv, token.encode(), b"")).hex() context = {'desktop_data': desktop_data, 'browser_url': reverse('zerver.views.auth.login_page', diff --git a/zerver/views/home.py b/zerver/views/home.py index a856661cc1..cc01eee44f 100644 --- a/zerver/views/home.py +++ b/zerver/views/home.py @@ -1,4 +1,5 @@ import logging +import secrets from typing import Any, Dict, List, Optional, Tuple from django.conf import settings @@ -19,7 +20,7 @@ from zerver.lib.push_notifications import num_push_devices_for_user from zerver.lib.streams import access_stream_by_name from zerver.lib.subdomains import get_subdomain from zerver.lib.users import compute_show_invites_and_add_streams -from zerver.lib.utils import generate_random_token, statsd +from zerver.lib.utils import statsd from zerver.models import PreregistrationUser, Realm, Stream, UserProfile from zerver.views.compatibility import is_outdated_desktop_app, is_unsupported_browser from zerver.views.portico import hello_view @@ -200,7 +201,7 @@ def home_real(request: HttpRequest) -> HttpResponse: request._log_data['extra'] = "[{}]".format(queue_id) - csp_nonce = generate_random_token(48) + csp_nonce = secrets.token_hex(24) user_permission_info = get_user_permission_info(user_profile) diff --git a/zerver/views/video_calls.py b/zerver/views/video_calls.py index b435717f7d..82e7654870 100644 --- a/zerver/views/video_calls.py +++ b/zerver/views/video_calls.py @@ -1,7 +1,8 @@ import hashlib import json import random -import string +import secrets +from base64 import b32encode from functools import partial from typing import Dict from urllib.parse import quote, urlencode, urljoin @@ -164,7 +165,7 @@ def get_bigbluebutton_url(request: HttpRequest, user_profile: UserProfile) -> Ht # https://docs.bigbluebutton.org/dev/api.html#create for reference on the api calls # https://docs.bigbluebutton.org/dev/api.html#usage for reference for checksum id = "zulip-" + str(random.randint(100000000000, 999999999999)) - password = ''.join(random.SystemRandom().choice(string.ascii_uppercase + string.digits) for _ in range(10)) + password = b32encode(secrets.token_bytes(7))[:10].decode() checksum = hashlib.sha1(("create" + "meetingID=" + id + "&moderatorPW=" + password + "&attendeePW=" + password + "a" + settings.BIG_BLUE_BUTTON_SECRET).encode()).hexdigest() url = add_query_to_redirect_url("/calls/bigbluebutton/join", urlencode({