CVE-2022-3273: Inadequate Encryption Strength (CWE-326)
value: Allocation of Resources Without Limits or Throttling in GitHub repository ikus060/rdiffweb prior to 2.5.0a4.
CWE description: The product stores or transmits sensitive data using an encryption scheme that is theoretically sound, but is not strong enough for the level of protection required.
CVSS v3 base score: 9.8
Published: 2022-10-06T18:16Z
Repository: https://github.com/ikus060/rdiffweb
Language: Python
Files changed: README.md, rdiffweb/controller/__init__.py, rdiffweb/controller/page_admin_users.py, rdiffweb/controller/page_pref_general.py, rdiffweb/controller/tests/test_page_prefs_general.py, rdiffweb/core/login.py, rdiffweb/core/model/_user.py, rdiffweb/core/model/tests/test_user.py
Commit date: 2022-09-30 11:47:01 -0400
Commit message: Limit incorrect attempts to change the user's password to prevent brute force attacks #225

Diff: diff --git a/README.md b/README.md
index b237f352..98932e48 100644
--- a/README.md
+++ b/README.md
@@ -130,10 +130,11 @@ This next release focus on two-factor-authentication as a measure to increase se
 * Replace deprecated references of `disutils.spawn.find_executable()` by `shutil.which()` #208
 * Add two-factor authentication with email verification #201
 * 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:
 
 * Drop Ubuntu Hirsute & Impish (End-of-life)
 * `session-dir` is deprecated and should be replace by `rate-limit-dir`. User's session are stored in database.
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
@@ -67,11 +67,11 @@ def flash(message, level='info'):
     """
     assert message
     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)
 
 
 def get_flashed_messages():
     if 'flash' in cherrypy.session:  # @UndefinedVariable
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
@@ -157,11 +157,11 @@ class UserForm(CherryForm):
             raise ValueError(_('Cannot change your own two-factor authentication settings.'))
 
     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 ''
         userobj.user_root = self.user_root.data
         if self.mfa.data and not userobj.email:
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
@@ -27,10 +27,14 @@ from wtforms.validators import DataRequired, EqualTo, InputRequired, Length, Opt
 from rdiffweb.controller import Controller, flash
 from rdiffweb.controller.form import CherryForm
 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')
     username = StringField(_('Username'), render_kw={'readonly': True})
     fullname = StringField(
@@ -83,15 +87,34 @@ class UserPasswordForm(CherryForm):
     def is_submitted(self):
         # Validate only if action is set_profile_info
         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):
     action = HiddenField(default='update_repos')
     update_repos = SubmitField(
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
@@ -180,21 +180,36 @@ class PagePrefGeneralTest(rdiffweb.test.WebCase):
         self._set_password(self.PASSWORD, "t", "a")
         self.assertInBody("The new password and its confirmation do not match.")
 
     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")
         self.assertInBody("Password must have between 8 and 128 characters.")
 
     def test_change_password_with_too_long(self):
         new_password = 'a' * 129
         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
         self.getPage(self.PREFS + '?action=set_password&new=pr3j5Dwi&confirm=pr3j5Dwi&current=' + self.PASSWORD)
         # Then nothing happen
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
@@ -19,11 +19,10 @@ import logging
 
 import cherrypy
 from cherrypy.process.plugins import SimplePlugin
 
 from rdiffweb.core.model import UserObject
-from rdiffweb.core.passwd import check_password
 
 logger = logging.getLogger(__name__)
 
 
 class LoginPlugin(SimplePlugin):
@@ -50,11 +49,11 @@ class LoginPlugin(SimplePlugin):
     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
 
     def login(self, username, password):
         """
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
@@ -342,28 +342,24 @@ class UserObject(Base):
 
     @hybrid_property
     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
 
         # Cannot update admin-password if defined
         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(
                 _('Password must have between %(min)d and %(max)d characters.')
                 % {'min': cfg.password_min_length, 'max': cfg.password_max_length}
@@ -446,10 +442,13 @@ class UserObject(Base):
                 # When it matches, let update the record.
                 access_token.access_time = datetime.datetime.utcnow
                 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):
     if value and value != oldvalue:
         cherrypy.engine.publish('user_password_changed', target)
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
@@ -198,32 +198,10 @@ class UserObjectTest(rdiffweb.test.WebCase):
         # 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_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')
         self.assertIsNotNone(UserObject.get_user('vicky'))
         # When deleting that user

Vulnerable code:     flash_message = FlashMessage(message, level)
            userobj.set_password(self.password.data, old_password=None)
        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')
        self.assertInBody("Wrong password")
from rdiffweb.core.passwd import check_password
        if user and check_password(password, user.hash_password):
    def set_password(self, password, old_password=None):
        assert old_password is None or isinstance(old_password, str)
        if old_password and not check_password(old_password, self.hash_password):
            raise ValueError(_("Wrong password"))

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

Fixed code: * 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)
    flash_message = FlashMessage(str(message), level)
            userobj.set_password(self.password.data)
# Maximum number of password change attempt before logout
CHANGE_PASSWORD_MAX_ATTEMPT = 5
CHANGE_PASSWORD_ATTEMPTS = 'change_password_attempts'

        # 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')
        self.assertInBody("Wrong current password")
    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.')

        if user and user.validate_password(password):
    def set_password(self, password):
    def validate_password(self, password):
        return check_password(password, self.hash_password)