path: root/net-nds/389-ds-base/files/389-ds-base-1.3.6-backport-invalid-password-mig.patch

From cefec5714cf0fdec4aa582a5fe020ef80d6024cd Mon Sep 17 00:00:00 2001
From: William Brown <firstyear@redhat.com>
Date: Thu, 18 Jan 2018 11:27:58 +1000
Subject: [PATCH] Ticket bz1525628 1.3.6 backport - invalid password migration
 causes unauth bind

Bug Description:  Slapi_ct_memcmp expects both inputs to be
at LEAST size n. If they are not, we only compared UP to n.

Invalid migrations of passwords (IE {CRYPT}XX) would create
a pw which is just salt and no hash. ct_memcmp would then
only verify the salt bits and would allow the authentication.

This relies on an administrative mistake both of allowing
password migration (nsslapd-allow-hashed-passwords) and then
subsequently migrating an INVALID password to the server.

Fix Description:  slapi_ct_memcmp now access n1, n2 size
and will FAIL if they are not the same, but will still compare
n bytes, where n is the "longest" memory, to the first byte
of the other to prevent length disclosure of the shorter
value (generally the mis-migrated password)

https://bugzilla.redhat.com/show_bug.cgi?id=1525628

Author: wibrown

Review by: ???
---
 .../bz1525628_ct_memcmp_invalid_hash_test.py       | 56 ++++++++++++++++++++
 ldap/servers/plugins/pwdstorage/clear_pwd.c        |  4 +-
 ldap/servers/plugins/pwdstorage/crypt_pwd.c        |  4 +-
 ldap/servers/plugins/pwdstorage/md5_pwd.c          | 36 ++++++-------
 ldap/servers/plugins/pwdstorage/sha_pwd.c          | 18 +++++--
 ldap/servers/plugins/pwdstorage/smd5_pwd.c         | 60 +++++++++++-----------
 ldap/servers/slapd/ch_malloc.c                     | 36 +++++++++++--
 ldap/servers/slapd/slapi-plugin.h                  |  2 +-
 8 files changed, 155 insertions(+), 61 deletions(-)
 create mode 100644 dirsrvtests/tests/suites/password/bz1525628_ct_memcmp_invalid_hash_test.py

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
new file mode 100644
index 0000000..2f38384
--- /dev/null
+++ b/dirsrvtests/tests/suites/password/bz1525628_ct_memcmp_invalid_hash_test.py
@@ -0,0 +1,56 @@
+# --- BEGIN COPYRIGHT BLOCK ---
+# Copyright (C) 2018 Red Hat, Inc.
+# All rights reserved.
+#
+# License: GPL (version 3 or any later version).
+# See LICENSE for details.
+# --- END COPYRIGHT BLOCK ---
+#
+
+import ldap
+import pytest
+import logging
+from lib389.topologies import topology_st
+from lib389._constants import PASSWORD, DEFAULT_SUFFIX
+
+from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES
+
+logging.getLogger(__name__).setLevel(logging.DEBUG)
+log = logging.getLogger(__name__)
+
+def test_invalid_hash_fails(topology_st):
+    """When given a malformed hash from userpassword migration
+    slapi_ct_memcmp would check only to the length of the shorter
+    field. This affects some values where it would ONLY verify
+    the salt is valid, and thus would allow any password to bind.
+
+    :id: 8131c029-7147-47db-8d03-ec5db2a01cfb
+    :setup: Standalone Instance
+    :steps:
+        1. Create a user
+        2. Add an invalid password hash (truncated)
+        3. Attempt to bind
+    :expectedresults:
+        1. User is added
+        2. Invalid pw hash is added
+        3. Bind fails
+    """
+    log.info("Running invalid hash test")
+
+    # Allow setting raw password hashes for migration.
+    topology_st.standalone.config.set('nsslapd-allow-hashed-passwords', 'on')
+
+    users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)
+    user = users.create(properties=TEST_USER_PROPERTIES)
+    user.set('userPassword', '{CRYPT}XX')
+
+    # Attempt to bind. This should fail.
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
+        user.bind(PASSWORD)
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
+        user.bind('XX')
+    with pytest.raises(ldap.INVALID_CREDENTIALS):
+        user.bind('{CRYPT}XX')
+
+    log.info("PASSED")
+
diff --git a/ldap/servers/plugins/pwdstorage/clear_pwd.c b/ldap/servers/plugins/pwdstorage/clear_pwd.c
index b9b362d..050e60d 100644
--- a/ldap/servers/plugins/pwdstorage/clear_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/clear_pwd.c
@@ -39,7 +39,7 @@ clear_pw_cmp( const char *userpwd, const char *dbpwd )
          * However, even if the first part of userpw matches dbpwd, but len !=, we
          * have already failed anyawy. This prevents substring matching.
          */
-        if (slapi_ct_memcmp(userpwd, dbpwd, len_dbp) != 0) {
+        if (slapi_ct_memcmp(userpwd, dbpwd, len_user, len_dbp) != 0) {
             result = 1;
         }
     } else {
@@ -51,7 +51,7 @@ clear_pw_cmp( const char *userpwd, const char *dbpwd )
          * dbpwd to itself. We have already got result == 1 if we are here, so we are
          * just trying to take up time!
          */
-        if (slapi_ct_memcmp(dbpwd, dbpwd, len_dbp)) {
+        if (slapi_ct_memcmp(dbpwd, dbpwd, len_dbp, len_dbp)) {
             /* Do nothing, we have the if to fix a coverity check. */
         }
     }
diff --git a/ldap/servers/plugins/pwdstorage/crypt_pwd.c b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
index dfd5af9..5fcff13 100644
--- a/ldap/servers/plugins/pwdstorage/crypt_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
@@ -56,13 +56,13 @@ crypt_close(Slapi_PBlock *pb __attribute__((unused)))
 int
 crypt_pw_cmp( const char *userpwd, const char *dbpwd )
 {
-    int rc;
+    int32_t rc;
     char *cp;
     PR_Lock(cryptlock);
     /* we use salt (first 2 chars) of encoded password in call to crypt() */
     cp = crypt( userpwd, dbpwd );
     if (cp) {
-       rc= slapi_ct_memcmp( dbpwd, cp, strlen(dbpwd));
+        rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd), strlen(cp));
     } else {
        rc = -1;
     }
diff --git a/ldap/servers/plugins/pwdstorage/md5_pwd.c b/ldap/servers/plugins/pwdstorage/md5_pwd.c
index b279946..2e1c472 100644
--- a/ldap/servers/plugins/pwdstorage/md5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/md5_pwd.c
@@ -30,13 +30,13 @@
 int
 md5_pw_cmp( const char *userpwd, const char *dbpwd )
 {
-   int rc=-1;
-   char * bver;
-   PK11Context *ctx=NULL;
-   unsigned int outLen;
-   unsigned char hash_out[MD5_HASH_LEN];
-   unsigned char b2a_out[MD5_HASH_LEN*2]; /* conservative */
-   SECItem binary_item;
+    int32_t rc = -1;
+    char *bver;
+    PK11Context *ctx = NULL;
+    unsigned int outLen;
+    unsigned char hash_out[MD5_HASH_LEN];
+    unsigned char b2a_out[MD5_HASH_LEN * 2]; /* conservative */
+    SECItem binary_item;
 
    ctx = PK11_CreateDigestContext(SEC_OID_MD5);
    if (ctx == NULL) {
@@ -51,17 +51,17 @@ md5_pw_cmp( const char *userpwd, const char *dbpwd )
    PK11_DigestFinal(ctx, hash_out, &outLen, sizeof hash_out);
    PK11_DestroyContext(ctx, 1);
 
-   /* convert the binary hash to base64 */
-   binary_item.data = hash_out;
-   binary_item.len = outLen;
-   bver = NSSBase64_EncodeItem(NULL, (char *)b2a_out, sizeof b2a_out, &binary_item);
-   /* bver points to b2a_out upon success */
-   if (bver) {
-	   rc = slapi_ct_memcmp(bver,dbpwd, strlen(dbpwd));
-   } else {
-	   slapi_log_err(SLAPI_LOG_PLUGIN, MD5_SUBSYSTEM_NAME,
-					   "Could not base64 encode hashed value for password compare");
-   }
+    /* convert the binary hash to base64 */
+    binary_item.data = hash_out;
+    binary_item.len = outLen;
+    bver = NSSBase64_EncodeItem(NULL, (char *)b2a_out, sizeof b2a_out, &binary_item);
+    /* bver points to b2a_out upon success */
+    if (bver) {
+        rc = slapi_ct_memcmp(bver, dbpwd, strlen(dbpwd), strlen(bver));
+    } else {
+        slapi_log_err(SLAPI_LOG_PLUGIN, MD5_SUBSYSTEM_NAME,
+                      "Could not base64 encode hashed value for password compare");
+    }
 loser:
    return rc;
 }
diff --git a/ldap/servers/plugins/pwdstorage/sha_pwd.c b/ldap/servers/plugins/pwdstorage/sha_pwd.c
index 5f41c5b..c9db896 100644
--- a/ldap/servers/plugins/pwdstorage/sha_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/sha_pwd.c
@@ -49,7 +49,7 @@ sha_pw_cmp (const char *userpwd, const char *dbpwd, unsigned int shaLen )
     char userhash[MAX_SHA_HASH_SIZE];
     char quick_dbhash[MAX_SHA_HASH_SIZE + SHA_SALT_LENGTH + 3];
     char *dbhash = quick_dbhash;
-    struct berval salt;
+    struct berval salt = {0};
     PRUint32 hash_len;
     unsigned int secOID;
     char *schemeName;
@@ -120,10 +120,20 @@ sha_pw_cmp (const char *userpwd, const char *dbpwd, unsigned int shaLen )
     }
 
     /* the proof is in the comparison... */
-    if ( hash_len >= shaLen ) {
-        result = slapi_ct_memcmp( userhash, dbhash, shaLen );
+    if (hash_len >= shaLen) {
+        /*
+         * This say "if the hash has a salt IE >, OR if they are equal, check the hash component ONLY.
+         * This is why we repeat shaLen twice, even though it seems odd. If you have a dbhast of ssha
+         * it's len is 28, and the userpw is 20, but 0 - 20 is the sha, and 21-28 is the salt, which
+         * has already been processed into userhash.
+         * The case where dbpwd is truncated is handled above in "invalid base64" arm.
+         */
+        result = slapi_ct_memcmp(userhash, dbhash, shaLen, shaLen);
     } else {
-        result = slapi_ct_memcmp( userhash, dbhash + OLD_SALT_LENGTH, hash_len - OLD_SALT_LENGTH );
+        /* This case is for if the salt is at the START, which only applies to DS40B1 case.
+         * May never be a valid check...
+         */
+        result = slapi_ct_memcmp(userhash, dbhash + OLD_SALT_LENGTH, shaLen, hash_len - OLD_SALT_LENGTH);
     }
 
 loser:
diff --git a/ldap/servers/plugins/pwdstorage/smd5_pwd.c b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
index 2e9d195..f6b4bb4 100644
--- a/ldap/servers/plugins/pwdstorage/smd5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
@@ -52,35 +52,37 @@ smd5_pw_cmp( const char *userpwd, const char *dbpwd )
    /*
     * Decode hash stored in database.
     */
-   hash_len = pwdstorage_base64_decode_len(dbpwd, 0);
-   if ( hash_len >= sizeof(quick_dbhash) ) { /* get more space: */
-      dbhash = (char*) slapi_ch_calloc( hash_len + 1, sizeof(char) );
-      if ( dbhash == NULL ) goto loser;
-   } else {
-      memset( quick_dbhash, 0, sizeof(quick_dbhash) );
-   }
-
-   hashresult = PL_Base64Decode( dbpwd, 0, dbhash );
-   if (NULL == hashresult) {
-      slapi_log_err(SLAPI_LOG_PLUGIN, SALTED_MD5_SUBSYSTEM_NAME,
-            "smd5_pw_cmp: userPassword \"%s\" is the wrong length "
-            "or is not properly encoded BASE64\n", dbpwd );
-      goto loser;
-   }
-
-   salt.bv_val = (void*)(dbhash + MD5_LENGTH); /* salt starts after hash value */
-   salt.bv_len = hash_len - MD5_LENGTH; /* remaining bytes must be salt */
-
-   /* create the hash */
-   memset( userhash, 0, sizeof(userhash) );
-   PK11_DigestBegin(ctx);
-   PK11_DigestOp(ctx, (const unsigned char *)userpwd, strlen(userpwd));
-   PK11_DigestOp(ctx, (unsigned char*)(salt.bv_val), salt.bv_len);
-   PK11_DigestFinal(ctx, userhash, &outLen, sizeof userhash);
-   PK11_DestroyContext(ctx, 1);
-
-   /* Compare everything up to the salt. */
-   rc = slapi_ct_memcmp( userhash, dbhash, MD5_LENGTH );
+    hash_len = pwdstorage_base64_decode_len(dbpwd, 0);
+    if (hash_len >= sizeof(quick_dbhash)) { /* get more space: */
+        dbhash = (char *)slapi_ch_calloc(hash_len + 1, sizeof(char));
+        if (dbhash == NULL)
+            goto loser;
+    } else {
+        memset(quick_dbhash, 0, sizeof(quick_dbhash));
+    }
+
+    hashresult = PL_Base64Decode(dbpwd, 0, dbhash);
+    if (NULL == hashresult) {
+        slapi_log_err(SLAPI_LOG_PLUGIN, SALTED_MD5_SUBSYSTEM_NAME,
+                      "smd5_pw_cmp: userPassword \"%s\" is the wrong length "
+                      "or is not properly encoded BASE64\n",
+                      dbpwd);
+        goto loser;
+    }
+
+    salt.bv_val = (void *)(dbhash + MD5_LENGTH); /* salt starts after hash value */
+    salt.bv_len = hash_len - MD5_LENGTH;         /* remaining bytes must be salt */
+
+    /* create the hash */
+    memset(userhash, 0, sizeof(userhash));
+    PK11_DigestBegin(ctx);
+    PK11_DigestOp(ctx, (const unsigned char *)userpwd, strlen(userpwd));
+    PK11_DigestOp(ctx, (unsigned char *)(salt.bv_val), salt.bv_len);
+    PK11_DigestFinal(ctx, userhash, &outLen, sizeof userhash);
+    PK11_DestroyContext(ctx, 1);
+
+    /* Compare everything up to the salt. */
+    rc = slapi_ct_memcmp(userhash, dbhash, MD5_LENGTH, MD5_LENGTH);
 
 loser:
    if ( dbhash && dbhash != quick_dbhash ) slapi_ch_free_string( (char **)&dbhash );
diff --git a/ldap/servers/slapd/ch_malloc.c b/ldap/servers/slapd/ch_malloc.c
index 52ccb64..66cb692 100644
--- a/ldap/servers/slapd/ch_malloc.c
+++ b/ldap/servers/slapd/ch_malloc.c
@@ -343,8 +343,8 @@ slapi_ch_smprintf(const char *fmt, ...)
 
 /* Constant time memcmp. Does not shortcircuit on failure! */
 /* This relies on p1 and p2 both being size at least n! */
-int
-slapi_ct_memcmp( const void *p1, const void *p2, size_t n)
+int32_t
+slapi_ct_memcmp(const void *p1, const void *p2, size_t n1, size_t n2)
 {
     int result = 0;
     const unsigned char *_p1 = (const unsigned char *)p1;
@@ -354,9 +354,35 @@ slapi_ct_memcmp( const void *p1, const void *p2, size_t n)
         return 2;
     }
 
-    for (size_t i = 0; i < n; i++) {
-        if (_p1[i] ^ _p2[i]) {
-            result = 1;
+    if (n1 == n2) {
+        for (size_t i = 0; i < n1; i++) {
+            if (_p1[i] ^ _p2[i]) {
+                result = 1;
+            }
+        }
+    } else {
+        const unsigned char *_pa;
+        const unsigned char *_pb;
+        size_t nl;
+        if (n2 > n1) {
+            _pa = _p2;
+            _pb = _p2;
+            nl = n2;
+        } else {
+            _pa = _p1;
+            _pb = _p1;
+            nl = n1;
+        }
+        /* We already fail as n1 != n2 */
+        result = 3;
+        for (size_t i = 0; i < nl; i++) {
+            if (_pa[i] ^ _pb[i]) {
+                /*
+                 * If we don't mutate result here, dead code elimination
+                 * we remove for loop.
+                 */
+                result = 4;
+            }
         }
     }
     return result;
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index d37bc63..2c5c4ce 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -5859,7 +5859,7 @@ char * slapi_ch_smprintf(const char *fmt, ...)
  * \param n length in bytes of the content of p1 AND p2.
  * \return 0 on match. 1 on non-match. 2 on presence of NULL pointer in p1 or p2.
  */
-int slapi_ct_memcmp( const void *p1, const void *p2, size_t n);
+int32_t slapi_ct_memcmp(const void *p1, const void *p2, size_t n1, size_t n2);
 
 /*
  * syntax plugin routines
-- 
1.8.3.1