U4-10975 - Failed Logon Attempts resets to 0 after maximum number of attempts

Created by Jeffrey Schoemaker 16 Feb 2018, 12:51:11 Updated by Ronald Barendse 16 Aug 2018, 11:48:42

Tags: PR Consider for sprint

Relates to: U4-10974

Subtask of: U4-11011

If you try to logon with an incorrect password the failedLogonAttempts is increased by 1. But if you reach the maximum number of attempts (default 5) the value is updated to 0 (and you get locked out). Also if you do this again, you will after attempt #5, be reset to zero again (but still locked out)

I would expected that the number should increase.

2 Attachments

Comments

Rebecca King 22 Feb 2018, 03:19:14

https://github.com/umbraco/Umbraco-CMS/pull/2462

We overrode the asp.net user Identity method that sets the failed attempts to zero automatically when a user exceeds the maximum failed attempts. The code now doesn't reset the failed attempts until the user account is unlocked. See here. https://github.com/umbraco/Umbraco-CMS/pull/2462/files#diff-b7c2bde4b2143bb7c59e47aeb906b20fR551

We called the overriden method from here: https://github.com/umbraco/Umbraco-CMS/pull/2462/files#diff-b7c2bde4b2143bb7c59e47aeb906b20fL512

We also modified the user.controller.js file in order to rebind the model so that when the user account is unlocked the view displays 0. https://github.com/umbraco/Umbraco-CMS/pull/2462/files#diff-bb9179041576ed46c71bb65229b7ed42R344

We tested this by creating a user and logging in with the incorrect password more than 5 times so that the account locked. Then logged in as an admin to unlock the account to ensure that the user failed attempts were set back to 0. We also tested that a correct password didn't increase the failed login attempts when the password was correct. We tested that a password reset by the user didn't effect the failed login attempt and that it did not unlock the account. We tested that a password change by an admin didn't unlock the account.


Ronald Barendse 14 Aug 2018, 15:05:20

@jeffrey.schoemaker@perplex.nl I guess the default ASP.NET Identity code has a good reason the reset the count to 0...

I can think of at least 2 reasons: to prevent having to update this value in the database after locking out the user and to prevent an integer overflow when more than 2147483647 failed logins have occurred ;-)

The ASP.NET Identity code doesn't say exactly why, but it IS explicitly mentioned here: https://github.com/aspnet/AspNetIdentity/blob/9c48993a446288032f9824633e6dae81257da06e/src/Microsoft.AspNet.Identity.Core/UserManager.cs#L1699-L1726

I would advise reverting back to the original code and hide the failed login attempts if a user is locked out (or change it to e.g. 'Failed login attempts: 5+').


Jeffrey Schoemaker 16 Aug 2018, 08:58:55

Hi @rgmbarendse, thanks for looking into it!

But I think their is one major difference between the Umbraco implementation and the default and that is "the user will be locked out for the next DefaultAccountLockoutTimeSpan" and Umbraco does not implement a lockout for a certain timespan. In other words if you get locked out, you will be locked out forever.

The reason it is resetted by default is that you're locked out for 10 minutes (or something like that) and then get a fresh start on login again.

So preferably Umbraco would implement the "Lockout for this timespan" (.NET default) or this counter should not be resetted I guess.

-- Jeffrey


Ronald Barendse 16 Aug 2018, 11:48:42

Hmm, yes that does indeed differ between the implementations.

But my points are still valid: the current Umbraco implementation now requires an additional database update for every failed login (to increase the count) and the integer overflow is not handled!

Does it matter how many failed logins occurred if the user is locked out anyway? Just knowing someone tried more than 5/n times would be enough... And if we're very strict on the 'grammar': if the user is locked out, but tried to login using valid credentials, is it a failed login attempt?

Implementing the lockout timespan would be a great addition (and if that's done, the counter DOES need to be reset). Either way, my advise would still be to revert back to resetting the count and updating the backend UI.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Easy

Category: Security

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.8.0, 7.8.1

Due in version: 7.12.0

Sprint:

Story Points:

Cycle: 10