From 6730c761d18aa547998f2add833c13f45f257fe7 Mon Sep 17 00:00:00 2001 From: Dmitriy Rabotyagov Date: Wed, 9 Aug 2023 20:41:05 +0200 Subject: [PATCH] Properly trimm bcrypt hashed passwords bcrypt hashing algorythm has a limitation on length of passwords it can hash on 72 bytes. In [1] a password trimm to 54 symbols has been implemented, which resulted in password being invalidated after the keystone upgrade, since passwords are trimmed differently by bcrypt itself, as well as len(str()) is not always equal to len(str().encode()) as trimming should be done based on bytes and not string itself. With the change we return a byte object from `verify_length_and_trunc_password`, so it does not need to be encoded afterwards, since we need to strip based on bytes rather then on length of the string. [1] https://review.opendev.org/c/openstack/keystone/+/828595 Closes-Bug: #2028809 Related-Bug: #1901891 Change-Id: Iea95a3c2df041a0046647b3d3dadead1a6d054d1 --- keystone/common/password_hashing.py | 15 ++++++++------- keystone/conf/identity.py | 2 +- keystone/tests/unit/common/test_utils.py | 8 ++++---- .../bcrypt_truncation_fix-674dc5d7f1e776f2.yaml | 7 +++++++ 4 files changed, 20 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index b38d3cba79..7c3c617e4e 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -68,7 +68,7 @@ def verify_length_and_trunc_password(password): # When using bcrypt, we limit the password length to 54 to ensure all # bytes are fully mixed. See: # https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues - BCRYPT_MAX_LENGTH = 54 + BCRYPT_MAX_LENGTH = 72 if (CONF.identity.password_hash_algorithm == 'bcrypt' and # nosec: B105 CONF.identity.max_password_length > BCRYPT_MAX_LENGTH): msg = "Truncating password to algorithm specific maximum length %d characters." @@ -78,16 +78,17 @@ def verify_length_and_trunc_password(password): max_length = CONF.identity.max_password_length try: - if len(password) > max_length: + password_utf8 = password.encode('utf-8') + if len(password_utf8) > max_length: if CONF.strict_password_check: raise exception.PasswordVerificationError(size=max_length) else: msg = "Truncating user password to %d characters." LOG.warning(msg, max_length) - return password[:max_length] + return password_utf8[:max_length] else: - return password - except TypeError: + return password_utf8 + except AttributeError: raise exception.ValidationError(attribute='string', target='password') @@ -100,7 +101,7 @@ def check_password(password, hashed): """ if password is None or hashed is None: return False - password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') + password_utf8 = verify_length_and_trunc_password(password) hasher = _get_hasher_from_ident(hashed) return hasher.verify(password_utf8, hashed) @@ -117,7 +118,7 @@ def hash_user_password(user): def hash_password(password): """Hash a password. Harder.""" params = {} - password_utf8 = verify_length_and_trunc_password(password).encode('utf-8') + password_utf8 = verify_length_and_trunc_password(password) conf_hasher = CONF.identity.password_hash_algorithm hasher = _HASHER_NAME_MAP.get(conf_hasher) diff --git a/keystone/conf/identity.py b/keystone/conf/identity.py index 5cce78cf91..d2cb34da17 100644 --- a/keystone/conf/identity.py +++ b/keystone/conf/identity.py @@ -103,7 +103,7 @@ performance. Changing this value does not effect existing passwords. This value can also be overridden by certain hashing algorithms maximum allowed length which takes precedence over the configured value. -The bcrypt max_password_length is 54. +The bcrypt max_password_length is 72 bytes. """)) list_limit = cfg.IntOpt( diff --git a/keystone/tests/unit/common/test_utils.py b/keystone/tests/unit/common/test_utils.py index 673175aea8..15ebc273ed 100644 --- a/keystone/tests/unit/common/test_utils.py +++ b/keystone/tests/unit/common/test_utils.py @@ -77,7 +77,7 @@ class UtilsTestCase(unit.BaseTestCase): self.config_fixture.config(strict_password_check=False) password = uuid.uuid4().hex verified = common_utils.verify_length_and_trunc_password(password) - self.assertEqual(password, verified) + self.assertEqual(password.encode('utf-8'), verified) def test_that_a_hash_can_not_be_validated_against_a_hash(self): # NOTE(dstanek): Bug 1279849 reported a problem where passwords @@ -97,7 +97,7 @@ class UtilsTestCase(unit.BaseTestCase): max_length = CONF.identity.max_password_length invalid_password = 'passw0rd' trunc = common_utils.verify_length_and_trunc_password(invalid_password) - self.assertEqual(invalid_password[:max_length], trunc) + self.assertEqual(invalid_password.encode('utf-8')[:max_length], trunc) def test_verify_long_password_strict_raises_exception(self): self.config_fixture.config(strict_password_check=True) @@ -139,8 +139,8 @@ class UtilsTestCase(unit.BaseTestCase): self.config_fixture.config(group='identity', password_hash_algorithm='bcrypt') self.config_fixture.config(group='identity', - max_password_length='64') - invalid_length_password = '0' * 64 + max_password_length='96') + invalid_length_password = '0' * 96 self.assertRaises(exception.PasswordVerificationError, common_utils.hash_password, invalid_length_password) diff --git a/releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml b/releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml new file mode 100644 index 0000000000..479a7aaafb --- /dev/null +++ b/releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Passwords that are hashed using bcrypt are now truncated properly to the + maximum allowed length by the algorythm. This solves regression, when + passwords longer then 54 symbols are getting invalidated after the + Keystone upgrade.