#47748 Simultaneous adding a user and binding as the user could fail in the password policy check
Closed: wontfix None Opened 10 years ago by nhosoi.

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.


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.

{{{
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.

Replying to [comment:6 rmeggins]:

{{{
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);

}}}

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

Reviewed by Rich (Thank you!!)

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

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

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 f9b2bec

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

Reviewed by Mark (Thank you!!)

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

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

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

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

Metadata Update from @nhosoi:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.2.11.29

7 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1080

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata