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
This commit is contained in:
parent
420f4ff46d
commit
3288af579d
@ -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:
|
||||
|
@ -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(
|
||||
|
@ -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:
|
||||
|
@ -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.
|
3
tox.ini
3
tox.ini
@ -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 =
|
||||
|
Loading…
x
Reference in New Issue
Block a user