From 7859ed26003858ebfd9a5e866b43f1a6a9e83dca Mon Sep 17 00:00:00 2001
From: "Dave Wilde (d34dh0r53)" <dwilde@redhat.com>
Date: Wed, 9 Feb 2022 11:28:59 -0600
Subject: [PATCH] Force algo specific maximum length

Two of the algorithms that we use for password hashing silently
length limit the size of the password that is hashed giving the
user a false sense of security [0], [1].  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.

Closes-bug: #1901891
Change-Id: I8d0bb2438b23227b5a66b94af6f8e198084fcd8d
---
 keystone/common/password_hashing.py           | 33 +++++++++++++++++--
 keystone/conf/identity.py                     |  7 +++-
 ...uncation-and-warning-bd69090315ec18a7.yaml | 10 ++++++
 3 files changed, 47 insertions(+), 3 deletions(-)
 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..9c48f4ec4b 100644
--- a/keystone/common/password_hashing.py
+++ b/keystone/common/password_hashing.py
@@ -57,8 +57,37 @@ 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
+
+    """
+    conf_max_length = CONF.identity.max_password_length
+    conf_hasher = CONF.identity.password_hash_algorithm
+    if conf_hasher == 'bcrypt':
+        # Setting this to 54 to ensure that all bytes are fully mixed per the
+        # passlib.hash.bcrypt security issue.
+        #  https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#security-issues
+        algorithm_max_length = 54
+    elif conf_hasher == 'sha512_crypt':
+        # Per the security issues for sha512_crypt (the same as sha256_crypt)
+        # the maximum password size is set to 4k by default.
+        # https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html
+        algorithm_max_length = 4096
+    else:
+        algorithm_max_length = None
+
+    if algorithm_max_length is not None:
+        if algorithm_max_length < conf_max_length:
+            max_length = algorithm_max_length
+        else:
+            max_length = conf_max_length
+    else:
+        max_length = conf_max_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 0dffe58d60..2955ca0a0d 100644
--- a/keystone/conf/identity.py
+++ b/keystone/conf/identity.py
@@ -99,7 +99,12 @@ 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.
+
+The bcrypt max_password_length is 54.
+
+The sha512_crypt max_password_length is 4096.
 """))
 
 list_limit = cfg.IntOpt(
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..a2cee918c5
--- /dev/null
+++ b/releasenotes/notes/max-password-length-truncation-and-warning-bd69090315ec18a7.yaml
@@ -0,0 +1,10 @@
+---
+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 and sha512_crypt have fixed allowed lengths defined,
+    54 for bcrypt and 4096 for sha512_crypt.  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 and the
+    first 4096 characters of existing sha512_crypt passwords will be validated.