AspNetIdentity icon indicating copy to clipboard operation
AspNetIdentity copied to clipboard

FailedAccessAttempts doesn't reset when TwoFactorEnabled and Browser Remembered

Open 3nth opened this issue 5 years ago • 2 comments

Using 2.2.1 but can see the issue in master

PasswordSignInAsync will reset it for non-2FA accounts: https://github.com/aspnet/AspNetIdentity/blob/4874623fb8cdaeeded92891af5017d8480fd014a/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs#L260

TwoFactorSignInAsync will reset it for 2FA accounts: https://github.com/aspnet/AspNetIdentity/blob/4874623fb8cdaeeded92891af5017d8480fd014a/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs#L167

But if the user has previously remembered the browser, TwoFactorSignInAsync never happens, so it never resets.

Now that I understand what is going on, we can address it by resetting as appropriate, but this behavior was not expected and leaves 2FA accounts that have been locked out before in a state such that any single successive password failure after the lockout has expired, will increment the count (which is still over the limit) and locks the account again.

Some thoughts on possibilities:

  1. Check TwoFactorBrowserRememberedAsync during PasswordSignInAsync and reset if remembered.
  2. Don't increment failed access attempts during PasswordSignInAsync on 2FA accounts, but have lockout only apply on the TwoFactorSignInAsync. Basically 2FA would only lockout with 2FA failures, but not password failures.
  3. Force a 2FA verification after a lockout has occurred/expired

3nth avatar Mar 07 '19 23:03 3nth

uh... or not. Sheesh. The count is reset during the lockout. whelp, back to figuring out what our problem is....

https://github.com/aspnet/AspNetIdentity/blob/4874623fb8cdaeeded92891af5017d8480fd014a/src/Microsoft.AspNet.Identity.Core/UserManager.cs#L1723

3nth avatar Mar 08 '19 00:03 3nth

Original issue not quite what I thought, but I do still note that the AccessFailedCount doesn't get reset after a successful login for the particular case of 2FA enabled and browser remembers.

So for a user who uses same browser every day and trusts it, the fails slowly accumulate over time and then one day it appears as if a single failure locks them out. The other paths reset on success, so it seems like this should too.

Adding a second clause here to check if TwoFactorEnabled and TwoFactorBrowserRememberedAsync would do it: https://github.com/aspnet/AspNetIdentity/blob/4874623fb8cdaeeded92891af5017d8480fd014a/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs#L260

I can PR that if you like it.

3nth avatar Mar 08 '19 01:03 3nth