To prevent brute force attacks on Koha accounts, staff and opac, we need to implement an account lockout process to Koha. After a number of failed login attempts a users account would become locked. The user would then need to use the reset password functionality to send a reset token to their email account. After a successful password reset the lockout flag would be removed. The number of failed login attempts before lockout will be configurable using a system preference.
would be better to have a CAPTCHA after N login attempt/minute from a class C? i'm concerned about "horizontal" brute force attacks, like checking password "1234" against random usernames
(In reply to Francesco Rivetti from comment #1) > would be better to have a CAPTCHA after N login attempt/minute from a class > C? That will not be parted of this enhancement. > i'm concerned about "horizontal" brute force attacks, like checking password > "1234" against random usernames See also bug 18298.
Probably an Architecture patch instead of Patrons?
Created attachment 61630 [details] [review] Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts And of course deletedborrowers.login_attempts
Created attachment 61631 [details] [review] Bug 18314: DBIC schema changes
Created attachment 61632 [details] [review] Bug 18314: Account lockout To prevent brute force attacks on Koha accounts, staff and opac, we need to implement an account lockout process to Koha. After a number of failed login attempts a users account would become locked. The user would then need to use the reset password functionality to send a reset token to their email account. After a successful password reset the lockout flag would be removed. The number of failed login attempts before lockout is configurable using a new system preference 'FailedLoginAttempts'. How does it work? When a patron enter an invalid password, the borrowers.login_attempts value for this patron is incremented. When this value reach the value of the pref FailedLoginAttempts, the password comparison is not done and the authentication is rejected. This login_attempts field is reset when a patron correctly logs in. When the account is locked the patron has to reset his/her password using the OpacResetPassword feature or ask a staff member to generate a new password. If the pref is not set (0, or '') the feature is considered as disabled, but the failed login attempts are stored anyway. Test plan: 0/ Apply patch and execute the update DB entry 1/ Switch on the feature by setting FailedLoginAttempts to 3 2/ Use an invalid password to login at the staff or OPAC interface 3/ After the third consecutive failures, you will be asked to reset your password if OpacResetPassword is set, or contact a staff member 4/ Switch on OpacResetPassword and reset your password 5/ Confirm that you are able to login 6/ Play with the different combinations QA details: The trick happens in C4::Auth::checkpw, to make things clear I had to create a return value (note the awesome name: @return) and replace the 3 successives if statements with elsif. Indeed if one of the condition is reached, it will return inside the given block.
Created attachment 61633 [details] [review] Bug 18314: Resetting the password removes the lock When a password is changed (updated by a staff member or using the "recover password via email" feature, the counter/flag needs to be reset.
Created attachment 61634 [details] [review] Bug 18314: Add tests
So happy to see this. I have a number of other tasks to do, but if I can find the time, I'll test this.
Hi, patch and db update apply cleanly. I see no change in behaviour in staff client nor opac side. I set the FailedLoginAttempts to 3, but I was able to attempt logging in with the wrong password unlimited times. Also, tests fail: # Subtest: update_password 1..7 ok 1 - Koha::Patron->update_password should warn if the userid is already used by another patron ok 2 - Koha::Patron->update_password should not have updated the userid ok 3 - Koha::Patron->update_password should not have updated the userid # Looks like you planned 7 tests but ran 3. not ok 8 - update_password # Failed test 'update_password' # at t/db_dependent/Koha/Patrons.t line 183. The method update is not covered by tests!# Looks like your test exited with 255 just after 8. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 15/22 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patrons.t (Wstat: 65280 Tests: 8 Failed: 1) Failed test: 8 Non-zero exit status: 255 Parse errors: Bad plan. You planned 22 tests but ran 8. Files=1, Tests=8, 3 wallclock secs ( 0.03 usr 0.01 sys + 1.28 cusr 0.10 csys = 1.42 CPU) Result: FAIL
Aleisha, tests pass and everything works for me. Could you double check please? (restart plack and memcached, check dependency, execute updateDB).
Patch tested with a sandbox, by Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62453 [details] [review] Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts And of course deletedborrowers.login_attempts Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62454 [details] [review] Bug 18314: DBIC schema changes Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62455 [details] [review] Bug 18314: Account lockout To prevent brute force attacks on Koha accounts, staff and opac, we need to implement an account lockout process to Koha. After a number of failed login attempts a users account would become locked. The user would then need to use the reset password functionality to send a reset token to their email account. After a successful password reset the lockout flag would be removed. The number of failed login attempts before lockout is configurable using a new system preference 'FailedLoginAttempts'. How does it work? When a patron enter an invalid password, the borrowers.login_attempts value for this patron is incremented. When this value reach the value of the pref FailedLoginAttempts, the password comparison is not done and the authentication is rejected. This login_attempts field is reset when a patron correctly logs in. When the account is locked the patron has to reset his/her password using the OpacResetPassword feature or ask a staff member to generate a new password. If the pref is not set (0, or '') the feature is considered as disabled, but the failed login attempts are stored anyway. Test plan: 0/ Apply patch and execute the update DB entry 1/ Switch on the feature by setting FailedLoginAttempts to 3 2/ Use an invalid password to login at the staff or OPAC interface 3/ After the third consecutive failures, you will be asked to reset your password if OpacResetPassword is set, or contact a staff member 4/ Switch on OpacResetPassword and reset your password 5/ Confirm that you are able to login 6/ Play with the different combinations QA details: The trick happens in C4::Auth::checkpw, to make things clear I had to create a return value (note the awesome name: @return) and replace the 3 successives if statements with elsif. Indeed if one of the condition is reached, it will return inside the given block. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62456 [details] [review] Bug 18314: Resetting the password removes the lock When a password is changed (updated by a staff member or using the "recover password via email" feature, the counter/flag needs to be reset. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62457 [details] [review] Bug 18314: Add tests Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
I hit a problem here: 1 - Staff/opac fail a logon 2 - Check db login_attempts = 1 3 - Succeed a login 4 - Check db login_attempts = 1 It should reset to zero, but instead accumulates until password reset. I think it would also be good to add a link to the staff side if password reset enabled to point staff member to opac
Created attachment 62872 [details] [review] Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts And of course deletedborrowers.login_attempts Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62873 [details] [review] Bug 18314: Account lockout To prevent brute force attacks on Koha accounts, staff and opac, we need to implement an account lockout process to Koha. After a number of failed login attempts a users account would become locked. The user would then need to use the reset password functionality to send a reset token to their email account. After a successful password reset the lockout flag would be removed. The number of failed login attempts before lockout is configurable using a new system preference 'FailedLoginAttempts'. How does it work? When a patron enter an invalid password, the borrowers.login_attempts value for this patron is incremented. When this value reach the value of the pref FailedLoginAttempts, the password comparison is not done and the authentication is rejected. This login_attempts field is reset when a patron correctly logs in. When the account is locked the patron has to reset his/her password using the OpacResetPassword feature or ask a staff member to generate a new password. If the pref is not set (0, or '') the feature is considered as disabled, but the failed login attempts are stored anyway. Test plan: 0/ Apply patch and execute the update DB entry 1/ Switch on the feature by setting FailedLoginAttempts to 3 2/ Use an invalid password to login at the staff or OPAC interface 3/ After the third consecutive failures, you will be asked to reset your password if OpacResetPassword is set, or contact a staff member 4/ Switch on OpacResetPassword and reset your password 5/ Confirm that you are able to login 6/ Play with the different combinations QA details: The trick happens in C4::Auth::checkpw, to make things clear I had to create a return value (note the awesome name: @return) and replace the 3 successives if statements with elsif. Indeed if one of the condition is reached, it will return inside the given block. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62874 [details] [review] Bug 18314: Resetting the password removes the lock When a password is changed (updated by a staff member or using the "recover password via email" feature, the counter/flag needs to be reset. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62875 [details] [review] Bug 18314: Add tests Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 62876 [details] [review] Bug 18314: DBIC schema changes
Created attachment 62877 [details] [review] Bug 18314: Fix reset number of login attempts on login success
Created attachment 62878 [details] [review] Bug 18314: Add link to 'reset your password' from staff
(In reply to Nick Clemens from comment #18) > I hit a problem here: > 1 - Staff/opac fail a logon > 2 - Check db login_attempts = 1 > 3 - Succeed a login > 4 - Check db login_attempts = 1 > > It should reset to zero, but instead accumulates until password reset. Indeed that did not work! Sorry about that, I was sure I tested it. > I think it would also be good to add a link to the staff side if password > reset enabled to point staff member to opac Done!
Created attachment 62884 [details] [review] Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts And of course deletedborrowers.login_attempts Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62885 [details] [review] Bug 18314: Account lockout To prevent brute force attacks on Koha accounts, staff and opac, we need to implement an account lockout process to Koha. After a number of failed login attempts a users account would become locked. The user would then need to use the reset password functionality to send a reset token to their email account. After a successful password reset the lockout flag would be removed. The number of failed login attempts before lockout is configurable using a new system preference 'FailedLoginAttempts'. How does it work? When a patron enter an invalid password, the borrowers.login_attempts value for this patron is incremented. When this value reach the value of the pref FailedLoginAttempts, the password comparison is not done and the authentication is rejected. This login_attempts field is reset when a patron correctly logs in. When the account is locked the patron has to reset his/her password using the OpacResetPassword feature or ask a staff member to generate a new password. If the pref is not set (0, or '') the feature is considered as disabled, but the failed login attempts are stored anyway. Test plan: 0/ Apply patch and execute the update DB entry 1/ Switch on the feature by setting FailedLoginAttempts to 3 2/ Use an invalid password to login at the staff or OPAC interface 3/ After the third consecutive failures, you will be asked to reset your password if OpacResetPassword is set, or contact a staff member 4/ Switch on OpacResetPassword and reset your password 5/ Confirm that you are able to login 6/ Play with the different combinations QA details: The trick happens in C4::Auth::checkpw, to make things clear I had to create a return value (note the awesome name: @return) and replace the 3 successives if statements with elsif. Indeed if one of the condition is reached, it will return inside the given block. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62886 [details] [review] Bug 18314: Resetting the password removes the lock When a password is changed (updated by a staff member or using the "recover password via email" feature, the counter/flag needs to be reset. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62887 [details] [review] Bug 18314: Add tests Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62888 [details] [review] Bug 18314: DBIC schema changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62889 [details] [review] Bug 18314: Fix reset number of login attempts on login success Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62890 [details] [review] Bug 18314: Add link to 'reset your password' from staff Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 62891 [details] [review] Bug 18314 (QA Followup) Use OpacBaseURL for password reset link
Pushed to master for 17.05, thanks Jonathan, Nick!
Like! This won't get ported back to 16.11.x as it is an enhancement.
This report causes t/db/SIP/Message.t to fail (quite often) since testbuilder fills login_attempts with a random number. Depends on a pref too btw.