2:01 p.m.
dirsrvtests/tests/suites/password/pwd_algo_test.py | 143 +++++++++++++++++++++
ldap/servers/plugins/pwdstorage/clear_pwd.c | 33 ++++
ldap/servers/plugins/pwdstorage/crypt_pwd.c | 2
ldap/servers/plugins/pwdstorage/md5_pwd.c | 2
ldap/servers/plugins/pwdstorage/ns-mta-md5_pwd.c | 1
ldap/servers/plugins/pwdstorage/sha_pwd.c | 15 +-
ldap/servers/plugins/pwdstorage/smd5_pwd.c | 2
ldap/servers/slapd/ch_malloc.c | 24 +++
ldap/servers/slapd/slapi-plugin.h | 16 ++
9 files changed, 227 insertions(+), 11 deletions(-)
New commits:
commit 762219a35005914c6c088d915ac9346ce7e28512
Author: William Brown <firstyear(a)redhat.com>
Date: Thu Jul 21 13:22:30 2016 +1000
Ticket bz1358565 - clear and unsalted password types are vulnerable to timing attack
Bug Description: Clear and unsalted password types were vulnerable to a timing
attack. This is due to the use of memcmp and strcmp in their comparison.
Fix Description: Add a constant time memcmp function, that does not shortcircuit.
Change all password comparison to use the constant time check. For the clear
scheme, alter the way we do the check to prevent length disclosure timing
attacks.
This resolves CVE-2016-5405
https://bugzilla.redhat.com/show_bug.cgi?id=1358565
https://access.redhat.com/security/cve/CVE-2016-5405
Author: wibrown
Review by: nhosoi (Thanks!)
(cherry picked from commit 9dcaa4a0c866d8696e0a2616ccf962af2833f0b8)
diff --git a/ldap/servers/slapd/ch_malloc.c b/ldap/servers/slapd/ch_malloc.c
index 8f46970..52ccb64 100644
--- a/ldap/servers/slapd/ch_malloc.c
+++ b/ldap/servers/slapd/ch_malloc.c
@@ -123,7 +123,7 @@ slapi_ch_memalign(size_t size, size_t alignment)
int oserr = errno;
oom_occurred();
- slapi_log_error(SLAPI_LOG_ERR, SLAPD_MODULE,
+ slapi_log_err(SLAPI_LOG_ERR, SLAPD_MODULE,
"malloc of %lu bytes failed; OS error %d (%s)%s\n",
size, oserr, slapd_system_strerror( oserr ), oom_advice );
exit( 1 );
@@ -349,13 +349,12 @@ slapi_ct_memcmp( const void *p1, const void *p2, size_t n)
int result = 0;
const unsigned char *_p1 = (const unsigned char *)p1;
const unsigned char *_p2 = (const unsigned char *)p2;
- size_t i;
if (_p1 == NULL || _p2 == NULL) {
return 2;
}
- for (i = 0; i < n; i++) {
+ for (size_t i = 0; i < n; i++) {
if (_p1[i] ^ _p2[i]) {
result = 1;
}
commit 3548aff21be9f58e08b3174cb27d9b59af67cc58
Author: Noriko Hosoi <nhosoi(a)redhat.com>
Date: Thu Aug 4 13:26:44 2016 -0700
Ticket bz1358565 - clear and unsalted password types are vulnerable to timing attack
Description: Build fails with the commit f0e03b5a51972a125fe78f448d1f68e288782d1e:
error: 'for' loop initial declarations are only allowed in C99 mode
for (size_t i = 0; i < n; i++) {
^
Moved "size_t i;" to the top of slapi_ct_memcmp.
(cherry picked from commit 53da6d718b3dfee6cdd78e112d1926e90d03128a)
diff --git a/ldap/servers/slapd/ch_malloc.c b/ldap/servers/slapd/ch_malloc.c
index 7a40b74..8f46970 100644
--- a/ldap/servers/slapd/ch_malloc.c
+++ b/ldap/servers/slapd/ch_malloc.c
@@ -349,12 +349,13 @@ slapi_ct_memcmp( const void *p1, const void *p2, size_t n)
int result = 0;
const unsigned char *_p1 = (const unsigned char *)p1;
const unsigned char *_p2 = (const unsigned char *)p2;
+ size_t i;
if (_p1 == NULL || _p2 == NULL) {
return 2;
}
- for (size_t i = 0; i < n; i++) {
+ for (i = 0; i < n; i++) {
if (_p1[i] ^ _p2[i]) {
result = 1;
}
commit c4b5dc8bf325f0a358dc135b91023c3edc103a39
Author: William Brown <firstyear(a)redhat.com>
Date: Thu Jul 21 13:22:30 2016 +1000
Ticket bz1358565 - clear and unsalted password types are vulnerable to timing attack
Bug Description: Clear and unsalted password types were vulnerable to a timing
attack. This is due to the use of memcmp and strcmp in their comparison.
Fix Description: Add a constant time memcmp function, that does not shortcircuit.
Change all password comparison to use the constant time check. For the clear
scheme, alter the way we do the check to prevent length disclosure timing
attacks.
This resolves CVE-2016-5405
https://bugzilla.redhat.com/show_bug.cgi?id=1358565
https://access.redhat.com/security/cve/CVE-2016-5405
Author: wibrown
Review by: nhosoi (Thanks!)
(cherry picked from commit 9dcaa4a0c866d8696e0a2616ccf962af2833f0b8)
(cherry picked from commit f0e03b5a51972a125fe78f448d1f68e288782d1e)
diff --git a/dirsrvtests/tests/suites/password/pwd_algo_test.py
b/dirsrvtests/tests/suites/password/pwd_algo_test.py
new file mode 100644
index 0000000..aa8cbf5
--- /dev/null
+++ b/dirsrvtests/tests/suites/password/pwd_algo_test.py
@@ -0,0 +1,143 @@
+import os
+import sys
+import time
+import ldap
+import logging
+import pytest
+from lib389 import DirSrv, Entry, tools, tasks
+from lib389.tools import DirSrvTools
+from lib389._constants import *
+from lib389.properties import *
+from lib389.tasks import *
+from lib389.utils import *
+
+DEBUGGING = True
+USER_DN = 'uid=user,ou=People,%s' % DEFAULT_SUFFIX
+
+if DEBUGGING:
+ logging.getLogger(__name__).setLevel(logging.DEBUG)
+else:
+ logging.getLogger(__name__).setLevel(logging.INFO)
+
+
+log = logging.getLogger(__name__)
+
+
+class TopologyStandalone(object):
+ """The DS Topology Class"""
+ def __init__(self, standalone):
+ """Init"""
+ standalone.open()
+ self.standalone = standalone
+
+
+(a)pytest.fixture(scope="module")
+def topology(request):
+ """Create DS Deployment"""
+
+ # Creating standalone instance ...
+ if DEBUGGING:
+ standalone = DirSrv(verbose=True)
+ else:
+ standalone = DirSrv(verbose=False)
+ args_instance[SER_HOST] = HOST_STANDALONE
+ args_instance[SER_PORT] = PORT_STANDALONE
+ args_instance[SER_SERVERID_PROP] = SERVERID_STANDALONE
+ args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX
+ args_standalone = args_instance.copy()
+ standalone.allocate(args_standalone)
+ instance_standalone = standalone.exists()
+ if instance_standalone:
+ standalone.delete()
+ standalone.create()
+ standalone.open()
+
+ def fin():
+ """If we are debugging just stop the instances, otherwise remove
+ them
+ """
+ if DEBUGGING:
+ standalone.stop()
+ else:
+ standalone.delete()
+
+ request.addfinalizer(fin)
+
+ # Clear out the tmp dir
+ standalone.clearTmpDir(__file__)
+
+ return TopologyStandalone(standalone)
+
+def _test_bind(inst, password):
+ result = True
+ userconn = ldap.initialize("ldap://%s:%s" % (HOST_STANDALONE,
PORT_STANDALONE))
+ try:
+ userconn.simple_bind_s(USER_DN, password)
+ userconn.unbind_s()
+ except ldap.INVALID_CREDENTIALS:
+ result = False
+ return result
+
+def _test_algo(inst, algo_name):
+ inst.config.set('passwordStorageScheme', algo_name)
+
+ if DEBUGGING:
+ print('Testing %s', algo_name)
+
+ # Create the user with a password
+ inst.add_s(Entry((
+ USER_DN, {
+ 'objectClass': 'top account
simplesecurityobject'.split(),
+ 'uid': 'user',
+ 'userpassword': 'Secret123'
+ })))
+
+ # Make sure when we read the userPassword field, it is the correct ALGO
+ pw_field = inst.search_s(USER_DN, ldap.SCOPE_BASE, '(objectClass=*)',
['userPassword'] )[0]
+
+ if DEBUGGING:
+ print(pw_field.getValue('userPassword'))
+
+ if algo_name != 'CLEAR':
+ assert(algo_name.lower() in pw_field.getValue('userPassword').lower())
+ # Now make sure a bind works
+ assert(_test_bind(inst, 'Secret123'))
+ # Bind with a wrong shorter password, should fail
+ assert(not _test_bind(inst, 'Wrong'))
+ # Bind with a wrong longer password, should fail
+ assert(not _test_bind(inst, 'This is even more wrong'))
+ # Bind with a wrong exact length password.
+ assert(not _test_bind(inst, 'Alsowrong'))
+ # Bind with a subset password, should fail
+ assert(not _test_bind(inst, 'Secret'))
+ if algo_name != 'CRYPT':
+ # Bind with a subset password that is 1 char shorter, to detect off by 1 in
clear
+ assert(not _test_bind(inst, 'Secret12'))
+ # Bind with a superset password, should fail
+ assert(not _test_bind(inst, 'Secret123456'))
+ # Delete the user
+ inst.delete_s(USER_DN)
+ # done!
+
+def test_pwd_algo_test(topology):
+ """
+ Assert that all of our password algorithms correctly PASS and FAIL varying
+ password conditions.
+
+ """
+ if DEBUGGING:
+ # Add debugging steps(if any)...
+ pass
+
+ for algo in ('CLEAR', 'CRYPT', 'MD5', 'SHA',
'SHA256', 'SHA384', 'SHA512', 'SMD5', 'SSHA',
'SSHA256', 'SSHA384', 'SSHA512'):
+ _test_algo(topology.standalone, algo)
+
+ log.info('Test PASSED')
+
+
+if __name__ == '__main__':
+ # Run isolated
+ # -s for DEBUG mode
+ CURRENT_FILE = os.path.realpath(__file__)
+ pytest.main("-s %s" % CURRENT_FILE)
+
diff --git a/ldap/servers/plugins/pwdstorage/clear_pwd.c
b/ldap/servers/plugins/pwdstorage/clear_pwd.c
index b9b362d..2afe16e 100644
--- a/ldap/servers/plugins/pwdstorage/clear_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/clear_pwd.c
@@ -26,6 +26,7 @@ int
clear_pw_cmp( const char *userpwd, const char *dbpwd )
{
int result = 0;
+ int len = 0;
int len_user = strlen(userpwd);
int len_dbp = strlen(dbpwd);
if ( len_user != len_dbp ) {
diff --git a/ldap/servers/plugins/pwdstorage/crypt_pwd.c
b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
index 29355a2..93b54b2 100644
--- a/ldap/servers/plugins/pwdstorage/crypt_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/crypt_pwd.c
@@ -54,7 +54,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= strcmp( dbpwd, cp);
+ rc= slapi_ct_memcmp( dbpwd, cp, strlen(dbpwd));
} else {
rc = -1;
}
diff --git a/ldap/servers/plugins/pwdstorage/md5_pwd.c
b/ldap/servers/plugins/pwdstorage/md5_pwd.c
index a2b374b..b279946 100644
--- a/ldap/servers/plugins/pwdstorage/md5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/md5_pwd.c
@@ -57,7 +57,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 = strcmp(bver,dbpwd);
+ 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");
diff --git a/ldap/servers/plugins/pwdstorage/ns-mta-md5_pwd.c
b/ldap/servers/plugins/pwdstorage/ns-mta-md5_pwd.c
index 2fed61f..ae1f7b8 100644
--- a/ldap/servers/plugins/pwdstorage/ns-mta-md5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/ns-mta-md5_pwd.c
@@ -84,6 +84,7 @@ ns_mta_md5_pw_cmp(const char * clear, const char *mangled)
mta_hash[32] = mta_salt[32] = 0;
+ /* This is salted, so we don't need to change it for constant time */
return( strcmp(mta_hash,ns_mta_hash_alg(buffer,mta_salt,clear)));
}
diff --git a/ldap/servers/plugins/pwdstorage/sha_pwd.c
b/ldap/servers/plugins/pwdstorage/sha_pwd.c
index 30fe725..5f41c5b 100644
--- a/ldap/servers/plugins/pwdstorage/sha_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/sha_pwd.c
@@ -120,13 +120,16 @@ sha_pw_cmp (const char *userpwd, const char *dbpwd, unsigned int
shaLen )
}
/* the proof is in the comparison... */
- result = ( hash_len >= shaLen ) ?
- ( memcmp( userhash, dbhash, shaLen ) ) : /* include salt */
- ( memcmp( userhash, dbhash + OLD_SALT_LENGTH,
- hash_len - OLD_SALT_LENGTH ) ); /* exclude salt */
+ if ( hash_len >= shaLen ) {
+ result = slapi_ct_memcmp( userhash, dbhash, shaLen );
+ } else {
+ result = slapi_ct_memcmp( userhash, dbhash + OLD_SALT_LENGTH, hash_len -
OLD_SALT_LENGTH );
+ }
- loser:
- if ( dbhash && dbhash != quick_dbhash ) slapi_ch_free_string( &dbhash );
+loser:
+ if ( dbhash && dbhash != quick_dbhash ) {
+ slapi_ch_free_string( &dbhash );
+ }
return result;
}
diff --git a/ldap/servers/plugins/pwdstorage/smd5_pwd.c
b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
index 1309d28..2e9d195 100644
--- a/ldap/servers/plugins/pwdstorage/smd5_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/smd5_pwd.c
@@ -80,7 +80,7 @@ smd5_pw_cmp( const char *userpwd, const char *dbpwd )
PK11_DestroyContext(ctx, 1);
/* Compare everything up to the salt. */
- rc = memcmp( userhash, dbhash, MD5_LENGTH );
+ rc = slapi_ct_memcmp( userhash, dbhash, 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 8278de4..7a40b74 100644
--- a/ldap/servers/slapd/ch_malloc.c
+++ b/ldap/servers/slapd/ch_malloc.c
@@ -340,3 +340,25 @@ slapi_ch_smprintf(const char *fmt, ...)
return p;
}
#endif
+
+/* 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)
+{
+ int result = 0;
+ const unsigned char *_p1 = (const unsigned char *)p1;
+ const unsigned char *_p2 = (const unsigned char *)p2;
+
+ if (_p1 == NULL || _p2 == NULL) {
+ return 2;
+ }
+
+ for (size_t i = 0; i < n; i++) {
+ if (_p1[i] ^ _p2[i]) {
+ result = 1;
+ }
+ }
+ return result;
+}
+
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index f4253de..7c087e6 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -5837,6 +5837,22 @@ char * slapi_ch_smprintf(const char *fmt, ...)
#else
;
#endif
+/**
+ * slapi_ct_memcmp is a constant time memory comparison function. This is for
+ * use with password hashes and other locations which could lead to a timing
+ * attack due to early shortcut returns. This function *does not* shortcircuit
+ * during the comparison, always checking every byte regardless if it has already
+ * found that the memory does not match.
+ *
+ * WARNING! p1 and p2 must both reference content that is at least of size 'n'.
+ * Else this function may over-run (And will certainly fail).
+ *
+ * \param p1 pointer to first value to check.
+ * \param p2 pointer to second value to check.
+ * \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);
/*
* syntax plugin routines
commit 3d92679cf97518aedcf6534ac5967edf8d2c9d28
Author: Noriko Hosoi <nhosoi(a)redhat.com>
Date: Mon Aug 8 10:12:33 2016 -0700
Ticket bz1358565 - clear and unsalted password types are vulnerable to timing attack
Description: Fixing a compiler warning introduced by commit
f0e03b5a51972a125fe78f448d1f68e288782d1e.
(cherry picked from commit c62ea0c98445d31fb55baebe9778fe860b3266ea)
diff --git a/ldap/servers/plugins/pwdstorage/clear_pwd.c
b/ldap/servers/plugins/pwdstorage/clear_pwd.c
index 84dac2a..b9b362d 100644
--- a/ldap/servers/plugins/pwdstorage/clear_pwd.c
+++ b/ldap/servers/plugins/pwdstorage/clear_pwd.c
@@ -25,7 +25,37 @@
int
clear_pw_cmp( const char *userpwd, const char *dbpwd )
{
- return( strcmp( userpwd, dbpwd ));
+ int result = 0;
+ int len_user = strlen(userpwd);
+ int len_dbp = strlen(dbpwd);
+ if ( len_user != len_dbp ) {
+ result = 1;
+ }
+ /* We have to do this comparison ANYWAY else we have a length timing attack. */
+ if ( len_user >= len_dbp ) {
+ /*
+ * If they are the same length, result will be 0 here, and if we pass
+ * the check, we don't update result either. IE we pass.
+ * 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) {
+ result = 1;
+ }
+ } else {
+ /*
+ * If we stretched the userPassword, we'll allow a new timing attack, where
+ * if we see a delay on a short pw, we know we are stretching.
+ * when the delay goes away, it means we've found the length.
+ * Instead, because we don't want to use the short pw for comp, we just
compare
+ * 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)) {
+ /* Do nothing, we have the if to fix a coverity check. */
+ }
+ }
+ return result;
}
char *