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: ???
Gbp-Pq: Name CVE-2017-15135.patch
--- /dev/null
+# --- 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")
+
* 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 {
* 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. */
}
}
int
crypt_pw_cmp(const char *userpwd, const char *dbpwd)
{
- int rc;
+ int32_t rc;
char *cp;
struct crypt_data data;
data.initialized = 0;
/* we use salt (first 2 chars) of encoded password in call to crypt_r() */
cp = crypt_r(userpwd, dbpwd, &data);
if (cp) {
- rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd));
+ rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd), strlen(cp));
} else {
rc = -1;
}
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;
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");
/* the proof is in the comparison... */
if (hash_len >= shaLen) {
- result = slapi_ct_memcmp(userhash, dbhash, shaLen);
+ result = slapi_ct_memcmp(userhash, dbhash, shaLen, shaLen);
} else {
- result = slapi_ct_memcmp(userhash, dbhash + OLD_SALT_LENGTH, hash_len - OLD_SALT_LENGTH);
+ result = slapi_ct_memcmp(userhash, dbhash + OLD_SALT_LENGTH, hash_len - OLD_SALT_LENGTH, hash_len - OLD_SALT_LENGTH);
}
loser:
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)
/* 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;
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;
* \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
projects / 389-ds-base.git / commitdiff