Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Add password score validation
  • Loading branch information
ikus060 committed Sep 27, 2022
1 parent 2406780 commit ee98e5a
Show file tree
Hide file tree
Showing 10 changed files with 56 additions and 51 deletions.
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions debian/control
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions doc/configuration.md
Expand Up @@ -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

Expand Down
12 changes: 0 additions & 12 deletions rdiffweb/controller/page_admin.py
Expand Up @@ -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
Expand Down
12 changes: 0 additions & 12 deletions rdiffweb/controller/pref_general.py
Expand Up @@ -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):
"""
Expand Down
38 changes: 19 additions & 19 deletions rdiffweb/controller/tests/test_page_admin.py
Expand Up @@ -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
Expand All @@ -117,26 +117,26 @@ 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')
# Then listener are not called
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')
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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.")

Expand All @@ -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.")
Expand All @@ -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')
Expand All @@ -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.")
Expand All @@ -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.")
Expand All @@ -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.")
Expand All @@ -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)
Expand Down Expand Up @@ -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!")

Expand Down
7 changes: 2 additions & 5 deletions rdiffweb/controller/tests/test_page_prefs.py
Expand Up @@ -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):
Expand Down
7 changes: 7 additions & 0 deletions rdiffweb/core/config.py
Expand Up @@ -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.
Expand Down
25 changes: 22 additions & 3 deletions rdiffweb/core/store.py
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions setup.cfg
Expand Up @@ -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
Expand Down

0 comments on commit ee98e5a

Please sign in to comment.