collective / collective.pwexpiry

Emulate Active Directory password complexity requirements.
1 stars 6 forks source link

Lockout after x failed attempts doesn't seem to work #22

Closed CorySanin closed 6 years ago

CorySanin commented 6 years ago

I installed collective.pwexpiry on a couple test sites in Plone 5.0.8 and 5.1.2. Password history stuff works as expected. I have no idea how to test if password expiration works without just waiting it out (If there's a way to manually set a password's age in order to test that, I would really appreciate some help with that also). Locking users out after x failed attempts doesn't appear to work at all on either version of Plone. I can't get any users to appear on the page that lists locked-out users. I'm not sure if it's just me or if the feature is actually broken.

Personally, I feel the lockout feature goes beyond the scope of this add-on. The only reason I noticed that it wasn't working for me was because I wanted to see if setting the number of bad attempts to 0 would disable it. But I didn't even get that far yet because having it set to 2 or 3 doesn't result in accounts getting locked out.

Is it just me? Do I have to do anything else other than just adding pwexpiry to my list of eggs in my buildout?

CorySanin commented 6 years ago

I can't seem to get password expiration to work, either. Maybe I didn't set it up correctly? Did I miss something?

csanahuja commented 6 years ago

I have just tested it on Plone 5.1. and it fails when the user whitelisted is empty. I have filled a PR.

Also I have seen once the user is blocked he only receive the account-is-blocked message when he logs with correct credentials, not when he keeps sending wrong passwords, is this the expected behavior?

Also @CorySanin, taken from the product here it shows how to see the password age to current date, maybe it helps you to do the desired, just change current_time creating another time with DateTime (https://docs.plone.org/develop/plone/misc/datetime.html)

    user = api.user.get(username='username')
    portal = api.portal.get()
    current_time = portal.ZopeTime()
    user.setMemberProperties({'password_date': current_time})
CorySanin commented 6 years ago

Huh. That's not what I personally would expect for account locking. Isn't the purpose of locking an account to prevent an attacker from brute-forcing the password online without the knowledge of the account holder? If the attacker simply clears their cookies between attempts, they can keep trying passwords, right? I just tried that and that seemed to work. A script could trivially bypass the locking mechanism in this manner, I believe.

csanahuja commented 6 years ago

The locking mechanism cannot be bypassed cleaning cookies as the account its locked internally. However you are right that with current status an attacker can keep trying passwords and he will exactly know when he get the right password as the feedback is different.

Looking the code a bit, seems easy to just tell the user the account is locked when the password fails, but the verification of the password would be made anyway so I am not sure if that is the best way

CorySanin commented 6 years ago

I meant before the account gets locked. Correct me if I'm wrong, but the account only gets locked if the correct password is entered after x failed attempts. If that is true, an attacker can attempt once, clean cookies, then attempt again. As far as the system is concerned, each attempt is the first attempt. When the correct password is finally attempted, it too will be the first attempt and will not lock the account.

csanahuja commented 6 years ago

The account gets locked after X failed attempts (by default 3) for 24h. It gets locked for everybody. If the user logs correctly and the account is not locked the counter of failed attempts resets to zero, if the account is already locked the user can't log.

There is no usage of cookies, the info related is stored individually for each user in the db. See this https://github.com/collective/collective.pwexpiry/blob/master/collective/pwexpiry/subscriber.py

CorySanin commented 6 years ago

Oh I see. I was only under the impression that it used session data to keep track of failed attempts because my testing seemed to support that. But the truth was that the feature just wasn't working. Strange thing was that just before I attempted to bypass locking an account, locking an account did work as expected. Perhaps that was just a fluke, I'm not sure.

That being said, my next concern was exactly what you mentioned before. If nobody notices that the account got locked, then the attacker still knows the password and can wait out the 24 hours. Seems to me it's just a matter of locking an account on bad attempt number x instead of waiting for the correct password, or at the very least showing the message at bad attempt x (but still only locking the account when the correct password is entered). That way the attacker doesn't get any confirmation that the password entered was correct.

Anyway, that goes way beyond the original reason I created the issue. So I did a fresh installation of Plone 5.1, added collective.pwexpiry to the list of eggs, started my instance, created a Plone site, installed the add-on, and added a user to the whitelist. If the only issue was that it didn’t work when the whitelist was empty, it should work as expected. So then I attempted to log in as a user not on the whitelist 3 times, each time using an incorrect password. Then I entered the correct information. Instead of notifying me that the account was locked, I am logged in as the user. I’ve repeated these steps in Plone 5.0.8 and got the same results. I have seen it lock users out, and I even saw a message about an expired password. However I don’t remember doing anything special to get it to work. It’s got to be something, because with a fresh install like what I’ve described, it doesn’t do anything.

csanahuja commented 6 years ago

Mmm maybe you tried with an admin user? The script skips it when the user has the Manager role

# If user is Manager, then ignore this and do not block
if user.has_role('Manager'):
    return
CorySanin commented 6 years ago

🤦‍♀️ That's exactly what it was. I figured the point of the whitelist was to manually whitelist admin accounts. I didn't know that was done automatically. Thank you!