Simultaneous adding a user and binding as the user could fail in the password policy check

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/47748

Created at 2014-03-19 04:17:33 by nhosoi (@nhosoi)
Closed as Fixed
Assigned to nhosoi (@nhosoi)
Associated bugzillas

Simultaneous adding a user and binding as the user could fail in the password policy check since the bind_target_entry is not created at the timing when it is retrieved.

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-21 06:29:57

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1079098

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-21 06:30:33

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1079099

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-21 06:35:51

git patch file (master) 0001-Ticket-47748-Simultaneous-adding-a-user-and-binding-.patch

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-21 06:36:32

Bug description: In do_bind, bind_target_entry is retrieved from the DB or the entry cache. There was a small window that the entry failed to retrieve from there but the bind procedure in the backend (be_bind) succeeds. In the case, NULL bind_target_entry is passed to the Pass- word Policy check and it fails.

Fix description: If be_bind returns SUCCESS and bind_target_entry is NULL, retrieve bind_target_entry agian, which is guaranteed since the entry was retrieved in the backend and placed in the entry cache.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2014-03-21 07:36:00

    784                             rc = need_new_pw(pb, &t, bind_target_entry, pw_response_requested); 
    785                             switch (need_new_pw(pb, &t, bind_target_entry, pw_response_requested)) {

I think this should be switch (rc) {

What about this code?

            /* get the entry now, so that we can give it to slapi_check_account_lock and reslimit_update_from_dn */
            if (! slapi_be_is_flag_set(be, SLAPI_BE_FLAG_REMOTE_DATA)) {
                bind_target_entry = get_entry(pb,  slapi_sdn_get_ndn(sdn));
                rc = slapi_check_account_lock ( pb, bind_target_entry, pw_response_requested, 1, 1);
            }

This is also before the be->be_bind call.

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-21 22:03:26

Replying to [comment:6 richm]:

  784                             rc = need_new_pw(pb, &t, bind_target_entry, pw_response_requested); 
  785                             switch (need_new_pw(pb, &t, bind_target_entry, pw_response_requested)) {

I think this should be switch (rc) { You are right. shame shame...

What about this code?

            /* get the entry now, so that we can give it to slapi_check_account_lock and reslimit_update_from_dn */
            if (! slapi_be_is_flag_set(be, SLAPI_BE_FLAG_REMOTE_DATA)) {
                bind_target_entry = get_entry(pb,  slapi_sdn_get_ndn(sdn));
                rc = slapi_check_account_lock ( pb, bind_target_entry, pw_response_requested, 1, 1);
            }

This is also before the be->be_bind call. I thought this is okay since we want to check if the account is locked or not as early as possible before going into the expensive backend procedure. Normally, bind_target_entry exists... But, again you are right. If we get bind_target_entry later, we have no chance to check if the account is locked or not... I'm modifying the patch something like this and testing it. Once the test is passed, I'm going to send out the review request take 2. Thanks, Rich!!


diff --git a/ldap/servers/slapd/bind.c b/ldap/servers/slapd/bind.c
index 0a889a0..2deb448 100644
--- a/ldap/servers/slapd/bind.c
+++ b/ldap/servers/slapd/bind.c
@@ -771,7 +771,13 @@ do_bind( Slapi_PBlock *pb )
*/
if (!bind_target_entry) {
bind_target_entry = get_entry(pb, slapi_sdn_get_ndn(sdn));
-                            if (!bind_target_entry) {
+                            if (bind_target_entry) {
+                                rc = slapi_check_account_lock(pb, bind_target_entry,
+                                                              pw_response_requested, 1, 1);
+                                if (1 == rc) { /* account is locked */
+                                    goto account_locked;
+                                }
+                            } else {
goto free_and_return;
}
}
@@ -782,7 +788,7 @@ do_bind( Slapi_PBlock *pb )
/* check if need new password before sending 
the bind success result */
rc = need_new_pw(pb, &t, bind_target_entry, pw_response_requested);
-                            switch (need_new_pw(pb, &t, bind_target_entry, pw_response_requested)) {
+                            switch (rc) {
case 1:
(void)slapi_add_pwd_control(pb, LDAP_CONTROL_PWEXPIRED, 0);
break;
@@ -810,7 +816,7 @@ do_bind( Slapi_PBlock *pb )
}
}
} else {
-                
+account_locked:                
if(cred.bv_len == 0) {
/* its an UnAuthenticated Bind, DN specified but no pw */
slapi_counter_increment(g_get_global_snmp_vars()->ops_tbl.dsUnAuthBinds);

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-22 00:23:15

git patch file (master; take 2) -- fixed mistakes in the previous patch (Thanks, Rich!!) 0001-Ticket-47748-Simultaneous-adding-a-user-and-binding-.2.patch

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2014-03-22 01:34:22

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-03-22 02:31:41

Reviewed by Rich (Thank you!!)

Pushed to master: dbfab50..4fc53e1 master -> master commit 4fc53e1a63222d0ff67c30a59f2cff4b535f90a8

Pushed to 389-ds-base-1.3.2: cdffb52..f8f063c 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit f8f063c3fb2c7642506cbad923c71972f78edac2

Pushed to 389-ds-base-1.3.1: 97ed029..feed9ba 389-ds-base-1.3.1 -> 389-ds-base-1.3.1 commit feed9ba773766ade744ca4d45aca46c91d4b7f4f

Pushed to 389-ds-base-1.2.11: 872b7d9..f9b2bec 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit f9b2bec732645bf0d219e790448b191c4652858e

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-09-10 02:11:30

git patch file (master) -- fixing a regression: "Simple bind hangs after enabling password policy" 0001-Ticket-47748-Simultaneous-adding-a-user-and-binding-.3.patch

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2014-09-10 02:48:17

Reviewed by Mark (Thank you!!)

Pushed to master: f5a0f6a..4f11606 master -> master commit 4f11606b02419c8ccdb319b8040e683af9109d1b

Pushed to 389-ds-base-1.3.3: b17f09b..8c82941 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 8c82941c0f2b0b5d7fa698a1ca3e4f26245cf85a

Pushed to 389-ds-base-1.3.2: eb6d5a3..5b6d60e 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 5b6d60ec4d3d93d1d69f6a071ce135a06f4c8cfd

Pushed to 389-ds-base-1.2.11: 39f44c5..aa935c9 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit aa935c9a9297ab22d3c7fc17381e735521d9cd03

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2017-02-11 22:54:37

Metadata Update from @nhosoi:

Issue assigned to nhosoi
Issue set to the milestone: 1.2.11.29

389ds / 389-ds-base

Simultaneous adding a user and binding as the user could fail in the password policy check #1080