From b5e3bb0a98268d18ceead36ab9b2b7eaacd659a8 Mon Sep 17 00:00:00 2001 From: Patrik Dufresne Date: Fri, 30 Sep 2022 11:47:01 -0400 Subject: [PATCH] Limit incorrect attempts to change the user's password to prevent brute force attacks #225 --- README.md | 1 + rdiffweb/controller/__init__.py | 2 +- rdiffweb/controller/page_admin_users.py | 2 +- rdiffweb/controller/page_pref_general.py | 33 ++++++++++++++++--- .../tests/test_page_prefs_general.py | 17 +++++++++- rdiffweb/core/login.py | 3 +- rdiffweb/core/model/_user.py | 9 +++-- rdiffweb/core/model/tests/test_user.py | 22 ------------- 8 files changed, 52 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index b237f352..98932e48 100644 --- a/README.md +++ b/README.md @@ -132,6 +132,7 @@ This next release focus on two-factor-authentication as a measure to increase se * Generate a new session on login and 2FA #220 * Enforce permission on /etc/rdiffweb configuration folder * Enforce validation on fullname, username and email +* Limit incorrect attempts to change the user's password to prevent brute force attacks #225 [CVE-2022-3273](https://nvd.nist.gov/vuln/detail/CVE-2022-3273) Breaking changes: diff --git a/rdiffweb/controller/__init__.py b/rdiffweb/controller/__init__.py index 18d882a7..00b866eb 100644 --- a/rdiffweb/controller/__init__.py +++ b/rdiffweb/controller/__init__.py @@ -69,7 +69,7 @@ def flash(message, level='info'): assert level in ['info', 'error', 'warning', 'success'] if 'flash' not in cherrypy.session: # @UndefinedVariable cherrypy.session['flash'] = [] # @UndefinedVariable - flash_message = FlashMessage(message, level) + flash_message = FlashMessage(str(message), level) cherrypy.session['flash'].append(flash_message) diff --git a/rdiffweb/controller/page_admin_users.py b/rdiffweb/controller/page_admin_users.py index 953ee719..668126f8 100644 --- a/rdiffweb/controller/page_admin_users.py +++ b/rdiffweb/controller/page_admin_users.py @@ -159,7 +159,7 @@ def validate_mfa(self, field): def populate_obj(self, userobj): # Save password if defined if self.password.data: - userobj.set_password(self.password.data, old_password=None) + userobj.set_password(self.password.data) userobj.role = self.role.data userobj.fullname = self.fullname.data or '' userobj.email = self.email.data or '' diff --git a/rdiffweb/controller/page_pref_general.py b/rdiffweb/controller/page_pref_general.py index df2b731c..453378ba 100644 --- a/rdiffweb/controller/page_pref_general.py +++ b/rdiffweb/controller/page_pref_general.py @@ -29,6 +29,10 @@ from rdiffweb.core.model import UserObject from rdiffweb.tools.i18n import gettext_lazy as _ +# Maximum number of password change attempt before logout +CHANGE_PASSWORD_MAX_ATTEMPT = 5 +CHANGE_PASSWORD_ATTEMPTS = 'change_password_attempts' + class UserProfileForm(CherryForm): action = HiddenField(default='set_profile_info') @@ -85,11 +89,30 @@ def is_submitted(self): return super().is_submitted() and self.action.data == 'set_password' def populate_obj(self, user): - try: - user.set_password(self.new.data, old_password=self.current.data) - flash(_("Password updated successfully."), level='success') - except ValueError as e: - flash(str(e), level='warning') + # Check if current password is "valid" if Not, rate limit the + # number of attempts and logout user after too many invalid attempts. + if not user.validate_password(self.current.data): + cherrypy.session[CHANGE_PASSWORD_ATTEMPTS] = cherrypy.session.get(CHANGE_PASSWORD_ATTEMPTS, 0) + 1 + attempts = cherrypy.session[CHANGE_PASSWORD_ATTEMPTS] + if attempts >= CHANGE_PASSWORD_MAX_ATTEMPT: + cherrypy.session.clear() + cherrypy.session.regenerate() + flash( + _("You were logged out because you entered the wrong password too many times."), + level='warning', + ) + raise cherrypy.HTTPRedirect('/login/') + flash(_("Wrong current password."), level='warning') + else: + # Clear number of attempts + if CHANGE_PASSWORD_ATTEMPTS in cherrypy.session: + del cherrypy.session[CHANGE_PASSWORD_ATTEMPTS] + # If Valid, update password + try: + user.set_password(self.new.data) + flash(_("Password updated successfully."), level='success') + except ValueError as e: + flash(str(e), level='warning') class RefreshForm(CherryForm): diff --git a/rdiffweb/controller/tests/test_page_prefs_general.py b/rdiffweb/controller/tests/test_page_prefs_general.py index b46765d1..15b8ee41 100644 --- a/rdiffweb/controller/tests/test_page_prefs_general.py +++ b/rdiffweb/controller/tests/test_page_prefs_general.py @@ -182,7 +182,7 @@ def test_change_password_with_wrong_confirmation(self): def test_change_password_with_wrong_password(self): self._set_password("oups", "pr3j5Dwi", "pr3j5Dwi") - self.assertInBody("Wrong password") + self.assertInBody("Wrong current password") def test_change_password_with_too_short(self): self._set_password(self.PASSWORD, "short", "short") @@ -193,6 +193,21 @@ def test_change_password_with_too_long(self): self._set_password(self.PASSWORD, new_password, new_password) self.assertInBody("Password must have between 8 and 128 characters.") + def test_change_password_too_many_attemps(self): + # When udating user's password with wrong current password 5 times + for _i in range(1, 5): + self._set_password('wrong', "pr3j5Dwi", "pr3j5Dwi") + self.assertStatus(200) + self.assertInBody("Wrong current password.") + # Then user session is cleared and user is redirect to login page + self._set_password('wrong', "pr3j5Dwi", "pr3j5Dwi") + self.assertStatus(303) + self.assertHeaderItemValue('Location', self.baseurl + '/login/') + # Then a warning message is displayed on login page + self.getPage('/login/') + self.assertStatus(200) + self.assertInBody('You were logged out because you entered the wrong password too many times.') + def test_change_password_method_get(self): # Given an authenticated user # Trying to update password with GET method diff --git a/rdiffweb/core/login.py b/rdiffweb/core/login.py index a3c43519..aca9903f 100644 --- a/rdiffweb/core/login.py +++ b/rdiffweb/core/login.py @@ -21,7 +21,6 @@ from cherrypy.process.plugins import SimplePlugin from rdiffweb.core.model import UserObject -from rdiffweb.core.passwd import check_password logger = logging.getLogger(__name__) @@ -52,7 +51,7 @@ def authenticate(self, username, password): Only verify the user's credentials using the database store. """ user = UserObject.query.filter_by(username=username).first() - if user and check_password(password, user.hash_password): + if user and user.validate_password(password): return username, {} return False diff --git a/rdiffweb/core/model/_user.py b/rdiffweb/core/model/_user.py index 441e10b3..d2bbcd09 100644 --- a/rdiffweb/core/model/_user.py +++ b/rdiffweb/core/model/_user.py @@ -344,13 +344,12 @@ def is_ldap(cls): def is_maintainer(self): return self.role <= self.MAINTAINER_ROLE - def set_password(self, password, old_password=None): + def set_password(self, password): """ Change the user's password. Raise a ValueError if the username or the password are invalid. """ assert isinstance(password, str) - assert old_password is None or isinstance(old_password, str) if not password: raise ValueError("password can't be empty") cfg = cherrypy.tree.apps[''].cfg @@ -359,9 +358,6 @@ def set_password(self, password, old_password=None): if self.username == cfg.admin_user and cfg.admin_password: raise ValueError(_("can't update admin-password defined in configuration file")) - if old_password and not check_password(old_password, self.hash_password): - raise ValueError(_("Wrong password")) - # Check password length if cfg.password_min_length > len(password) or len(password) > cfg.password_max_length: raise ValueError( @@ -448,6 +444,9 @@ def validate_access_token(self, token): return True return False + def validate_password(self, password): + return check_password(password, self.hash_password) + @event.listens_for(UserObject.hash_password, "set") def hash_password_set(target, value, oldvalue, initiator): diff --git a/rdiffweb/core/model/tests/test_user.py b/rdiffweb/core/model/tests/test_user.py index ff27b7ca..b5258958 100644 --- a/rdiffweb/core/model/tests/test_user.py +++ b/rdiffweb/core/model/tests/test_user.py @@ -200,28 +200,6 @@ def test_set_password_update(self): # Check if listener called self.listener.user_password_changed.assert_called_once_with(userobj) - def test_set_password_with_old_password(self): - # Given a user in drabase with a password - userobj = UserObject.add_user('john', 'password') - self.listener.user_password_changed.reset_mock() - # When updating the user's password with old_password - userobj.set_password('new_password', old_password='password') - # Then password is SSHA - self.assertTrue(check_password('new_password', userobj.hash_password)) - # Check if listener called - self.listener.user_password_changed.assert_called_once_with(userobj) - - def test_set_password_with_invalid_old_password(self): - # Given a user in drabase with a password - userobj = UserObject.add_user('foo', 'password') - self.listener.user_password_changed.reset_mock() - # When updating the user's password with wrong old_password - # Then an exception is raised - with self.assertRaises(ValueError): - userobj.set_password('new_password', old_password='invalid') - # Check if listener called - self.listener.user_password_changed.assert_not_called() - def test_delete_user(self): # Given an existing user in database userobj = UserObject.add_user('vicky')