CVE-2022-3179: Weak Password Requirements (CWE-521)
value: Weak Password Requirements in GitHub repository ikus060/rdiffweb prior to 2.4.2.
CWE description: The product does not require that users should have strong passwords, which makes it easier for attackers to compromise user accounts.
CVSS v3 base score: 8.8
Published: 2022-09-13T17:15Z
Repository: https://github.com/ikus060/rdiffweb
Language: Python
Files changed: README.md, doc/configuration.md, rdiffweb/controller/page_admin.py, rdiffweb/controller/pref_general.py, rdiffweb/controller/tests/test_page_admin.py, rdiffweb/controller/tests/test_page_prefs.py, rdiffweb/core/config.py, rdiffweb/core/store.py
Commit date: 2022-09-12 11:17:51 -0400
Commit message: Enforce minimum and maximum password length #211

Diff: diff --git a/README.md b/README.md
index 85b66d52..3aa065e8 100644
--- a/README.md
+++ b/README.md
@@ -111,10 +111,11 @@ Professional support for Rdiffweb is available by contacting [IKUS Soft](https:/
 
 This releases include a security fix. If you are using an earlier version, you should upgrade to this release immediately.
 
 * Use 'Secure' Attribute with Sensitive Cookie in HTTPS Session. [CVE-2022-3174](https://nvd.nist.gov/vuln/detail/CVE-2022-3174) #209
 * Avoid leakage of the stack trace in the default error page. [CVE-2022-3175](https://nvd.nist.gov/vuln/detail/CVE-2022-3175) #210
+* Enforce minimum and maximum password length #211
 
 ## 2.4.1 (2022-09-08)
 
 This releases include a security fix. If you are using an earlier version, you should upgrade to this release immediately.
 
diff --git a/doc/configuration.md b/doc/configuration.md
index 0b0a3993..6e4556da 100644
--- a/doc/configuration.md
+++ b/doc/configuration.md
@@ -276,10 +276,24 @@ session even if the web server gets restarted, you may persist them on disk with
 
 | Option | Description | Example | 
 | --- | --- | --- |
 | session-dir | location where to store user session information. When undefined, the user sessions are kept in memory. | /var/lib/rdiffweb/session |
 
+## Custom user's password length limits
+
+By default, Rdiffweb supports passwords with the following lengths:
+
+* Minimum: 8 characters
+* Maximum: 128 characters
+
+Changing the minimum or maximum length does not affect existing users' passwords. Existing users are not prompted to reset their passwords to meet the new limits. The new limit only applies when an existing user changes their password.
+
+| Option | Description | Example |
+| --- | --- | --- |
+| password-min-length | Minimum length of the user's password | 8 |
+| password-max-length | Maximum length of the user's password | 128 |
+
 ## Configure Rdiffweb appearance
 
 A number of options are available to customize the appearance of Rdiffweb to your
 need. Most likely, you will want to make it closer to your business brand.
 
diff --git a/rdiffweb/controller/page_admin.py b/rdiffweb/controller/page_admin.py
index 1c685de4..dc367fc3 100644
--- a/rdiffweb/controller/page_admin.py
+++ b/rdiffweb/controller/page_admin.py
@@ -166,11 +166,11 @@ class SizeField(Field):
 
 class UserForm(CherryForm):
     userid = StringField(_('UserID'))
     username = StringField(_('Username'), validators=[validators.data_required()])
     email = EmailField(_('Email'), validators=[validators.optional()])
-    password = PasswordField(_('Password'))
+    password = PasswordField(_('Password'), validators=[validators.optional()])
     user_root = StringField(
         _('Root directory'), description=_("Absolute path defining the location of the repositories for this user.")
     )
     role = SelectField(
         _('User Role'),
@@ -188,10 +188,24 @@ class UserForm(CherryForm):
     )
     disk_usage = SizeField(
         _('Quota Used'), validators=[validators.optional()], description=_("Disk spaces (in bytes) used by this user.")
     )
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.password.validators += [
+            validators.length(
+                min=self.app.cfg.password_min_length,
+                max=self.app.cfg.password_max_length,
+                message=_('Password must have between %(min)d and %(max)d characters.'),
+            )
+        ]
+
+    @property
+    def app(self):
+        return cherrypy.request.app
+
     def validate_role(self, field):
         # Don't allow the user to changes it's "role" state.
         currentuser = cherrypy.request.currentuser
         if self.username.data == currentuser.username and self.role.data != currentuser.role:
             raise ValueError(_('Cannot edit your own role.'))
diff --git a/rdiffweb/controller/pref_general.py b/rdiffweb/controller/pref_general.py
index 804ac39a..2720b6b1 100644
--- a/rdiffweb/controller/pref_general.py
+++ b/rdiffweb/controller/pref_general.py
@@ -23,11 +23,11 @@ import logging
 import re
 
 import cherrypy
 from wtforms.fields.html5 import EmailField
 from wtforms.fields.simple import PasswordField
-from wtforms.validators import DataRequired, EqualTo, InputRequired, Regexp
+from wtforms.validators import DataRequired, EqualTo, InputRequired, Length, Regexp
 
 from rdiffweb.controller import Controller, flash
 from rdiffweb.controller.cherrypy_wtf import CherryForm
 from rdiffweb.tools.i18n import ugettext as _
 
@@ -52,10 +52,24 @@ class UserPasswordForm(CherryForm):
     )
     confirm = PasswordField(
         _('Confirm new password'), validators=[InputRequired(_("Confirmation password is missing."))]
     )
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.new.validators += [
+            Length(
+                min=self.app.cfg.password_min_length,
+                max=self.app.cfg.password_max_length,
+                message=_('Password must have between %(min)d and %(max)d characters.'),
+            )
+        ]
+
+    @property
+    def app(self):
+        return cherrypy.request.app
+
 
 class PrefsGeneralPanelProvider(Controller):
     """
     Plugin to change user profile and password.
     """
diff --git a/rdiffweb/controller/tests/test_page_admin.py b/rdiffweb/controller/tests/test_page_admin.py
index 02134a46..76d1d8fd 100644
--- a/rdiffweb/controller/tests/test_page_admin.py
+++ b/rdiffweb/controller/tests/test_page_admin.py
@@ -105,32 +105,32 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
 
     login = True
 
     def test_add_user_with_role_admin(self):
         # When trying to create a new user with role admin
-        self._add_user("admin_role", "admin_role@test.com", "test2", "/home/", ADMIN_ROLE)
+        self._add_user("admin_role", "admin_role@test.com", "password", "/home/", ADMIN_ROLE)
         # Then page return success
         self.assertStatus(200)
         # Then database is updated
         userobj = self.app.store.get_user('admin_role')
         self.assertEqual(ADMIN_ROLE, userobj.role)
         # Then notification was raised
         self.listener.user_added.assert_called_once_with(userobj)
 
     def test_add_user_with_role_maintainer(self):
-        self._add_user("maintainer_role", "maintainer_role@test.com", "test2", "/home/", MAINTAINER_ROLE)
+        self._add_user("maintainer_role", "maintainer_role@test.com", "password", "/home/", MAINTAINER_ROLE)
         self.assertStatus(200)
         self.assertEqual(MAINTAINER_ROLE, self.app.store.get_user('maintainer_role').role)
 
     def test_add_user_with_role_user(self):
-        self._add_user("user_role", "user_role@test.com", "test2", "/home/", USER_ROLE)
+        self._add_user("user_role", "user_role@test.com", "password", "/home/", USER_ROLE)
         self.assertStatus(200)
         self.assertEqual(USER_ROLE, self.app.store.get_user('user_role').role)
 
     def test_add_user_with_invalid_role(self):
         # When trying to create a new user with an invalid role (admin instead of 0)
-        self._add_user("invalid", "invalid@test.com", "test2", "/home/", 'admin')
+        self._add_user("invalid", "invalid@test.com", "test1234", "/home/", 'admin')
         # Then an error message is displayed to the user
         self.assertStatus(200)
         self.assertInBody('role: Invalid Choice: could not coerce')
         # Then listener are not called
         self.listener.user_added.assert_not_called()
@@ -143,11 +143,11 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
         # Then listener are not called
         self.listener.user_added.assert_not_called()
 
     def test_add_edit_delete(self):
         #  Add user to be listed
-        self._add_user("test2", "test2@test.com", "test2", "/home/", USER_ROLE)
+        self._add_user("test2", "test2@test.com", "test1234", "/home/", USER_ROLE)
         self.assertInBody("User added successfully.")
         self.assertInBody("test2")
         self.assertInBody("test2@test.com")
         self.listener.user_added.assert_called_once()
         self.listener.user_password_changed.assert_called_once()
@@ -173,11 +173,11 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
 
     def test_add_edit_delete_user_with_encoding(self):
         """
         Check creation of user with non-ascii char.
         """
-        self._add_user("Éric", "éric@test.com", "Éric", "/home/", USER_ROLE)
+        self._add_user("Éric", "éric@test.com", "password", "/home/", USER_ROLE)
         self.assertInBody("User added successfully.")
         self.assertInBody("Éric")
         self.assertInBody("éric@test.com")
         # Update user
         self._edit_user("Éric", "eric.létourno@test.com", "écureuil", "/tmp/", ADMIN_ROLE)
@@ -205,13 +205,13 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
     def test_add_user_with_existing_username(self):
         """
         Verify failure trying to add the same user.
         """
         # Given a user named `test1`
-        self._add_user("test1", "test1@test.com", "test1", "/tmp/", USER_ROLE)
+        self._add_user("test1", "test1@test.com", "password", "/tmp/", USER_ROLE)
         # When trying to create a new user with the same name
-        self._add_user("test1", "test1@test.com", "test1", "/tmp/", USER_ROLE)
+        self._add_user("test1", "test1@test.com", "password", "/tmp/", USER_ROLE)
         # Then the user list is displayed with an error message.
         self.assertStatus(200)
         self.assertInBody("User test1 already exists.")
 
     def test_add_user_with_invalid_root_directory(self):
@@ -220,22 +220,22 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
         """
         try:
             self._delete_user("test5")
         except Exception:
             pass
-        self._add_user("test5", "test1@test.com", "test5", "/var/invalid/", USER_ROLE)
+        self._add_user("test5", "test1@test.com", "password", "/var/invalid/", USER_ROLE)
         self.assertInBody("User added successfully.")
         self.assertInBody("User&#39;s root directory /var/invalid/ is not accessible!")
 
     def test_add_without_email(self):
         #  Add user to be listed
-        self._add_user("test2", None, "test2", "/tmp/", USER_ROLE)
+        self._add_user("test2", None, "password", "/tmp/", USER_ROLE)
         self.assertInBody("User added successfully.")
 
     def test_add_without_user_root(self):
         #  Add user to be listed
-        self._add_user("test6", None, "test6", None, USER_ROLE)
+        self._add_user("test6", None, "password", None, USER_ROLE)
         self.assertInBody("User added successfully.")
 
         user = self.app.store.get_user('test6')
         self.assertEqual('', user.user_root)
 
@@ -265,10 +265,19 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
         # Try deleting admin user
         self._delete_user(self.USERNAME)
         self.assertStatus(200)
         self.assertInBody("can&#39;t delete admin user")
 
+    def test_change_password_with_too_short(self):
+        self._edit_user(self.USERNAME, password='short')
+        self.assertInBody("Password must have between 8 and 128 characters.")
+
+    def test_change_password_with_too_long(self):
+        new_password = 'a' * 129
+        self._edit_user(self.USERNAME, password=new_password)
+        self.assertInBody("Password must have between 8 and 128 characters.")
+
     def test_change_admin_password(self):
         # Given rdiffweb is configured with admin-password option
         self.app.cfg.admin_password = 'hardcoded'
         try:
             # When trying to update admin password
@@ -282,11 +291,11 @@ class AdminUsersAsAdminTest(AbstractAdminTest):
     def test_edit_user_with_invalid_path(self):
         """
         Verify failure trying to update user with invalid path.
         """
         self.app.store.add_user('test1')
-        self._edit_user("test1", "test1@test.com", "test", "/var/invalid/", USER_ROLE)
+        self._edit_user("test1", "test1@test.com", "password", "/var/invalid/", USER_ROLE)
         self.assertNotInBody("User added successfully.")
         self.assertInBody("User&#39;s root directory /var/invalid/ is not accessible!")
 
     def test_list(self):
         self.getPage("/admin/users/")
diff --git a/rdiffweb/controller/tests/test_page_prefs.py b/rdiffweb/controller/tests/test_page_prefs.py
index 59bfcad6..0f098073 100644
--- a/rdiffweb/controller/tests/test_page_prefs.py
+++ b/rdiffweb/controller/tests/test_page_prefs.py
@@ -84,26 +84,35 @@ class PrefsTest(rdiffweb.test.WebCase):
         self._set_profile_info("test@test.com, test2@test.com")
         self.assertInBody("Invalid email")
 
     def test_change_password(self):
         # When udating user's password
-        self._set_password(self.PASSWORD, "newpass", "newpass")
+        self._set_password(self.PASSWORD, "newpassword", "newpassword")
         self.assertInBody("Password updated successfully.")
         # Then a notification is raised
         self.listener.user_password_changed.assert_called_once()
         # Change it back
-        self._set_password("newpass", self.PASSWORD, self.PASSWORD)
+        self._set_password("newpassword", self.PASSWORD, self.PASSWORD)
         self.assertInBody("Password updated successfully.")
 
     def test_change_password_with_wrong_confirmation(self):
         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", "t", "t")
+        self._set_password("oups", "newpassword", "newpassword")
         self.assertInBody("Wrong 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_invalid_pref(self):
         """
         Check if invalid prefs url is 404 Not Found.
         """
         self.getPage("/prefs/invalid/")
diff --git a/rdiffweb/core/config.py b/rdiffweb/core/config.py
index cd5fc086..183d0487 100644
--- a/rdiffweb/core/config.py
+++ b/rdiffweb/core/config.py
@@ -418,10 +418,24 @@ def get_parser():
         action='store_true',
         help='used to hide SSH Key management to avoid users to add or remove SSH Key using the web application',
         default=False,
     )
 
+    parser.add(
+        '--password-min-length',
+        type=int,
+        help="Minimum length of the user's password",
+        default=8,
+    )
+
+    parser.add(
+        '--password-max-length',
+        type=int,
+        help="Maximum length of the user's password",
+        default=128,
+    )
+
     parser.add_argument('--version', action='version', version='%(prog)s ' + VERSION)
 
     # Here we append a list of arguments for each locale.
     flags = ['--welcome-msg'] + ['--welcome-msg-' + i for i in ['ca', 'en', 'es', 'fr', 'ru']] + ['--welcomemsg']
     parser.add_argument(
diff --git a/rdiffweb/core/store.py b/rdiffweb/core/store.py
index 68569af8..6646c0ec 100644
--- a/rdiffweb/core/store.py
+++ b/rdiffweb/core/store.py
@@ -395,10 +395,13 @@ class UserObject(object):
         """
         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 = self._store.app.cfg
+        if cfg.password_min_length > len(password) > cfg.password_max_length:
+            raise ValueError("invalid password length")
 
         # Cannot update admin-password if defined
         if self.username == self._store._admin_user and self._store._admin_password:
             raise ValueError(_("can't update admin-password defined in configuration file"))
 

Vulnerable code:     password = PasswordField(_('Password'))
from wtforms.validators import DataRequired, EqualTo, InputRequired, Regexp
        self._add_user("admin_role", "admin_role@test.com", "test2", "/home/", ADMIN_ROLE)
        self._add_user("maintainer_role", "maintainer_role@test.com", "test2", "/home/", MAINTAINER_ROLE)
        self._add_user("user_role", "user_role@test.com", "test2", "/home/", USER_ROLE)
        self._add_user("invalid", "invalid@test.com", "test2", "/home/", 'admin')
        self._add_user("test2", "test2@test.com", "test2", "/home/", USER_ROLE)
        self._add_user("Éric", "éric@test.com", "Éric", "/home/", USER_ROLE)
        self._add_user("test1", "test1@test.com", "test1", "/tmp/", USER_ROLE)
        self._add_user("test1", "test1@test.com", "test1", "/tmp/", USER_ROLE)
        self._add_user("test5", "test1@test.com", "test5", "/var/invalid/", USER_ROLE)
        self._add_user("test2", None, "test2", "/tmp/", USER_ROLE)
        self._add_user("test6", None, "test6", None, USER_ROLE)
        self._edit_user("test1", "test1@test.com", "test", "/var/invalid/", USER_ROLE)
        self._set_password(self.PASSWORD, "newpass", "newpass")
        self._set_password("newpass", self.PASSWORD, self.PASSWORD)
        self._set_password("oups", "t", "t")
Fixed code: * Enforce minimum and maximum password length #211
## Custom user's password length limits

By default, Rdiffweb supports passwords with the following lengths:

* Minimum: 8 characters
* Maximum: 128 characters

Changing the minimum or maximum length does not affect existing users' passwords. Existing users are not prompted to reset their passwords to meet the new limits. The new limit only applies when an existing user changes their password.

| Option | Description | Example |
| --- | --- | --- |
| password-min-length | Minimum length of the user's password | 8 |
| password-max-length | Maximum length of the user's password | 128 |

    password = PasswordField(_('Password'), validators=[validators.optional()])
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.password.validators += [
            validators.length(
                min=self.app.cfg.password_min_length,
                max=self.app.cfg.password_max_length,
                message=_('Password must have between %(min)d and %(max)d characters.'),
            )
        ]

    @property
    def app(self):
        return cherrypy.request.app

from wtforms.validators import DataRequired, EqualTo, InputRequired, Length, Regexp
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.new.validators += [
            Length(
                min=self.app.cfg.password_min_length,
                max=self.app.cfg.password_max_length,
                message=_('Password must have between %(min)d and %(max)d characters.'),
            )
        ]

    @property
    def app(self):
        return cherrypy.request.app

        self._add_user("admin_role", "admin_role@test.com", "password", "/home/", ADMIN_ROLE)
        self._add_user("maintainer_role", "maintainer_role@test.com", "password", "/home/", MAINTAINER_ROLE)
        self._add_user("user_role", "user_role@test.com", "password", "/home/", USER_ROLE)
        self._add_user("invalid", "invalid@test.com", "test1234", "/home/", 'admin')
        self._add_user("test2", "test2@test.com", "test1234", "/home/", USER_ROLE)
        self._add_user("Éric", "éric@test.com", "password", "/home/", USER_ROLE)
        self._add_user("test1", "test1@test.com", "password", "/tmp/", USER_ROLE)
        self._add_user("test1", "test1@test.com", "password", "/tmp/", USER_ROLE)
        self._add_user("test5", "test1@test.com", "password", "/var/invalid/", USER_ROLE)
        self._add_user("test2", None, "password", "/tmp/", USER_ROLE)
        self._add_user("test6", None, "password", None, USER_ROLE)
    def test_change_password_with_too_short(self):
        self._edit_user(self.USERNAME, password='short')
        self.assertInBody("Password must have between 8 and 128 characters.")

    def test_change_password_with_too_long(self):
        new_password = 'a' * 129
        self._edit_user(self.USERNAME, password=new_password)
        self.assertInBody("Password must have between 8 and 128 characters.")

        self._edit_user("test1", "test1@test.com", "password", "/var/invalid/", USER_ROLE)
        self._set_password(self.PASSWORD, "newpassword", "newpassword")
        self._set_password("newpassword", self.PASSWORD, self.PASSWORD)
        self._set_password("oups", "newpassword", "newpassword")
    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.")

    parser.add(
        '--password-min-length',
        type=int,
        help="Minimum length of the user's password",
        default=8,
    )

    parser.add(
        '--password-max-length',
        type=int,
        help="Maximum length of the user's password",
        default=128,
    )

        cfg = self._store.app.cfg
        if cfg.password_min_length > len(password) > cfg.password_max_length:
            raise ValueError("invalid password length")