From 4219c54d9706f5597e8186d4f983b30587c2762e Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Tue, 13 Feb 2018 10:32:06 -0500 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 --- ldap/servers/plugins/pwdstorage/clear_pwd.c | 4 +- ldap/servers/plugins/pwdstorage/crypt_pwd.c | 4 +- ldap/servers/plugins/pwdstorage/md5_pwd.c | 14 +++---- ldap/servers/plugins/pwdstorage/sha_pwd.c | 18 +++++++-- ldap/servers/plugins/pwdstorage/smd5_pwd.c | 60 +++++++++++++++-------------- ldap/servers/slapd/ch_malloc.c | 37 +++++++++++++++--- ldap/servers/slapd/slapi-plugin.h | 2 +- 7 files changed, 88 insertions(+), 51 deletions(-) diff --git a/ldap/servers/plugins/pwdstorage/clear_pwd.c b/ldap/servers/plugins/pwdstorage/clear_pwd.c index b9b362d34..050e60dd7 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 dfd5af94b..ff8eabb07 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 b27994667..88c11688b 100644 --- a/ldap/servers/plugins/pwdstorage/md5_pwd.c +++ b/ldap/servers/plugins/pwdstorage/md5_pwd.c @@ -30,12 +30,12 @@ int md5_pw_cmp( const char *userpwd, const char *dbpwd ) { - int rc=-1; - char * bver; - PK11Context *ctx=NULL; + 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 */ + unsigned char b2a_out[MD5_HASH_LEN * 2]; /* conservative */ SECItem binary_item; ctx = PK11_CreateDigestContext(SEC_OID_MD5); @@ -57,10 +57,10 @@ md5_pw_cmp( const char *userpwd, const char *dbpwd ) 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)); + 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"); + 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 5f41c5b93..c9db8964a 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 2e9d195ea..f6b4bb4a0 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 52ccb64e8..da0b5f6d8 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; @@ -353,10 +353,35 @@ slapi_ct_memcmp( const void *p1, const void *p2, size_t n) if (_p1 == NULL || _p2 == NULL) { 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 16aa1b711..8555be939 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -5856,7 +5856,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 -- 2.13.6