diff options
author | Dave Wilde (d34dh0r53) <dwilde@redhat.com> | 2022-02-09 11:28:59 -0600 |
---|---|---|
committer | Dave Wilde <dwilde@redhat.com> | 2023-02-22 14:43:35 -0600 |
commit | 3288af579de8ee312c36fb78ac9309ce8c554827 (patch) | |
tree | 258abd088bd32979b7b3dc52dbef195dbbf28551 | |
parent | 420f4ff46da106b67912cecdff939f5dc0b079d0 (diff) | |
download | keystone-3288af579de8ee312c36fb78ac9309ce8c554827.tar.gz |
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.
[0]: https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues
Closes-bug: #1901891
Change-Id: I8d0bb2438b23227b5a66b94af6f8e198084fcd8d
-rw-r--r-- | keystone/common/password_hashing.py | 22 | ||||
-rw-r--r-- | keystone/conf/identity.py | 6 | ||||
-rw-r--r-- | keystone/tests/unit/common/test_utils.py | 11 | ||||
-rw-r--r-- | releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml | 9 | ||||
-rw-r--r-- | tox.ini | 3 |
5 files changed, 48 insertions, 3 deletions
diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index 4e62d9c38..b38d3cba7 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -57,8 +57,26 @@ 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 = 54 + 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: if CONF.strict_password_check: diff --git a/keystone/conf/identity.py b/keystone/conf/identity.py index 0dffe58d6..5cce78cf9 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 54. """)) list_limit = cfg.IntOpt( diff --git a/keystone/tests/unit/common/test_utils.py b/keystone/tests/unit/common/test_utils.py index 39962b4f6..8b8280dc9 100644 --- a/keystone/tests/unit/common/test_utils.py +++ b/keystone/tests/unit/common/test_utils.py @@ -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='64') + invalid_length_password = '0' * 64 + 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/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml b/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml new file mode 100644 index 000000000..003dc47df --- /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. @@ -109,6 +109,9 @@ enable-extensions = H203,H904 ignore = D100,D101,D102,D103,D104,D106,D107,D203,D401,E305,E402,H211,H214,W503,W504,W605 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 = |