From ee98e5af78ec60db8a17fef6ea0ca250e3f31eec Mon Sep 17 00:00:00 2001 From: Patrik Dufresne Date: Tue, 27 Sep 2022 12:32:40 -0400 Subject: [PATCH] Add password score validation --- README.md | 1 + debian/control | 1 + doc/configuration.md | 3 ++ rdiffweb/controller/page_admin.py | 12 ------- rdiffweb/controller/pref_general.py | 12 ------- rdiffweb/controller/tests/test_page_admin.py | 38 ++++++++++---------- rdiffweb/controller/tests/test_page_prefs.py | 7 ++-- rdiffweb/core/config.py | 7 ++++ rdiffweb/core/store.py | 25 +++++++++++-- setup.cfg | 1 + 10 files changed, 56 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 4b514dd2..e366a02f 100644 --- a/README.md +++ b/README.md @@ -112,6 +112,7 @@ 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. * Add `Cache-Control` and other security headers [CVE-2022-3292](https://nvd.nist.gov/vuln/detail/CVE-2022-3292) +* Enforce password policy using `password-score` based on [zxcvbn](https://github.com/dropbox/zxcvbn) [CVE-2022-3326](https://nvd.nist.gov/vuln/detail/CVE-2022-3326) ## 2.4.8 (2022-09-26) diff --git a/debian/control b/debian/control index 5e0299dc..b5342bac 100644 --- a/debian/control +++ b/debian/control @@ -28,6 +28,7 @@ Build-Depends: python3-setuptools-scm, python3-sqlalchemy, python3-wtforms, + python3-zxcvbn, rdiff-backup, Rules-Requires-Root: no Standards-Version: 4.5.1 diff --git a/doc/configuration.md b/doc/configuration.md index 6e4556da..df12a3ef 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -291,6 +291,9 @@ Changing the minimum or maximum length does not affect existing users' passwords | --- | --- | --- | | password-min-length | Minimum length of the user's password | 8 | | password-max-length | Maximum length of the user's password | 128 | +| password-score | Minimum zxcvbn's score for password. Value from 0 to 4. Default value 1. | 4 | + +You may want to read more about [zxcvbn](https://github.com/dropbox/zxcvbn) score value. ## Configure Rdiffweb appearance diff --git a/rdiffweb/controller/page_admin.py b/rdiffweb/controller/page_admin.py index 4cc0a9b9..51578658 100644 --- a/rdiffweb/controller/page_admin.py +++ b/rdiffweb/controller/page_admin.py @@ -209,18 +209,6 @@ class UserForm(CherryForm): _('Quota Used'), validators=[validators.optional()], description=_("Disk spaces (in bytes) used by this user.") ) - def validate_password(self, field): - validator = 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.'), - ) - validator(self, field) - - @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 diff --git a/rdiffweb/controller/pref_general.py b/rdiffweb/controller/pref_general.py index d25c4ebd..e0b0ca03 100644 --- a/rdiffweb/controller/pref_general.py +++ b/rdiffweb/controller/pref_general.py @@ -61,18 +61,6 @@ class UserPasswordForm(CherryForm): _('Confirm new password'), validators=[InputRequired(_("Confirmation password is missing."))] ) - def validate_new(self, field): - validator = 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.'), - ) - validator(self, field) - - @property - def app(self): - return cherrypy.request.app - class PrefsGeneralPanelProvider(Controller): """ diff --git a/rdiffweb/controller/tests/test_page_admin.py b/rdiffweb/controller/tests/test_page_admin.py index 77cd0535..89b81a9a 100644 --- a/rdiffweb/controller/tests/test_page_admin.py +++ b/rdiffweb/controller/tests/test_page_admin.py @@ -107,7 +107,7 @@ class AdminUsersAsAdminTest(AbstractAdminTest): 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", "password", "/home/", ADMIN_ROLE) + self._add_user("admin_role", "admin_role@test.com", "pr3j5Dwi", "/home/", ADMIN_ROLE) # Then page return success self.assertStatus(200) # Then database is updated @@ -117,18 +117,18 @@ def test_add_user_with_role_admin(self): 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", "password", "/home/", MAINTAINER_ROLE) + self._add_user("maintainer_role", "maintainer_role@test.com", "pr3j5Dwi", "/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", "password", "/home/", USER_ROLE) + self._add_user("user_role", "user_role@test.com", "pr3j5Dwi", "/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", "test1234", "/home/", 'admin') + self._add_user("invalid", "invalid@test.com", "pr3j5Dwi", "/home/", 'admin') # Then an error message is displayed to the user self.assertStatus(200) self.assertInBody('role: Invalid Choice: could not coerce') @@ -136,7 +136,7 @@ def test_add_user_with_invalid_role(self): self.listener.user_added.assert_not_called() # When trying to create a new user with an invalid role (-1) - self._add_user("invalid", "invalid@test.com", "test2", "/home/", -1) + self._add_user("invalid", "invalid@test.com", "pr3j5Dwi", "/home/", -1) # Then an error message is displayed to the user self.assertStatus(200) self.assertInBody('role: Not a valid choice') @@ -145,7 +145,7 @@ def test_add_user_with_invalid_role(self): def test_add_edit_delete(self): # Add user to be listed - self._add_user("test2", "test2@test.com", "test1234", "/home/", USER_ROLE) + self._add_user("test2", "test2@test.com", "pr3j5Dwi", "/home/", USER_ROLE) self.assertInBody("User added successfully.") self.assertInBody("test2") self.assertInBody("test2@test.com") @@ -175,7 +175,7 @@ def test_add_edit_delete_user_with_encoding(self): """ Check creation of user with non-ascii char. """ - self._add_user("Éric", "éric@test.com", "password", "/home/", USER_ROLE) + self._add_user("Éric", "éric@test.com", "pr3j5Dwi", "/home/", USER_ROLE) self.assertInBody("User added successfully.") self.assertInBody("Éric") self.assertInBody("éric@test.com") @@ -198,7 +198,7 @@ def test_add_user_with_empty_username(self): """ Verify failure trying to create user without username. """ - self._add_user("", "test1@test.com", "test1", "/tmp/", USER_ROLE) + self._add_user("", "test1@test.com", "pr3j5Dwi", "/tmp/", USER_ROLE) self.assertStatus(200) self.assertInBody("username: This field is required.") @@ -207,9 +207,9 @@ 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", "password", "/tmp/", USER_ROLE) + self._add_user("test1", "test1@test.com", "pr3j5Dwi", "/tmp/", USER_ROLE) # When trying to create a new user with the same name - self._add_user("test1", "test1@test.com", "password", "/tmp/", USER_ROLE) + self._add_user("test1", "test1@test.com", "pr3j5Dwi", "/tmp/", USER_ROLE) # Then the user list is displayed with an error message. self.assertStatus(200) self.assertInBody("User test1 already exists.") @@ -222,18 +222,18 @@ def test_add_user_with_invalid_root_directory(self): self._delete_user("test5") except Exception: pass - self._add_user("test5", "test1@test.com", "password", "/var/invalid/", USER_ROLE) + self._add_user("test5", "test1@test.com", "pr3j5Dwi", "/var/invalid/", USER_ROLE) self.assertInBody("User added successfully.") self.assertInBody("User's root directory /var/invalid/ is not accessible!") def test_add_without_email(self): # Add user to be listed - self._add_user("test2", None, "password", "/tmp/", USER_ROLE) + self._add_user("test2", None, "pr3j5Dwi", "/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, "password", None, USER_ROLE) + self._add_user("test6", None, "pr3j5Dwi", None, USER_ROLE) self.assertInBody("User added successfully.") user = self.app.store.get_user('test6') @@ -243,7 +243,7 @@ def test_add_with_username_too_long(self): # Given a too long username username = "test2" * 52 # When trying to create the user - self._add_user(username, None, "password", "/tmp/", USER_ROLE) + self._add_user(username, None, "pr3j5Dwi", "/tmp/", USER_ROLE) # Then an error is raised self.assertStatus(200) self.assertInBody("Username too long.") @@ -252,7 +252,7 @@ def test_add_with_email_too_long(self): # Given a too long username email = ("test2" * 50) + "@test.com" # When trying to create the user - self._add_user("test2", email, "password", "/tmp/", USER_ROLE) + self._add_user("test2", email, "pr3j5Dwi", "/tmp/", USER_ROLE) # Then an error is raised self.assertStatus(200) self.assertInBody("Email too long.") @@ -261,7 +261,7 @@ def test_add_with_user_root_too_long(self): # Given a too long user root user_root = "/temp/" * 50 # When trying to create the user - self._add_user("test2", "test@test,com", "password", user_root, USER_ROLE) + self._add_user("test2", "test@test,com", "pr3j5Dwi", user_root, USER_ROLE) # Then an error is raised self.assertStatus(200) self.assertInBody("Root directory too long.") @@ -285,9 +285,9 @@ def test_delete_user_admin(self): Verify failure to delete our self. """ # Create another admin user - self._add_user('admin2', '', 'password', '', ADMIN_ROLE) + self._add_user('admin2', '', 'pr3j5Dwi', '', ADMIN_ROLE) self.getPage("/logout/") - self._login('admin2', 'password') + self._login('admin2', 'pr3j5Dwi') # Try deleting admin user self._delete_user(self.USERNAME) @@ -330,7 +330,7 @@ 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", "password", "/var/invalid/", USER_ROLE) + self._edit_user("test1", "test1@test.com", "pr3j5Dwi", "/var/invalid/", USER_ROLE) self.assertNotInBody("User added successfully.") self.assertInBody("User's root directory /var/invalid/ is not accessible!") diff --git a/rdiffweb/controller/tests/test_page_prefs.py b/rdiffweb/controller/tests/test_page_prefs.py index 674fd16b..86278ac3 100644 --- a/rdiffweb/controller/tests/test_page_prefs.py +++ b/rdiffweb/controller/tests/test_page_prefs.py @@ -90,20 +90,17 @@ def test_change_email_with_too_long(self): def test_change_password(self): # When udating user's password - self._set_password(self.PASSWORD, "newpassword", "newpassword") + self._set_password(self.PASSWORD, "pr3j5Dwi", "pr3j5Dwi") self.assertInBody("Password updated successfully.") # Then a notification is raised self.listener.user_password_changed.assert_called_once() - # Change it back - 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", "newpassword", "newpassword") + self._set_password("oups", "pr3j5Dwi", "pr3j5Dwi") self.assertInBody("Wrong password") def test_change_password_with_too_short(self): diff --git a/rdiffweb/core/config.py b/rdiffweb/core/config.py index 183d0487..ccf3bdf5 100644 --- a/rdiffweb/core/config.py +++ b/rdiffweb/core/config.py @@ -434,6 +434,13 @@ def get_parser(): default=128, ) + parser.add( + '--password-score', + type=lambda x: max(1, min(int(x), 4)), + help="Minimum zxcvbn's score for password. Value from 1 to 4. Default value 2. Read more about it here: https://github.com/dropbox/zxcvbn", + default=2, + ) + parser.add_argument('--version', action='version', version='%(prog)s ' + VERSION) # Here we append a list of arguments for each locale. diff --git a/rdiffweb/core/store.py b/rdiffweb/core/store.py index 6646c0ec..c384a40c 100644 --- a/rdiffweb/core/store.py +++ b/rdiffweb/core/store.py @@ -28,6 +28,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.sql.expression import and_, or_, select from sqlalchemy.sql.functions import count +from zxcvbn import zxcvbn from rdiffweb.core import RdiffError, authorizedkeys from rdiffweb.core.config import Option @@ -397,17 +398,35 @@ def set_password(self, password, old_password=None): 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")) + # Check current password if old_password and not check_password(old_password, self.hash_password): raise ValueError(_("Wrong password")) + # Check password length + cfg = self._store.app.cfg + 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} + ) + + # Verify password score using zxcvbn + stats = zxcvbn(password) + if stats.get('score') < cfg.password_score: + msg = _('Password too weak.') + warning = stats.get('feedback', {}).get('warning') + suggestions = stats.get('feedback', {}).get('suggestions') + if warning: + msg += ' ' + warning + if suggestions: + msg += ' ' + ' '.join(suggestions) + raise ValueError(msg) + logger.info("updating user password [%s]", self.username) self.hash_password = hash_password(password) self._store.bus.publish('user_password_changed', self) diff --git a/setup.cfg b/setup.cfg index 5bdb70d9..f0643f03 100644 --- a/setup.cfg +++ b/setup.cfg @@ -37,6 +37,7 @@ install_requires = psutil>=2.1.1 sqlalchemy WTForms<3.0.0 + zxcvbn>=4.4.27 setup_requires = babel>=0.9.6 setuptools_scm