From 59731c53287382ca314bade3d13253ff3a72345f Mon Sep 17 00:00:00 2001 From: rmlibre Date: Tue, 30 Jul 2024 23:53:18 -0400 Subject: [PATCH 1/4] refactor: swap conditional logic --- hushline/settings.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/hushline/settings.py b/hushline/settings.py index a78ec98a..aa1bd158 100644 --- a/hushline/settings.py +++ b/hushline/settings.py @@ -260,19 +260,18 @@ def change_password() -> str | Response: return redirect(url_for("settings.index")) # Verify the old password - if not user.check_password(change_password_form.old_password.data): - flash("Incorrect old password.", "error") - return redirect(url_for("settings.index")) - - # Set the new password - user.password_hash = change_password_form.new_password.data - db.session.commit() - session.clear() # Clears the session, logging the user out - flash( - "👍 Password successfully changed. Please log in again.", - "success", - ) - return redirect(url_for("login")) # Redirect to the login page for re-authentication + if user.check_password(change_password_form.old_password.data): + # Set the new password + user.password_hash = change_password_form.new_password.data + db.session.commit() + session.clear() # Clears the session, logging the user out + flash( + "👍 Password successfully changed. Please log in again.", + "success", + ) + return redirect(url_for("login")) # Redirect to the login page for re-authentication + flash("Incorrect old password.", "error") + return redirect(url_for("settings.index")) @bp.route("/change-username", methods=["POST"]) @require_2fa From 9ec5eb010edcba9e0cb7b70a34f04cac142c4861 Mon Sep 17 00:00:00 2001 From: rmlibre Date: Wed, 31 Jul 2024 00:23:38 -0400 Subject: [PATCH 2/4] fix: safely ensure new passwords are distinct Suggested in review from PR [#447]: https://github.com/scidsg/hushline/pull/447#discussion_r1697767396 --- hushline/forms.py | 12 ++++++++++++ hushline/settings.py | 14 +++++++++++--- tests/test_settings.py | 14 ++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/hushline/forms.py b/hushline/forms.py index 3bc1a0ef..9efd0dd7 100644 --- a/hushline/forms.py +++ b/hushline/forms.py @@ -1,4 +1,6 @@ import re +from hmac import compare_digest as bytes_are_equal +from typing import Callable from flask_wtf import FlaskForm from wtforms import Field, Form, StringField @@ -25,5 +27,15 @@ def __call__(self, form: Form, field: Field) -> None: raise ValidationError(self.message) +def is_valid_password_swap( + *, check_password: Callable[[str], bool], old_password: str, new_password: str +) -> bool: + # since the passwords can be of different lengths, the equality test must occur *iif* + # the correctness test passes, since timing differences leak length information + if check_password(old_password): + return not bytes_are_equal(old_password.encode(), new_password.encode()) + return False + + class TwoFactorForm(FlaskForm): verification_code = StringField("2FA Code", validators=[DataRequired(), Length(min=6, max=6)]) diff --git a/hushline/settings.py b/hushline/settings.py index aa1bd158..e199301d 100644 --- a/hushline/settings.py +++ b/hushline/settings.py @@ -21,7 +21,7 @@ from .crypto import is_valid_pgp_key from .db import db -from .forms import ComplexPassword, TwoFactorForm +from .forms import ComplexPassword, TwoFactorForm, is_valid_password_swap from .model import Message, SecondaryUsername, User from .utils import require_2fa @@ -176,7 +176,11 @@ def index() -> str | Response: # noqa: PLR0911, PLR0912 # Handle Change Password Form Submission if change_password_form.validate_on_submit(): - if user.check_password(change_password_form.old_password.data): + if is_valid_password_swap( + check_password=user.check_password, + old_password=change_password_form.old_password.data, + new_password=change_password_form.new_password.data, + ): user.password_hash = change_password_form.new_password.data db.session.commit() flash("👍 Password changed successfully.") @@ -260,7 +264,11 @@ def change_password() -> str | Response: return redirect(url_for("settings.index")) # Verify the old password - if user.check_password(change_password_form.old_password.data): + if is_valid_password_swap( + check_password=user.check_password, + old_password=change_password_form.old_password.data, + new_password=change_password_form.new_password.data, + ): # Set the new password user.password_hash = change_password_form.new_password.data db.session.commit() diff --git a/tests/test_settings.py b/tests/test_settings.py index cc4b59b6..f5da9633 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -110,6 +110,20 @@ def test_change_password(client: FlaskClient) -> None: assert logged_in_user is not None assert user.id == logged_in_user.id + # Submit POST request to verify new password must be distinct from the previous + response = client.post( + "/settings/change-password", + data={ + "old_password": original_password, + "new_password": original_password, + }, + follow_redirects=True, + ) + assert response.status_code == 200 + assert "settings" in response.request.url + assert b"Incorrect old password" in response.data + assert b"Password successfully changed" not in response.data + # Submit POST request to change the username & verify update was successful response = client.post( "/settings/change-password", From d9bfd9d63051b934eb97f5412b56c529fdbd2242 Mon Sep 17 00:00:00 2001 From: rmlibre Date: Wed, 31 Jul 2024 14:21:33 -0400 Subject: [PATCH 3/4] docs: normalize password change flash messages --- hushline/settings.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/hushline/settings.py b/hushline/settings.py index e199301d..91303cd6 100644 --- a/hushline/settings.py +++ b/hushline/settings.py @@ -183,9 +183,10 @@ def index() -> str | Response: # noqa: PLR0911, PLR0912 ): user.password_hash = change_password_form.new_password.data db.session.commit() - flash("👍 Password changed successfully.") - else: - flash("⛔️ Incorrect old password.") + flash("👍 Password successfully changed. Please log in again.", "success") + # Redirect to the login page for re-authentication + return redirect(url_for("login")) + flash("⛔️ Incorrect old password.", "error") return redirect(url_for("settings")) # Check if user is admin and add admin-specific data @@ -260,7 +261,7 @@ def change_password() -> str | Response: change_password_form = ChangePasswordForm(request.form) if not change_password_form.validate_on_submit(): - flash("New password is invalid.") + flash("⛔️ New password is invalid.", "error") return redirect(url_for("settings.index")) # Verify the old password @@ -273,12 +274,10 @@ def change_password() -> str | Response: user.password_hash = change_password_form.new_password.data db.session.commit() session.clear() # Clears the session, logging the user out - flash( - "👍 Password successfully changed. Please log in again.", - "success", - ) - return redirect(url_for("login")) # Redirect to the login page for re-authentication - flash("Incorrect old password.", "error") + flash("👍 Password successfully changed. Please log in again.", "success") + # Redirect to the login page for re-authentication + return redirect(url_for("login")) + flash("⛔️ Incorrect old password.", "error") return redirect(url_for("settings.index")) @bp.route("/change-username", methods=["POST"]) From 9dfcdc5e7c84a8467b7d9260f004c8680098f3b2 Mon Sep 17 00:00:00 2001 From: rmlibre Date: Wed, 31 Jul 2024 15:27:39 -0400 Subject: [PATCH 4/4] fix: safely flash reiterated password error on change attempt --- hushline/forms.py | 6 +++- hushline/settings.py | 62 ++++++++++++++++++++++-------------------- tests/test_settings.py | 2 +- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/hushline/forms.py b/hushline/forms.py index 9efd0dd7..ee52b52b 100644 --- a/hushline/forms.py +++ b/hushline/forms.py @@ -7,6 +7,10 @@ from wtforms.validators import DataRequired, Length, ValidationError +class WrongPassword(ValueError): + pass + + class ComplexPassword: def __init__(self, message: str | None = None) -> None: if not message: @@ -34,7 +38,7 @@ def is_valid_password_swap( # the correctness test passes, since timing differences leak length information if check_password(old_password): return not bytes_are_equal(old_password.encode(), new_password.encode()) - return False + raise WrongPassword class TwoFactorForm(FlaskForm): diff --git a/hushline/settings.py b/hushline/settings.py index 91303cd6..7319543d 100644 --- a/hushline/settings.py +++ b/hushline/settings.py @@ -21,7 +21,7 @@ from .crypto import is_valid_pgp_key from .db import db -from .forms import ComplexPassword, TwoFactorForm, is_valid_password_swap +from .forms import ComplexPassword, TwoFactorForm, WrongPassword, is_valid_password_swap from .model import Message, SecondaryUsername, User from .utils import require_2fa @@ -176,17 +176,20 @@ def index() -> str | Response: # noqa: PLR0911, PLR0912 # Handle Change Password Form Submission if change_password_form.validate_on_submit(): - if is_valid_password_swap( - check_password=user.check_password, - old_password=change_password_form.old_password.data, - new_password=change_password_form.new_password.data, - ): - user.password_hash = change_password_form.new_password.data - db.session.commit() - flash("👍 Password successfully changed. Please log in again.", "success") - # Redirect to the login page for re-authentication - return redirect(url_for("login")) - flash("⛔️ Incorrect old password.", "error") + try: + if is_valid_password_swap( + check_password=user.check_password, + old_password=change_password_form.old_password.data, + new_password=change_password_form.new_password.data, + ): + user.password_hash = change_password_form.new_password.data + db.session.commit() + flash("👍 Password successfully changed. Please log in again.", "success") + # Redirect to the login page for re-authentication + return redirect(url_for("login")) + flash("⛔️ New password is invalid.", "error") + except WrongPassword: + flash("⛔️ Incorrect old password.", "error") return redirect(url_for("settings")) # Check if user is admin and add admin-specific data @@ -259,25 +262,24 @@ def change_password() -> str | Response: flash("User not found.", "error") return redirect(url_for("login")) - change_password_form = ChangePasswordForm(request.form) - if not change_password_form.validate_on_submit(): + # Validate the password swap + try: + change_password_form = ChangePasswordForm(request.form) + if change_password_form.validate_on_submit() and is_valid_password_swap( + check_password=user.check_password, + old_password=change_password_form.old_password.data, + new_password=change_password_form.new_password.data, + ): + # Set the new password + user.password_hash = change_password_form.new_password.data + db.session.commit() + session.clear() # Clears the session, logging the user out + flash("👍 Password successfully changed. Please log in again.", "success") + # Redirect to the login page for re-authentication + return redirect(url_for("login")) flash("⛔️ New password is invalid.", "error") - return redirect(url_for("settings.index")) - - # Verify the old password - if is_valid_password_swap( - check_password=user.check_password, - old_password=change_password_form.old_password.data, - new_password=change_password_form.new_password.data, - ): - # Set the new password - user.password_hash = change_password_form.new_password.data - db.session.commit() - session.clear() # Clears the session, logging the user out - flash("👍 Password successfully changed. Please log in again.", "success") - # Redirect to the login page for re-authentication - return redirect(url_for("login")) - flash("⛔️ Incorrect old password.", "error") + except WrongPassword: + flash("⛔️ Incorrect old password.", "error") return redirect(url_for("settings.index")) @bp.route("/change-username", methods=["POST"]) diff --git a/tests/test_settings.py b/tests/test_settings.py index f5da9633..6d12832c 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -121,7 +121,7 @@ def test_change_password(client: FlaskClient) -> None: ) assert response.status_code == 200 assert "settings" in response.request.url - assert b"Incorrect old password" in response.data + assert b"New password is invalid" in response.data assert b"Password successfully changed" not in response.data # Submit POST request to change the username & verify update was successful