diff --git a/max/auth/__init__.py b/max/auth/__init__.py index 27525f0..b041bca 100644 --- a/max/auth/__init__.py +++ b/max/auth/__init__.py @@ -1,4 +1,9 @@ -from .auth import change_password, check_password, hash_password # noqa +from .auth import ( # noqa + change_password, + check_password, + hash_password, + validate_password, +) from .middlewares import AuthMiddleware, no_authentication_required # noqa from .routes import init_routes diff --git a/max/auth/auth.py b/max/auth/auth.py index 4bb1c29..8ddbc19 100644 --- a/max/auth/auth.py +++ b/max/auth/auth.py @@ -1,16 +1,31 @@ import base64 import hashlib -import os -from flask import session +import bcrypt +from flask import current_app, session from max.crypto import constant_time_compare, force_bytes from max.db import get_user_by_email, update_user +from max.exceptions import InvalidPassword from max.sessions import session_storage from max.translations import t +BLF_CRYPT_PREFIX = "{BLF-CRYPT}" SSHA512_PREFIX = "{SSHA512}" +BCRYPT_ROUNDS = 12 +# bcrypt only uses the first 72 bytes +MAX_PASSWORD_BYTES = 72 +# Hash of a random, unknown password. It is checked for unknown users, so the +# response time does not tell whether an email exists. +DUMMY_HASH = ( + BLF_CRYPT_PREFIX + "$2b$12$J27D9dEpQ5ff/I48HG05p.97oJGTO7UiaKGIbVuzV6nw0lo6gZ5wq" +) + LOGIN_FAILED_MESSAGE = "Email or password is wrong, or the account is deactivated" +PASSWORD_TOO_LONG_MESSAGE = ( + "The password must be at most 72 bytes long " + "(72 characters, fewer with umlauts or special characters)" +) def login(email, password): @@ -21,6 +36,7 @@ def login(email, password): user = get_user_by_email(email) if user is None: + check_password(password, DUMMY_HASH) return t(LOGIN_FAILED_MESSAGE) if not check_password(password, user.passwordhash): @@ -29,6 +45,8 @@ def login(email, password): if not user.enabled: return t(LOGIN_FAILED_MESSAGE) + rehash_password_if_needed(user, password) + # Start a new session (new id, new CSRF token) against session fixation. session.clear() session["user_id"] = user.id @@ -51,10 +69,63 @@ def change_password(user, password): session_storage.remove_user_sessions(user.id) -def hash_password(password: str, salt: bytes = None): - if salt is None: - salt = os.urandom(32) +def rehash_password_if_needed(user, password): + """Replaces an outdated hash, the plain password is only known at the login""" + if not needs_rehash(user.passwordhash): + return + error = validate_password(password) + if error: + current_app.logger.warning( + 'Cannot rehash the password of user %s: "%s"', user.id, error + ) + return + update_user(user.id, passwordhash=hash_password(password)) + current_app.logger.info("Rehashed the password of user %s", user.id) + +def validate_password(password: str): + """Returns the (untranslated) error message, or None if the password is valid""" + if len(force_bytes(password)) > MAX_PASSWORD_BYTES: + return PASSWORD_TOO_LONG_MESSAGE + return None + + +def hash_password(password: str): + error = validate_password(password) + if error: + raise InvalidPassword(error) + + hash = bcrypt.hashpw(force_bytes(password), bcrypt.gensalt(rounds=BCRYPT_ROUNDS)) + return BLF_CRYPT_PREFIX + hash.decode() + + +def needs_rehash(hash: str): + if not hash.startswith(BLF_CRYPT_PREFIX): + return True + # "$2b$12$…": the cost factor follows the version + rounds = int(hash[len(BLF_CRYPT_PREFIX) :].split("$")[2]) # noqa + return rounds < BCRYPT_ROUNDS + + +def check_password(password: str, hash: str): + if hash.startswith(BLF_CRYPT_PREFIX): + # Such a password cannot have been set, and bcrypt would raise + if validate_password(password): + return False + bcrypt_hash = hash[len(BLF_CRYPT_PREFIX) :] # noqa + return bcrypt.checkpw(force_bytes(password), force_bytes(bcrypt_hash)) + + if hash.startswith(SSHA512_PREFIX): + return check_ssha512_password(password, hash) + + raise RuntimeError("The hash has an unknown scheme") + + +# {SSHA512} is outdated. Existing hashes are still checked and replaced at the +# next login. Remove this once no hash uses it anymore (see F-08). + + +def hash_ssha512_password(password: str, salt: bytes): sha = hashlib.sha512() sha.update(force_bytes(password)) sha.update(salt) @@ -62,16 +133,13 @@ def hash_password(password: str, salt: bytes = None): return SSHA512_PREFIX + sha_and_salt.decode() -def get_salt(hash: str): - if not hash.startswith(SSHA512_PREFIX): - raise RuntimeError(f"The hash does not start with '{SSHA512_PREFIX}''") - +def get_ssha512_salt(hash: str): sha_and_salt = base64.b64decode(hash[len(SSHA512_PREFIX) :]) # noqa return sha_and_salt[64:] -def check_password(password: str, hash: str): - salt = get_salt(hash) - hash_from_password = hash_password(password, salt=salt) +def check_ssha512_password(password: str, hash: str): + salt = get_ssha512_salt(hash) + hash_from_password = hash_ssha512_password(password, salt) return constant_time_compare(hash, hash_from_password) diff --git a/max/cli.py b/max/cli.py index f74a80f..6e5fb67 100644 --- a/max/cli.py +++ b/max/cli.py @@ -1,6 +1,10 @@ import click -from .auth import check_password as auth_check_password, hash_password +from .auth import ( + check_password as auth_check_password, + hash_password, + validate_password, +) from .db import ( add_domain as db_add_domain, remove_domain as db_remove_domain, @@ -17,10 +21,17 @@ def register_cli(app): app.cli.command("remove-domain")(remove_domain) +def abort_if_password_invalid(password): + error = validate_password(password) + if error: + raise click.ClickException(error) + + @click.argument("email") @click.argument("password") def reset_user(email, password): """Resets or create a user given by the email""" + abort_if_password_invalid(password) hash = hash_password(password) reset_or_create_user(email, hash, False) @@ -29,6 +40,7 @@ def reset_user(email, password): @click.argument("password") def reset_admin(email, password): """Resets or create an admin given by the email""" + abort_if_password_invalid(password) hash = hash_password(password) reset_or_create_user(email, hash, True) @@ -36,6 +48,7 @@ def reset_admin(email, password): @click.argument("password") def make_hash(password): """Creates a salted hash from the password""" + abort_if_password_invalid(password) print(hash_password(password)) diff --git a/max/exceptions.py b/max/exceptions.py index cf94b62..025fbd4 100644 --- a/max/exceptions.py +++ b/max/exceptions.py @@ -21,6 +21,12 @@ class PermissionDenied(MaxException): """ +class InvalidPassword(MaxException): + """ + Callers must check the password with validate_password before hashing it. + """ + + class LanguageNotSet(MaxException): """ Something is translated outside of the I18NMiddleware, which sets the diff --git a/max/private_static/i18n/de.json b/max/private_static/i18n/de.json index 9229a64..145b238 100644 --- a/max/private_static/i18n/de.json +++ b/max/private_static/i18n/de.json @@ -50,6 +50,7 @@ "Note changed successfully!": "Notiz wurde erfolgreich bearbeitet!", "Both passwords must be given": "Beide Passwörter müssen angegeben werden", "Both passwords must be the same": "Beide Passwörter müssen übereinstimmen", + "The password must be at most 72 bytes long (72 characters, fewer with umlauts or special characters)": "Das Passwort darf höchstens 72 Bytes lang sein (72 Zeichen, mit Umlauten oder Sonderzeichen weniger)", "Current password": "Aktuelles Passwort", "The current password must be given": "Das aktuelle Passwort muss angegeben werden", "The current password is wrong": "Das aktuelle Passwort ist falsch", diff --git a/max/routes/user/change_password.py b/max/routes/user/change_password.py index 1654362..6f58c55 100644 --- a/max/routes/user/change_password.py +++ b/max/routes/user/change_password.py @@ -1,6 +1,6 @@ from flask import flash -from max.auth import change_password, check_password +from max.auth import change_password, check_password, validate_password from max.permissions import AllowAdminOrSelf from max.translations import t @@ -45,6 +45,11 @@ class UserChangePassword(AllowAdminOrSelf, BackToUsersMixin, FetchUserTemplateGe flash(t("Both passwords must be the same"), category="error") return + password_error = validate_password(pw1) + if password_error: + flash(t(password_error), category="error") + return + change_password(self.user, pw1) flash(t("Password changed successfully!"), category="success") return self.redirect() diff --git a/max/routes/user/create.py b/max/routes/user/create.py index af06428..f9a6b16 100644 --- a/max/routes/user/create.py +++ b/max/routes/user/create.py @@ -1,6 +1,6 @@ from flask import flash, url_for -from max.auth import hash_password +from max.auth import hash_password, validate_password from max.db import EmailAlreadyExists, create_user, get_domains from max.permissions import AllowAdmin from max.translations import t @@ -48,7 +48,12 @@ class UserCreate(AllowAdmin, CheckEmailMixin, BaseTemplateGetView): flash(t("Both passwords must be the same"), category="error") error = True else: - self.password = pw1 + password_error = validate_password(pw1) + if password_error: + flash(t(password_error), category="error") + error = True + else: + self.password = pw1 enabled = self.request.form.get("enabled") if enabled == "on": diff --git a/max/templates/user/change-password.html b/max/templates/user/change-password.html index 1da5ccf..e65432c 100644 --- a/max/templates/user/change-password.html +++ b/max/templates/user/change-password.html @@ -14,12 +14,12 @@ {% endif %}