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