Blame SOURCES/0024-Issue-4817-BUG-locked-crypt-accounts-on-import-may-a.patch

6d0b66
From 2f0218f91d35c83a2aaecb71849a54b2481390ab Mon Sep 17 00:00:00 2001
6d0b66
From: Firstyear <william@blackhats.net.au>
6d0b66
Date: Fri, 9 Jul 2021 11:53:35 +1000
6d0b66
Subject: [PATCH] Issue 4817 - BUG - locked crypt accounts on import may allow
6d0b66
 all passwords (#4819)
6d0b66
6d0b66
Bug Description: Due to mishanding of short dbpwd hashes, the
6d0b66
crypt_r algorithm was misused and was only comparing salts
6d0b66
in some cases, rather than checking the actual content
6d0b66
of the password.
6d0b66
6d0b66
Fix Description: Stricter checks on dbpwd lengths to ensure
6d0b66
that content passed to crypt_r has at least 2 salt bytes and
6d0b66
1 hash byte, as well as stricter checks on ct_memcmp to ensure
6d0b66
that compared values are the same length, rather than potentially
6d0b66
allowing overruns/short comparisons.
6d0b66
6d0b66
fixes: https://github.com/389ds/389-ds-base/issues/4817
6d0b66
6d0b66
Author: William Brown <william@blackhats.net.au>
6d0b66
6d0b66
Review by: @mreynolds389
6d0b66
---
6d0b66
 .../password/pwd_crypt_asterisk_test.py       | 50 +++++++++++++++++++
6d0b66
 ldap/servers/plugins/pwdstorage/crypt_pwd.c   | 20 +++++---
6d0b66
 2 files changed, 64 insertions(+), 6 deletions(-)
6d0b66
 create mode 100644 dirsrvtests/tests/suites/password/pwd_crypt_asterisk_test.py
6d0b66
6d0b66
diff --git a/dirsrvtests/tests/suites/password/pwd_crypt_asterisk_test.py b/dirsrvtests/tests/suites/password/pwd_crypt_asterisk_test.py
6d0b66
new file mode 100644
6d0b66
index 000000000..d76614db1
6d0b66
--- /dev/null
6d0b66
+++ b/dirsrvtests/tests/suites/password/pwd_crypt_asterisk_test.py
6d0b66
@@ -0,0 +1,50 @@
6d0b66
+# --- BEGIN COPYRIGHT BLOCK ---
6d0b66
+# Copyright (C) 2021 William Brown <william@blackhats.net.au>
6d0b66
+# All rights reserved.
6d0b66
+#
6d0b66
+# License: GPL (version 3 or any later version).
6d0b66
+# See LICENSE for details.
6d0b66
+# --- END COPYRIGHT BLOCK ---
6d0b66
+#
6d0b66
+import ldap
6d0b66
+import pytest
6d0b66
+from lib389.topologies import topology_st
6d0b66
+from lib389.idm.user import UserAccounts
6d0b66
+from lib389._constants import (DEFAULT_SUFFIX, PASSWORD)
6d0b66
+
6d0b66
+pytestmark = pytest.mark.tier1
6d0b66
+
6d0b66
+def test_password_crypt_asterisk_is_rejected(topology_st):
6d0b66
+    """It was reported that {CRYPT}* was allowing all passwords to be
6d0b66
+    valid in the bind process. This checks that we should be rejecting
6d0b66
+    these as they should represent locked accounts. Similar, {CRYPT}!
6d0b66
+
6d0b66
+    :id: 0b8f1a6a-f3eb-4443-985e-da14d0939dc3
6d0b66
+    :setup: Single instance
6d0b66
+    :steps: 1. Set a password hash in with CRYPT and the content *
6d0b66
+            2. Test a bind
6d0b66
+            3. Set a password hash in with CRYPT and the content !
6d0b66
+            4. Test a bind
6d0b66
+    :expectedresults:
6d0b66
+            1. Successfully set the values
6d0b66
+            2. The bind fails
6d0b66
+            3. Successfully set the values
6d0b66
+            4. The bind fails
6d0b66
+    """
6d0b66
+    topology_st.standalone.config.set('nsslapd-allow-hashed-passwords', 'on')
6d0b66
+    topology_st.standalone.config.set('nsslapd-enable-upgrade-hash', 'off')
6d0b66
+
6d0b66
+    users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)
6d0b66
+    user = users.create_test_user()
6d0b66
+
6d0b66
+    user.set('userPassword', "{CRYPT}*")
6d0b66
+
6d0b66
+    # Attempt to bind with incorrect password.
6d0b66
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
6d0b66
+        badconn = user.bind('badpassword')
6d0b66
+
6d0b66
+    user.set('userPassword', "{CRYPT}!")
6d0b66
+    # Attempt to bind with incorrect password.
6d0b66
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
6d0b66
+        badconn = user.bind('badpassword')
6d0b66
+
6d0b66
diff --git a/ldap/servers/plugins/pwdstorage/crypt_pwd.c b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
6d0b66
index 9031b2199..1b37d41ed 100644
6d0b66
--- a/ldap/servers/plugins/pwdstorage/crypt_pwd.c
6d0b66
+++ b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
6d0b66
@@ -48,15 +48,23 @@ static unsigned char itoa64[] = /* 0 ... 63 => ascii - 64 */
6d0b66
 int
6d0b66
 crypt_pw_cmp(const char *userpwd, const char *dbpwd)
6d0b66
 {
6d0b66
-    int rc;
6d0b66
-    char *cp;
6d0b66
+    int rc = -1;
6d0b66
+    char *cp = NULL;
6d0b66
+    size_t dbpwd_len = strlen(dbpwd);
6d0b66
     struct crypt_data data;
6d0b66
     data.initialized = 0;
6d0b66
 
6d0b66
-    /* we use salt (first 2 chars) of encoded password in call to crypt_r() */
6d0b66
-    cp = crypt_r(userpwd, dbpwd, &data);
6d0b66
-    if (cp) {
6d0b66
-        rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd));
6d0b66
+    /*
6d0b66
+     * there MUST be at least 2 chars of salt and some pw bytes, else this is INVALID and will
6d0b66
+     * allow any password to bind as we then only compare SALTS.
6d0b66
+     */
6d0b66
+    if (dbpwd_len >= 3) {
6d0b66
+        /* we use salt (first 2 chars) of encoded password in call to crypt_r() */
6d0b66
+        cp = crypt_r(userpwd, dbpwd, &data);
6d0b66
+    }
6d0b66
+    /* If these are not the same length, we can not proceed safely with memcmp. */
6d0b66
+    if (cp && dbpwd_len == strlen(cp)) {
6d0b66
+        rc = slapi_ct_memcmp(dbpwd, cp, dbpwd_len);
6d0b66
     } else {
6d0b66
         rc = -1;
6d0b66
     }
6d0b66
-- 
6d0b66
2.31.1
6d0b66