From 7852ca24a4eb86cb271ef7ec5e07f8f9c97f926d Mon Sep 17 00:00:00 2001 From: "Dave Wilde (d34dh0r53)" Date: Wed, 9 Feb 2022 11:28:59 -0600 Subject: [PATCH] Force algo specific maximum length & Properly trimm bcrypt hashed passwords This is the squash of 2 patches related to bcrypt hashing settings. 1. Force algo specific maximum length The bcrypt algorithm that we use for password hashing silently length limits the size of the password that is hashed giving the user a false sense of security [0]. This patch adds a check in the verify_length_and_trunc_password function for the hash in use and updates the max_length accordingly, this will override the configured value and log a warning if the password is truncated. Conflicts: * tox.ini [0]: https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues 2. 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 original change id: Iea95a3c2df041a0046647b3d3dadead1a6d054d1 (cherry picked from commit 6730c761d18aa547998f2add833c13f45f257fe7) (cherry picked from commit 65f1fb6b4a54386f473369b05c8d10d77fb6710c) Closes-bug: #1901891 Change-Id: I8d0bb2438b23227b5a66b94af6f8e198084fcd8d (cherry picked from commit 3288af579de8ee312c36fb78ac9309ce8c554827) (cherry picked from commit 1b3536a7a4d72e7f7b95cc1874a450accad3ec8d) --- keystone/common/password_hashing.py | 35 ++++++++++++++----- keystone/conf/identity.py | 6 +++- keystone/tests/unit/common/test_utils.py | 15 ++++++-- ...crypt_truncation_fix-674dc5d7f1e776f2.yaml | 7 ++++ ...uncation-and-warning-bd69090315ec18a7.yaml | 9 +++++ tox.ini | 3 ++ 6 files changed, 64 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/bcrypt_truncation_fix-674dc5d7f1e776f2.yaml create mode 100644 releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index 4e62d9c386..7c3c617e4e 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -57,19 +57,38 @@ def _get_hasher_from_ident(hashed): def verify_length_and_trunc_password(password): - """Verify and truncate the provided password to the max_password_length.""" - max_length = CONF.identity.max_password_length + """Verify and truncate the provided password to the max_password_length. + + We also need to check that the configured password hashing algorithm does + not silently truncate the password. For example, passlib.hash.bcrypt does + this: + https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues + + """ + # 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 = 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." + LOG.warning(msg, BCRYPT_MAX_LENGTH) + max_length = BCRYPT_MAX_LENGTH + else: + 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') @@ -82,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) @@ -99,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 0dffe58d60..d2cb34da17 100644 --- a/keystone/conf/identity.py +++ b/keystone/conf/identity.py @@ -99,7 +99,11 @@ max_password_length = cfg.IntOpt( max=passlib.utils.MAX_PASSWORD_SIZE, help=utils.fmt(""" Maximum allowed length for user passwords. Decrease this value to improve -performance. Changing this value does not effect existing passwords. +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 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 39962b4f6f..c91f0c2cb1 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) @@ -134,6 +134,17 @@ class UtilsTestCase(unit.BaseTestCase): common_utils.hash_password, invalid_length_password) + def test_max_algo_length_truncates_password(self): + self.config_fixture.config(strict_password_check=True) + self.config_fixture.config(group='identity', + password_hash_algorithm='bcrypt') + self.config_fixture.config(group='identity', + max_password_length='96') + invalid_length_password = '0' * 96 + self.assertRaises(exception.PasswordVerificationError, + common_utils.hash_password, + invalid_length_password) + def _create_test_user(self, password=OPTIONAL): user = {"name": "hthtest"} if password is not self.OPTIONAL: 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. diff --git a/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml b/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml new file mode 100644 index 0000000000..003dc47df5 --- /dev/null +++ b/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml @@ -0,0 +1,9 @@ +--- +security: + - | + Passwords will now be automatically truncated if the max_password_length is + greater than the allowed length for the selected password hashing + algorithm. Currently only bcrypt has fixed allowed lengths defined which is + 54 characters. A warning will be generated in the log if a password is + truncated. This will not affect existing passwords, however only the first + 54 characters of existing bcrypt passwords will be validated. diff --git a/tox.ini b/tox.ini index 19478c90fa..9978f3cc4f 100644 --- a/tox.ini +++ b/tox.ini @@ -119,6 +119,9 @@ enable-extensions = H203,H904 ignore = D100,D101,D102,D103,D104,D203,E402,W503,W504 exclude=.venv,.git,.tox,build,dist,*lib/python*,*egg,tools,vendor,.update-venv,*.ini,*.po,*.pot max-complexity=24 +per-file-ignores = +# URL lines too long + keystone/common/password_hashing.py: E501 [testenv:docs] deps =