Blame SOURCES/0065-Ticket-bz1525628-invalid-password-migration-causes-u.patch

081b2d
From 40fcaabfaa2c865471cc5fb1fab04106bc3ec611 Mon Sep 17 00:00:00 2001
081b2d
From: William Brown <firstyear@redhat.com>
081b2d
Date: Thu, 18 Jan 2018 11:27:58 +1000
081b2d
Subject: [PATCH] Ticket bz1525628 - invalid password migration causes unauth
081b2d
 bind
081b2d
081b2d
Bug Description:  Slapi_ct_memcmp expects both inputs to be
081b2d
at LEAST size n. If they are not, we only compared UP to n.
081b2d
081b2d
Invalid migrations of passwords (IE {CRYPT}XX) would create
081b2d
a pw which is just salt and no hash. ct_memcmp would then
081b2d
only verify the salt bits and would allow the authentication.
081b2d
081b2d
This relies on an administrative mistake both of allowing
081b2d
password migration (nsslapd-allow-hashed-passwords) and then
081b2d
subsequently migrating an INVALID password to the server.
081b2d
081b2d
Fix Description:  slapi_ct_memcmp now access n1, n2 size
081b2d
and will FAIL if they are not the same, but will still compare
081b2d
n bytes, where n is the "longest" memory, to the first byte
081b2d
of the other to prevent length disclosure of the shorter
081b2d
value (generally the mis-migrated password)
081b2d
081b2d
https://bugzilla.redhat.com/show_bug.cgi?id=1525628
081b2d
081b2d
Author: wibrown
081b2d
081b2d
Review by: ???
081b2d
081b2d
Signed-off-by: Mark Reynolds <mreynolds@redhat.com>
081b2d
---
081b2d
 .../bz1525628_ct_memcmp_invalid_hash_test.py       | 56 ++++++++++++++++++++++
081b2d
 ldap/servers/plugins/pwdstorage/clear_pwd.c        |  4 +-
081b2d
 ldap/servers/plugins/pwdstorage/crypt_pwd.c        |  4 +-
081b2d
 ldap/servers/plugins/pwdstorage/md5_pwd.c          |  4 +-
081b2d
 ldap/servers/plugins/pwdstorage/sha_pwd.c          | 16 +++++--
081b2d
 ldap/servers/plugins/pwdstorage/smd5_pwd.c         |  2 +-
081b2d
 ldap/servers/slapd/ch_malloc.c                     | 36 ++++++++++++--
081b2d
 ldap/servers/slapd/slapi-plugin.h                  |  2 +-
081b2d
 8 files changed, 108 insertions(+), 16 deletions(-)
081b2d
 create mode 100644 dirsrvtests/tests/suites/password/bz1525628_ct_memcmp_invalid_hash_test.py
081b2d
081b2d
diff --git a/dirsrvtests/tests/suites/password/bz1525628_ct_memcmp_invalid_hash_test.py b/dirsrvtests/tests/suites/password/bz1525628_ct_memcmp_invalid_hash_test.py
081b2d
new file mode 100644
081b2d
index 000000000..2f38384a1
081b2d
--- /dev/null
081b2d
+++ b/dirsrvtests/tests/suites/password/bz1525628_ct_memcmp_invalid_hash_test.py
081b2d
@@ -0,0 +1,56 @@
081b2d
+# --- BEGIN COPYRIGHT BLOCK ---
081b2d
+# Copyright (C) 2018 Red Hat, Inc.
081b2d
+# All rights reserved.
081b2d
+#
081b2d
+# License: GPL (version 3 or any later version).
081b2d
+# See LICENSE for details.
081b2d
+# --- END COPYRIGHT BLOCK ---
081b2d
+#
081b2d
+
081b2d
+import ldap
081b2d
+import pytest
081b2d
+import logging
081b2d
+from lib389.topologies import topology_st
081b2d
+from lib389._constants import PASSWORD, DEFAULT_SUFFIX
081b2d
+
081b2d
+from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES
081b2d
+
081b2d
+logging.getLogger(__name__).setLevel(logging.DEBUG)
081b2d
+log = logging.getLogger(__name__)
081b2d
+
081b2d
+def test_invalid_hash_fails(topology_st):
081b2d
+    """When given a malformed hash from userpassword migration
081b2d
+    slapi_ct_memcmp would check only to the length of the shorter
081b2d
+    field. This affects some values where it would ONLY verify
081b2d
+    the salt is valid, and thus would allow any password to bind.
081b2d
+
081b2d
+    :id: 8131c029-7147-47db-8d03-ec5db2a01cfb
081b2d
+    :setup: Standalone Instance
081b2d
+    :steps:
081b2d
+        1. Create a user
081b2d
+        2. Add an invalid password hash (truncated)
081b2d
+        3. Attempt to bind
081b2d
+    :expectedresults:
081b2d
+        1. User is added
081b2d
+        2. Invalid pw hash is added
081b2d
+        3. Bind fails
081b2d
+    """
081b2d
+    log.info("Running invalid hash test")
081b2d
+
081b2d
+    # Allow setting raw password hashes for migration.
081b2d
+    topology_st.standalone.config.set('nsslapd-allow-hashed-passwords', 'on')
081b2d
+
081b2d
+    users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)
081b2d
+    user = users.create(properties=TEST_USER_PROPERTIES)
081b2d
+    user.set('userPassword', '{CRYPT}XX')
081b2d
+
081b2d
+    # Attempt to bind. This should fail.
081b2d
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
081b2d
+        user.bind(PASSWORD)
081b2d
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
081b2d
+        user.bind('XX')
081b2d
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
081b2d
+        user.bind('{CRYPT}XX')
081b2d
+
081b2d
+    log.info("PASSED")
081b2d
+
081b2d
diff --git a/ldap/servers/plugins/pwdstorage/clear_pwd.c b/ldap/servers/plugins/pwdstorage/clear_pwd.c
081b2d
index f5e6f9d4c..3d340752d 100644
081b2d
--- a/ldap/servers/plugins/pwdstorage/clear_pwd.c
081b2d
+++ b/ldap/servers/plugins/pwdstorage/clear_pwd.c
081b2d
@@ -39,7 +39,7 @@ clear_pw_cmp(const char *userpwd, const char *dbpwd)
081b2d
          * However, even if the first part of userpw matches dbpwd, but len !=, we
081b2d
          * have already failed anyawy. This prevents substring matching.
081b2d
          */
081b2d
-        if (slapi_ct_memcmp(userpwd, dbpwd, len_dbp) != 0) {
081b2d
+        if (slapi_ct_memcmp(userpwd, dbpwd, len_user, len_dbp) != 0) {
081b2d
             result = 1;
081b2d
         }
081b2d
     } else {
081b2d
@@ -51,7 +51,7 @@ clear_pw_cmp(const char *userpwd, const char *dbpwd)
081b2d
          * dbpwd to itself. We have already got result == 1 if we are here, so we are
081b2d
          * just trying to take up time!
081b2d
          */
081b2d
-        if (slapi_ct_memcmp(dbpwd, dbpwd, len_dbp)) {
081b2d
+        if (slapi_ct_memcmp(dbpwd, dbpwd, len_dbp, len_dbp)) {
081b2d
             /* Do nothing, we have the if to fix a coverity check. */
081b2d
         }
081b2d
     }
081b2d
diff --git a/ldap/servers/plugins/pwdstorage/crypt_pwd.c b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
081b2d
index 3bd226581..0dccd1b51 100644
081b2d
--- a/ldap/servers/plugins/pwdstorage/crypt_pwd.c
081b2d
+++ b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
081b2d
@@ -65,13 +65,13 @@ crypt_close(Slapi_PBlock *pb __attribute__((unused)))
081b2d
 int
081b2d
 crypt_pw_cmp(const char *userpwd, const char *dbpwd)
081b2d
 {
081b2d
-    int rc;
081b2d
+    int32_t rc;
081b2d
     char *cp;
081b2d
     PR_Lock(cryptlock);
081b2d
     /* we use salt (first 2 chars) of encoded password in call to crypt() */
081b2d
     cp = crypt(userpwd, dbpwd);
081b2d
     if (cp) {
081b2d
-        rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd));
081b2d
+        rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd), strlen(cp));
081b2d
     } else {
081b2d
         rc = -1;
081b2d
     }
081b2d
diff --git a/ldap/servers/plugins/pwdstorage/md5_pwd.c b/ldap/servers/plugins/pwdstorage/md5_pwd.c
081b2d
index 1e2cf58e7..2c2aacaa6 100644
081b2d
--- a/ldap/servers/plugins/pwdstorage/md5_pwd.c
081b2d
+++ b/ldap/servers/plugins/pwdstorage/md5_pwd.c
081b2d
@@ -30,7 +30,7 @@
081b2d
 int
081b2d
 md5_pw_cmp(const char *userpwd, const char *dbpwd)
081b2d
 {
081b2d
-    int rc = -1;
081b2d
+    int32_t rc = -1;
081b2d
     char *bver;
081b2d
     PK11Context *ctx = NULL;
081b2d
     unsigned int outLen;
081b2d
@@ -57,7 +57,7 @@ md5_pw_cmp(const char *userpwd, const char *dbpwd)
081b2d
     bver = NSSBase64_EncodeItem(NULL, (char *)b2a_out, sizeof b2a_out, &binary_item);
081b2d
     /* bver points to b2a_out upon success */
081b2d
     if (bver) {
081b2d
-        rc = slapi_ct_memcmp(bver, dbpwd, strlen(dbpwd));
081b2d
+        rc = slapi_ct_memcmp(bver, dbpwd, strlen(dbpwd), strlen(bver));
081b2d
     } else {
081b2d
         slapi_log_err(SLAPI_LOG_PLUGIN, MD5_SUBSYSTEM_NAME,
081b2d
                       "Could not base64 encode hashed value for password compare");
081b2d
diff --git a/ldap/servers/plugins/pwdstorage/sha_pwd.c b/ldap/servers/plugins/pwdstorage/sha_pwd.c
081b2d
index 1fbe0bc82..381b31d7c 100644
081b2d
--- a/ldap/servers/plugins/pwdstorage/sha_pwd.c
081b2d
+++ b/ldap/servers/plugins/pwdstorage/sha_pwd.c
081b2d
@@ -49,7 +49,7 @@ sha_pw_cmp(const char *userpwd, const char *dbpwd, unsigned int shaLen)
081b2d
     char userhash[MAX_SHA_HASH_SIZE];
081b2d
     char quick_dbhash[MAX_SHA_HASH_SIZE + SHA_SALT_LENGTH + 3];
081b2d
     char *dbhash = quick_dbhash;
081b2d
-    struct berval salt;
081b2d
+    struct berval salt = {0};
081b2d
     PRUint32 hash_len;
081b2d
     unsigned int secOID;
081b2d
     char *schemeName;
081b2d
@@ -122,9 +122,19 @@ sha_pw_cmp(const char *userpwd, const char *dbpwd, unsigned int shaLen)
081b2d
 
081b2d
     /* the proof is in the comparison... */
081b2d
     if (hash_len >= shaLen) {
081b2d
-        result = slapi_ct_memcmp(userhash, dbhash, shaLen);
081b2d
+        /*
081b2d
+         * This say "if the hash has a salt IE >, OR if they are equal, check the hash component ONLY.
081b2d
+         * This is why we repeat shaLen twice, even though it seems odd. If you have a dbhast of ssha
081b2d
+         * it's len is 28, and the userpw is 20, but 0 - 20 is the sha, and 21-28 is the salt, which
081b2d
+         * has already been processed into userhash.
081b2d
+         * The case where dbpwd is truncated is handled above in "invalid base64" arm.
081b2d
+         */
081b2d
+        result = slapi_ct_memcmp(userhash, dbhash, shaLen, shaLen);
081b2d
     } else {
081b2d
-        result = slapi_ct_memcmp(userhash, dbhash + OLD_SALT_LENGTH, hash_len - OLD_SALT_LENGTH);
081b2d
+        /* This case is for if the salt is at the START, which only applies to DS40B1 case.
081b2d
+         * May never be a valid check...
081b2d
+         */
081b2d
+        result = slapi_ct_memcmp(userhash, dbhash + OLD_SALT_LENGTH, shaLen, hash_len - OLD_SALT_LENGTH);
081b2d
     }
081b2d
 
081b2d
 loser:
081b2d
diff --git a/ldap/servers/plugins/pwdstorage/smd5_pwd.c b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
081b2d
index a83ac6fa4..cbfc74ff3 100644
081b2d
--- a/ldap/servers/plugins/pwdstorage/smd5_pwd.c
081b2d
+++ b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
081b2d
@@ -82,7 +82,7 @@ smd5_pw_cmp(const char *userpwd, const char *dbpwd)
081b2d
     PK11_DestroyContext(ctx, 1);
081b2d
 
081b2d
     /* Compare everything up to the salt. */
081b2d
-    rc = slapi_ct_memcmp(userhash, dbhash, MD5_LENGTH);
081b2d
+    rc = slapi_ct_memcmp(userhash, dbhash, MD5_LENGTH, MD5_LENGTH);
081b2d
 
081b2d
 loser:
081b2d
     if (dbhash && dbhash != quick_dbhash)
081b2d
diff --git a/ldap/servers/slapd/ch_malloc.c b/ldap/servers/slapd/ch_malloc.c
081b2d
index ef436b3e8..90a2b2c1a 100644
081b2d
--- a/ldap/servers/slapd/ch_malloc.c
081b2d
+++ b/ldap/servers/slapd/ch_malloc.c
081b2d
@@ -336,8 +336,8 @@ slapi_ch_smprintf(const char *fmt, ...)
081b2d
 
081b2d
 /* Constant time memcmp. Does not shortcircuit on failure! */
081b2d
 /* This relies on p1 and p2 both being size at least n! */
081b2d
-int
081b2d
-slapi_ct_memcmp(const void *p1, const void *p2, size_t n)
081b2d
+int32_t
081b2d
+slapi_ct_memcmp(const void *p1, const void *p2, size_t n1, size_t n2)
081b2d
 {
081b2d
     int result = 0;
081b2d
     const unsigned char *_p1 = (const unsigned char *)p1;
081b2d
@@ -347,9 +347,35 @@ slapi_ct_memcmp(const void *p1, const void *p2, size_t n)
081b2d
         return 2;
081b2d
     }
081b2d
 
081b2d
-    for (size_t i = 0; i < n; i++) {
081b2d
-        if (_p1[i] ^ _p2[i]) {
081b2d
-            result = 1;
081b2d
+    if (n1 == n2) {
081b2d
+        for (size_t i = 0; i < n1; i++) {
081b2d
+            if (_p1[i] ^ _p2[i]) {
081b2d
+                result = 1;
081b2d
+            }
081b2d
+        }
081b2d
+    } else {
081b2d
+        const unsigned char *_pa;
081b2d
+        const unsigned char *_pb;
081b2d
+        size_t nl;
081b2d
+        if (n2 > n1) {
081b2d
+            _pa = _p2;
081b2d
+            _pb = _p2;
081b2d
+            nl = n2;
081b2d
+        } else {
081b2d
+            _pa = _p1;
081b2d
+            _pb = _p1;
081b2d
+            nl = n1;
081b2d
+        }
081b2d
+        /* We already fail as n1 != n2 */
081b2d
+        result = 3;
081b2d
+        for (size_t i = 0; i < nl; i++) {
081b2d
+            if (_pa[i] ^ _pb[i]) {
081b2d
+                /*
081b2d
+                 * If we don't mutate result here, dead code elimination
081b2d
+                 * we remove for loop.
081b2d
+                 */
081b2d
+                result = 4;
081b2d
+            }
081b2d
         }
081b2d
     }
081b2d
     return result;
081b2d
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
081b2d
index 4566202d3..95cdcc0da 100644
081b2d
--- a/ldap/servers/slapd/slapi-plugin.h
081b2d
+++ b/ldap/servers/slapd/slapi-plugin.h
081b2d
@@ -5862,7 +5862,7 @@ char *slapi_ch_smprintf(const char *fmt, ...)
081b2d
  * \param n length in bytes of the content of p1 AND p2.
081b2d
  * \return 0 on match. 1 on non-match. 2 on presence of NULL pointer in p1 or p2.
081b2d
  */
081b2d
-int slapi_ct_memcmp(const void *p1, const void *p2, size_t n);
081b2d
+int32_t slapi_ct_memcmp(const void *p1, const void *p2, size_t n1, size_t n2);
081b2d
 
081b2d
 /*
081b2d
  * syntax plugin routines
081b2d
-- 
081b2d
2.13.6
081b2d