Limit incorrect attempts to change the user's password to prevent bru…

…te force attacks #225

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:
 

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)
 
 

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 ''

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):

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

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
 

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):

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')