On 03/19/2013 05:09 PM, Rob Crittenden wrote:
> Martin Kosek wrote:
>> On 03/19/2013 10:57 AM, Martin Kosek wrote:
>>> On 03/18/2013 04:07 PM, Rob Crittenden wrote:
>>>> Martin Kosek wrote:
>>>>> On 03/15/2013 04:42 PM, Rob Crittenden wrote:
>>>>>> Rob Crittenden wrote:
>>>>>>> Martin Kosek wrote:
>>>>>>>> On 03/11/2013 10:07 PM, Rob Crittenden wrote:
>>>>>>>>> Fixed a number of issues applying password policy against LDAP binds.
>>>>>>>>> See patch
>>>>>>>>> for details.
>>>>>>>>>
>>>>>>>>> rob
>>>>>>>>>
>>>>>>>>
>>>>>>>> I see some issues with this fix:
>>>>>>>>
>>>>>>>> 1) Shouldn't group password policy serve only as an override to the 
>>>>>>>> main
>>>>>>>> policy? I.e. if I have this policy:
>>>>>>>>
>>>>>>>> # ipa pwpolicy-show test
>>>>>>>>      Group: test
>>>>>>>>      Priority: 10
>>>>>>>>      Max failures: 2
>>>>>>>>
>>>>>>>> We should still follow settings other than "Max failures" configured in
>>>>>>>> global policy, right? At least the Kerberos seem to do it. I think we
>>>>>>>> should be consistent in this case. Now, other values just seem to be
>>>>>>>> zero.
>>>>>>>
>>>>>>> There should be only one policy. It isn't supposed to merge policies
>>>>>>> together (there is only one krbPwdPolicyReference per principal).
>>>>>
>>>>> That's a good point.
>>>>>
>>>>>>>
>>>>>>> How is the KDC acting differently?
>>>>>
>>>>> For example, if you set only maximal number of bad password guesses, it 
>>>>> does
>>>>> not allow any more (user fbar1 is a member of test group):
>>>>>
>>>>> # ipa pwpolicy-mod test --maxfail 3
>>>>>     Group: test
>>>>>     Priority: 10
>>>>>     Max failures: 3
>>>>>
>>>>> # kinit fbar1
>>>>> Password for fb...@idm.lab.bos.redhat.com:
>>>>> kinit: Password incorrect while getting initial credentials
>>>>> # kinit fbar1
>>>>> Password for fb...@idm.lab.bos.redhat.com:
>>>>> kinit: Password incorrect while getting initial credentials
>>>>> # kinit fbar1
>>>>> Password for fb...@idm.lab.bos.redhat.com:
>>>>> kinit: Password incorrect while getting initial credentials
>>>>> # kinit fbar1
>>>>> kinit: Clients credentials have been revoked while getting initial
>>>>> credentials
>>>>>
>>>>> But LDAP binds are still allowed
>>>>>
>>>>> # ldapsearch -h localhost -D
>>>>> uid=fbar1,cn=users,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com -x -w
>>>>> foo
>>>>> -s base -b ""
>>>>> ldap_bind: Invalid credentials (49)
>>>>>
>>>>> I think this is just caused by different processing of
>>>>> krbpwdfailurecountinterval in ipa-kdb and in bind preop (when it is not 
>>>>> set,
>>>>> max auth tries checks are pretty much disabled).
>>>>
>>>> We can't examine things until a successful bind is done. If it is done and 
>>>> we
>>>> determine that they should be locked out we refuse to continue.
>>>
>>> I am not sure I follow - what do you mean by "examine things"? The bind 
>>> preop
>>> plugin can check current policy with unsuccessful login, right? I just 
>>> thought
>>> that when the custom policy has krbpwdmaxfailure set, but does not have
>>> krbpwdfailurecountinterval set, we should still enforce the krbpwdmaxfailure
>>> "somehow", as ipa-kdb does.
>>>
>>> If not, I think we should either mark those params in pwpolicy plugin as
>>> required or at least add a note to ipa help mentioning this limitation for 
>>> LDAP
>>> binds....
>>>
>>>>
>>>>>
>>>>>>>
>>>>>>>> I think we will need to fix both the pre-op and the post-op to make 
>>>>>>>> this
>>>>>>>> working really consistently.
>>>>>>>>
>>>>>>>> 2) The lockout post-op still counts failed logins even though we are in
>>>>>>>> lockout time, is this expected? It is another point if inconsistency
>>>>>>>> with Kerberos auth. It leaves user's krbloginfailedcount stay on "Max
>>>>>>>> failures".
>>>>>>>
>>>>>>> Ok.
>>>>>>>
>>>>>>>>
>>>>>>>> 3) Sometimes, I get into a state when I lockout a new user with 
>>>>>>>> Kerberos
>>>>>>>> and then wait some time until the lockout time passes (no admin 
>>>>>>>> unlock),
>>>>>>>> I am able to run as many LDAP binds as I want.
>>>>>>>
>>>>>>> Can you clarify? Successful or unsuccessful binds?
>>>>>
>>>>> Unsuccessful binds. I will try to reproduce it again when you fix the
>>>>> crash, it
>>>>> is hard to investigate it with this crash around.
>>>>>
>>>>>>>
>>>>>>>> This is all I found so far. Honza is also reviewing it, so I will let
>>>>>>>> him post hist findings too.
>>>>>>>>
>>>>>>>> Martin
>>>>>>
>>>>>> Here is an updated patch to not increment past the max failures on LDAP
>>>>>> binds.
>>>>>
>>>>> The new patch now causes 389-ds to crash with SIGSEGV if I try to bind as 
>>>>> a
>>>>> user with no group policy assigned (Stacktrace attached).
>>>>
>>>> Stupid mistake on my part.
>>>>
>>>> rob
>>>>
>>>
>>> Fixed now :) I found one more issue. When you add a new user with a 
>>> password,
>>> wrong LDAP binds do not increase failure count until the latest failure
>>> timestamp is set, for example via failed Kerberos login:
>>>
>>> # ipa user-status fbar2
>>> -----------------------
>>> Account disabled: False
>>> -----------------------
>>>    Server: vm-037.idm.lab.bos.redhat.com
>>>    Failed logins: 0
>>>    Last successful authentication: N/A
>>>    Last failed authentication: N/A
>>>    Time now: 2013-03-19T09:01:35Z
>>> ----------------------------
>>> Number of entries returned 1
>>> ----------------------------
>>>
>>> # ldapsearch -h localhost -D
>>> uid=fbar2,cn=users,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com -x -w 
>>> foo
>>> -s base -b ""
>>> ldap_bind: Invalid credentials (49)
>>>
>>> # ipa user-status fbar2
>>> -----------------------
>>> Account disabled: False
>>> -----------------------
>>>    Server: vm-037.idm.lab.bos.redhat.com
>>>    Failed logins: 0
>>>    Last successful authentication: N/A
>>>    Last failed authentication: N/A
>>>    Time now: 2013-03-19T09:01:54Z
>>> ----------------------------
>>> Number of entries returned 1
>>> ----------------------------
>>>
>>> # kinit fbar2
>>> Password for fb...@idm.lab.bos.redhat.com:
>>> kinit: Password incorrect while getting initial credentials
>>>
>>> # ipa user-status fbar2
>>> -----------------------
>>> Account disabled: False
>>> -----------------------
>>>    Server: vm-037.idm.lab.bos.redhat.com
>>>    Failed logins: 1
>>>    Last successful authentication: N/A
>>>    Last failed authentication: 2013-03-19T09:02:14Z
>>>    Time now: 2013-03-19T09:02:16Z
>>> ----------------------------
>>> Number of entries returned 1
>>> ----------------------------
>>>
>>> # ldapsearch -h localhost -D
>>> uid=fbar2,cn=users,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com -x -w 
>>> foo
>>> -s base -b ""
>>> ldap_bind: Invalid credentials (49)
>>>
>>> # ipa user-status fbar2
>>> -----------------------
>>> Account disabled: False
>>> -----------------------
>>>    Server: vm-037.idm.lab.bos.redhat.com
>>>    Failed logins: 2     <<<<<< It increased now
>>>    Last successful authentication: N/A
>>>    Last failed authentication: 2013-03-19T09:02:20Z
>>>    Time now: 2013-03-19T09:02:24Z
>>> ----------------------------
>>> Number of entries returned 1
>>> ----------------------------
>>>
>>>
>>> As for the issue you could not reproduce, it is also quite difficult to
>>> reproduce it for me, I usually have to combine same failed/successful 
>>> logins,
>>> waits for lockouts etc. I need to look at that more closely. Just for the
>>> record, attaching an LDIF of a user in such a state when I can do as many
>>> failing ldapsearches as I want. This is the password policy:
>>>
>>> # ipa pwpolicy-show
>>>    Group: global_policy
>>>    Max lifetime (days): 90
>>>    Min lifetime (hours): 1
>>>    History size: 0
>>>    Character classes: 0
>>>    Min length: 8
>>>    Max failures: 6
>>>    Failure reset interval: 60
>>>    Lockout duration: 30
>>>
>>> Martin
>>>
>>
>> I discovered that the issue is in the postop:
>>
>> +            if (failedcount < max_fail) {
>> +                PR_snprintf(failedcountstr, sizeof(failedcountstr), "%lu",
>> failedcount);
>> +                slapi_mods_add_string(smods, LDAP_MOD_DELETE,
>> "krbLoginFailedCount", failedcountstr);
>> +                failedcount += 1;
>> +                PR_snprintf(failedcountstr, sizeof(failedcountstr), "%lu",
>> failedcount);
>> +                slapi_mods_add_string(smods, LDAP_MOD_ADD,
>> "krbLoginFailedCount", failedcountstr);
>>               }
>>
>> We try to delete failedcount and add failedcount+1, but this operation may 
>> fail
>> if the failedcount was set to 0 previously due to exceeded
>> krbpwdfailurecountinterval. This also makes the succeeding last failed auth
>> timestamp update to fail too and cause this vulnerability.
>>
>> I was able to fix it with this change:
>>
>> @@ -526,11 +535,9 @@ static int ipalockout_postop(Slapi_PBlock *pb)
>>            */
>>           if (failed_bind) {
>>               if (failedcount < max_fail) {
>> -                PR_snprintf(failedcountstr, sizeof(failedcountstr), "%lu",
>> failedcount);
>> -                slapi_mods_add_string(smods, LDAP_MOD_DELETE,
>> "krbLoginFailedCount", failedcountstr);
>>                   failedcount += 1;
>>                   PR_snprintf(failedcountstr, sizeof(failedcountstr), "%lu",
>> failedcount);
>> -                slapi_mods_add_string(smods, LDAP_MOD_ADD,
>> "krbLoginFailedCount", failedcountstr);
>> +                slapi_mods_add_string(smods, LDAP_MOD_REPLACE,
>> "krbLoginFailedCount", failedcountstr);
>>               }
>>               if (lastfail) {
>>                   if (!gmtime_r(&(time_now), &utctime)) {
>>
>> Martin
>>
> 
> I cam to a similar conclusion. We want to be careful when deleting values so 
> we
> can handle the case where multiple bind attempts are happening at once so we
> try to avoid using REPLACE. I changed it so we save the value of failedcount
> when the lockout expires.
> 
> I also added a chance to handle the case where there is no failedcount at all.
> I was trying to delete a non-existent 0 value.
> 
> rob

Ok, the updated version works well. I think we are getting close to finish this.

I now also tried krbPwdLockoutDuration=0 and it does not/cannot work properly
as we are still reseting failedcount to zero even though the account is locked 
out.

Shouldn't we do it only if the account is not in lockout period? I.e. like this:

...
    if (failedcount >= max_fail) {
        if (lockout_duration == 0) {
            errstr = "Entry permanently locked.\n";
            ret = LDAP_UNWILLING_TO_PERFORM;
            goto done;
        }

        if (time_now < last_failed + lockout_duration) {
            /* Too many failures */
            LOG_TRACE("Too many failed logins. %lu out of %d\n", failedcount,
max_fail);
            errstr = "Too many failed logins.\n";
            ret = LDAP_UNWILLING_TO_PERFORM;
        }
    } else if (lastfail != NULL) {
        /* Do failedcount processing */
    }
...

Same in postop, we reset failedcount to 0 even though we may be in lockout 
period.

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to