Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
openSUSE
389-ds.10198
0008-invalid-password-migration-causes-unauth-b...
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File 0008-invalid-password-migration-causes-unauth-bind.patch of Package 389-ds.10198
From a34311ef85dbb7a6ff9f982688b2bd74219795d7 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 - 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 | 4 +- ldap/servers/plugins/pwdstorage/sha_pwd.c | 16 +++++-- ldap/servers/plugins/pwdstorage/smd5_pwd.c | 2 +- ldap/servers/slapd/ch_malloc.c | 36 ++++++++++++-- ldap/servers/slapd/slapi-plugin.h | 2 +- 8 files changed, 108 insertions(+), 16 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 f5e6f9d..3d34075 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 3bd2265..0dccd1b 100644 --- a/ldap/servers/plugins/pwdstorage/crypt_pwd.c +++ b/ldap/servers/plugins/pwdstorage/crypt_pwd.c @@ -65,13 +65,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 1e2cf58..2c2aaca 100644 --- a/ldap/servers/plugins/pwdstorage/md5_pwd.c +++ b/ldap/servers/plugins/pwdstorage/md5_pwd.c @@ -30,7 +30,7 @@ int md5_pw_cmp(const char *userpwd, const char *dbpwd) { - int rc = -1; + int32_t rc = -1; char *bver; PK11Context *ctx = NULL; unsigned int outLen; @@ -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 = 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"); diff --git a/ldap/servers/plugins/pwdstorage/sha_pwd.c b/ldap/servers/plugins/pwdstorage/sha_pwd.c index 1fbe0bc..381b31d 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; @@ -122,9 +122,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 a83ac6f..cbfc74f 100644 --- a/ldap/servers/plugins/pwdstorage/smd5_pwd.c +++ b/ldap/servers/plugins/pwdstorage/smd5_pwd.c @@ -82,7 +82,7 @@ smd5_pw_cmp(const char *userpwd, const char *dbpwd) PK11_DestroyContext(ctx, 1); /* Compare everything up to the salt. */ - rc = slapi_ct_memcmp(userhash, dbhash, MD5_LENGTH); + rc = slapi_ct_memcmp(userhash, dbhash, MD5_LENGTH, MD5_LENGTH); loser: if (dbhash && dbhash != quick_dbhash) diff --git a/ldap/servers/slapd/ch_malloc.c b/ldap/servers/slapd/ch_malloc.c index ef436b3..90a2b2c 100644 --- a/ldap/servers/slapd/ch_malloc.c +++ b/ldap/servers/slapd/ch_malloc.c @@ -336,8 +336,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; @@ -347,9 +347,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 4566202..95cdcc0 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -5862,7 +5862,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
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
