From ea31415690291ab5be6c70a20738e5b1291b92dd Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Mon, 26 Feb 2018 18:09:32 -0500
Subject: [PATCH] Bug 1525628 - CVE-2017-15135: Authentication bypass due to
lack of size check in slapi_ct_memcmp function in ch_malloc.c
Description: Backport this fix top 1.2.11(RHEL 6.9)
---
ldap/servers/plugins/pwdstorage/clear_pwd.c | 4 +-
ldap/servers/plugins/pwdstorage/crypt_pwd.c | 2 +-
ldap/servers/plugins/pwdstorage/md5_pwd.c | 2 +-
ldap/servers/plugins/pwdstorage/sha_pwd.c | 16 ++++++--
ldap/servers/plugins/pwdstorage/smd5_pwd.c | 60 +++++++++++++++--------------
ldap/servers/slapd/ch_malloc.c | 34 ++++++++++++++--
ldap/servers/slapd/slapi-plugin.h | 2 +-
7 files changed, 79 insertions(+), 41 deletions(-)
diff --git a/ldap/servers/plugins/pwdstorage/clear_pwd.c b/ldap/servers/plugins/pwdstorage/clear_pwd.c
index 5a889d4aa..55809a252 100644
--- a/ldap/servers/plugins/pwdstorage/clear_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/clear_pwd.c
@@ -68,7 +68,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_dbp, len_dbp) != 0) {
result = 1;
}
} else {
@@ -80,7 +80,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 37557deb1..eb498e603 100644
--- a/ldap/servers/plugins/pwdstorage/crypt_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
@@ -87,7 +87,7 @@ crypt_pw_cmp( const char *userpwd, const char *dbpwd )
/* 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 d00ef945b..8e352edd3 100644
--- a/ldap/servers/plugins/pwdstorage/md5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/md5_pwd.c
@@ -86,7 +86,7 @@ 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_error(SLAPI_LOG_PLUGIN, MD5_SUBSYSTEM_NAME,
"Could not base64 encode hashed value for password compare");
diff --git a/ldap/servers/plugins/pwdstorage/sha_pwd.c b/ldap/servers/plugins/pwdstorage/sha_pwd.c
index 1b787524b..994e20480 100644
--- a/ldap/servers/plugins/pwdstorage/sha_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/sha_pwd.c
@@ -78,7 +78,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};
int hash_len; /* must be a signed valued -- see below */
unsigned int secOID;
char *schemeName;
@@ -150,9 +150,19 @@ 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 );
+ /*
+ * 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 ecf9760a4..5afff6022 100644
--- a/ldap/servers/plugins/pwdstorage/smd5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
@@ -81,35 +81,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_error( 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_error(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 38bca5c18..1da053e3f 100644
--- a/ldap/servers/slapd/ch_malloc.c
+++ b/ldap/servers/slapd/ch_malloc.c
@@ -740,7 +740,7 @@ memory_record_dump( caddr_t data, caddr_t arg )
/* 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)
+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;
@@ -751,9 +751,35 @@ slapi_ct_memcmp( const void *p1, const void *p2, size_t n)
return 2;
}
- for (i = 0; i < n; i++) {
- if (_p1[i] ^ _p2[i]) {
- result = 1;
+ if (n1 == n2) {
+ for (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 (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 7f361731b..ac040c716 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -5531,7 +5531,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);
+int slapi_ct_memcmp( const void *p1, const void *p2, size_t n1, size_t n2);
/*
* syntax plugin routines
--
2.13.6