From 95b39e29361812a62f2e038c89a88d717c82794e Mon Sep 17 00:00:00 2001
From: William Brown <firstyear@redhat.com>
Date: Mon, 31 Jul 2017 14:13:49 +1000
Subject: [PATCH] Ticket 49336 - SECURITY: Locked account provides different
return code
Bug Description: The directory server password lockout policy prevents binds
from operating once a threshold of failed passwords has been met. During
this lockout, if you bind with a successful password, a different error code
is returned. This means that an attacker has no ratelimit or penalty during
an account lock, and can continue to attempt passwords via bruteforce, using
the change in return code to ascertain a sucessful password auth.
Fix Description: Move the account lock check *before* the password bind
check. If the account is locked, we do not mind disclosing this as the
attacker will either ignore it (and will not bind anyway), or they will
be forced to back off as the attack is not working preventing the
bruteforce.
https://pagure.io/389-ds-base/issue/49336
Author: wibrown
Review by: tbordaz (Thanks!)
Signed-off-by: Mark Reynolds <mreynolds@redhat.com>
---
.../suites/password/pwd_lockout_bypass_test.py | 55 ++++++++++++++++++++++
ldap/servers/slapd/bind.c | 29 ++++++++----
ldap/servers/slapd/pw_verify.c | 15 +++---
3 files changed, 84 insertions(+), 15 deletions(-)
create mode 100644 dirsrvtests/tests/suites/password/pwd_lockout_bypass_test.py
diff --git a/dirsrvtests/tests/suites/password/pwd_lockout_bypass_test.py b/dirsrvtests/tests/suites/password/pwd_lockout_bypass_test.py
new file mode 100644
index 0000000..e4add72
--- /dev/null
+++ b/dirsrvtests/tests/suites/password/pwd_lockout_bypass_test.py
@@ -0,0 +1,55 @@
+# --- BEGIN COPYRIGHT BLOCK ---
+# Copyright (C) 2017 Red Hat, Inc.
+# All rights reserved.
+#
+# License: GPL (version 3 or any later version).
+# See LICENSE for details.
+# --- END COPYRIGHT BLOCK ---
+#
+import pytest
+from lib389.tasks import *
+from lib389.utils import *
+from lib389.topologies import topology_st
+from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES
+import ldap
+
+# The irony of these names is not lost on me.
+GOOD_PASSWORD = 'password'
+BAD_PASSWORD = 'aontseunao'
+
+logging.getLogger(__name__).setLevel(logging.INFO)
+log = logging.getLogger(__name__)
+
+def test_lockout_bypass(topology_st):
+ inst = topology_st.standalone
+
+ # Configure the lock policy
+ inst.config.set('passwordMaxFailure', '1')
+ inst.config.set('passwordLockoutDuration', '99999')
+ inst.config.set('passwordLockout', 'on')
+
+ # Create the account
+ users = UserAccounts(inst, DEFAULT_SUFFIX)
+ testuser = users.create(properties=TEST_USER_PROPERTIES)
+ testuser.set('userPassword', GOOD_PASSWORD)
+
+ conn = testuser.bind(GOOD_PASSWORD)
+ assert conn != None
+ conn.unbind_s()
+
+ # Bind with bad creds twice
+ # This is the failure.
+ with pytest.raises(ldap.INVALID_CREDENTIALS):
+ conn = testuser.bind(BAD_PASSWORD)
+ # Now we should not be able to ATTEMPT the bind. It doesn't matter that
+ # we disclose that we have hit the rate limit here, what matters is that
+ # it exists.
+ with pytest.raises(ldap.CONSTRAINT_VIOLATION):
+ conn = testuser.bind(BAD_PASSWORD)
+
+ # now bind with good creds
+ # Should be error 19 still.
+ with pytest.raises(ldap.CONSTRAINT_VIOLATION):
+ conn = testuser.bind(GOOD_PASSWORD)
+
+
diff --git a/ldap/servers/slapd/bind.c b/ldap/servers/slapd/bind.c
index 7f4414f..064ace1 100644
--- a/ldap/servers/slapd/bind.c
+++ b/ldap/servers/slapd/bind.c
@@ -662,12 +662,14 @@ do_bind( Slapi_PBlock *pb )
/* We could be serving multiple database backends. Select the appropriate one */
/* pw_verify_be_dn will select the backend we need for us. */
- if (auto_bind) {
- /* We have no password material. We should just check who we are binding as. */
- rc = pw_validate_be_dn(pb, &referral);
- } else {
- rc = pw_verify_be_dn(pb, &referral);
- }
+ /*
+ * WARNING: We have to validate *all* other conditions *first* before
+ * we attempt the bind!
+ *
+ * this is because ldbm_bind.c will SEND THE FAILURE.
+ */
+
+ rc = pw_validate_be_dn(pb, &referral);
if (rc == SLAPI_BIND_NO_BACKEND) {
send_nobackend_ldap_result( pb );
@@ -736,8 +738,18 @@ do_bind( Slapi_PBlock *pb )
myrc = 0;
}
if (!auto_bind) {
- /*
- * There could be a race that bind_target_entry was not added
+ /*
+ * Okay, we've made it here. FINALLY check if the entry really
+ * can bind or not. THIS IS THE PASSWORD CHECK.
+ */
+ rc = pw_verify_be_dn(pb, &referral);
+ if (rc != SLAPI_BIND_SUCCESS) {
+ /* Invalid pass - lets bail ... */
+ goto bind_failed;
+ }
+
+ /*
+ * There could be a race that bind_target_entry was not added
* when bind_target_entry was retrieved before be_bind, but it
* was in be_bind. Since be_bind returned SLAPI_BIND_SUCCESS,
* the entry is in the DS. So, we need to retrieve it once more.
@@ -786,6 +798,7 @@ do_bind( Slapi_PBlock *pb )
}
}
} else { /* if auto_bind || rc == slapi_bind_success | slapi_bind_anonymous */
+ bind_failed:
if (rc == LDAP_OPERATIONS_ERROR) {
send_ldap_result( pb, LDAP_UNWILLING_TO_PERFORM, NULL, "Function not implemented", 0, NULL );
goto free_and_return;
diff --git a/ldap/servers/slapd/pw_verify.c b/ldap/servers/slapd/pw_verify.c
index 852b027..cb182ed 100644
--- a/ldap/servers/slapd/pw_verify.c
+++ b/ldap/servers/slapd/pw_verify.c
@@ -55,7 +55,7 @@ pw_verify_root_dn(const char *dn, const Slapi_Value *cred)
int
pw_verify_be_dn(Slapi_PBlock *pb, Slapi_Entry **referral)
{
- int rc = 0;
+ int rc = SLAPI_BIND_SUCCESS;
Slapi_Backend *be = NULL;
if (slapi_mapping_tree_select(pb, &be, referral, NULL, 0) != LDAP_SUCCESS) {
@@ -109,14 +109,10 @@ pw_validate_be_dn(Slapi_PBlock *pb, Slapi_Entry **referral)
slapi_pblock_get(pb, SLAPI_BIND_CREDENTIALS, &cred);
slapi_pblock_get(pb, SLAPI_BIND_METHOD, &method);
- if (pb_sdn != NULL || cred != NULL) {
+ if (pb_sdn == NULL) {
return LDAP_OPERATIONS_ERROR;
}
- if (*referral) {
- return SLAPI_BIND_REFERRAL;
- }
-
/* We need a slapi_sdn_isanon? */
if (method == LDAP_AUTH_SIMPLE && cred->bv_len == 0) {
return SLAPI_BIND_ANONYMOUS;
@@ -130,7 +126,11 @@ pw_validate_be_dn(Slapi_PBlock *pb, Slapi_Entry **referral)
if (slapi_mapping_tree_select(pb, &be, referral, NULL, 0) != LDAP_SUCCESS) {
return SLAPI_BIND_NO_BACKEND;
}
- slapi_be_Unlock(be);
+
+ if (*referral) {
+ slapi_be_Unlock(be);
+ return SLAPI_BIND_REFERRAL;
+ }
slapi_pblock_set(pb, SLAPI_BACKEND, be);
slapi_pblock_set(pb, SLAPI_PLUGIN, be->be_database);
@@ -138,6 +138,7 @@ pw_validate_be_dn(Slapi_PBlock *pb, Slapi_Entry **referral)
set_db_default_result_handlers(pb);
/* The backend associated with this identity is real. */
+ slapi_be_Unlock(be);
return SLAPI_BIND_SUCCESS;
}
--
2.9.4