Merged PR 31116: [internal/release/7.0] Always return SignInResult.Failed if updating AccessFailedCount fails

Author: Stephen Halter

src/Identity/Core/src/SignInManager.cs

@@ -4,6 +4,7 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Security.Claims;
+using System.Text;
 using Microsoft.AspNetCore.Authentication;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Logging;
@@ -388,7 +389,14 @@ public virtual async Task<SignInResult> CheckPasswordSignInAsync(TUser user, str
             // Only reset the lockout when not in quirks mode if either TFA is not enabled or the client is remembered for TFA.
             if (alwaysLockout || !await IsTfaEnabled(user) || await IsTwoFactorClientRememberedAsync(user))
             {
-                await ResetLockout(user);
+                var resetLockoutResult = await ResetLockoutWithResult(user);
+                if (!resetLockoutResult.Succeeded)
+                {
+                    // ResetLockout got an unsuccessful result that could be caused by concurrency failures indicating an
+                    // attacker could be trying to bypass the MaxFailedAccessAttempts limit. Return the same failure we do
+                    // when failing to increment the lockout to avoid giving an attacker extra guesses at the password.
+                    return SignInResult.Failed;
+                }
             }
 
             return SignInResult.Success;
@@ -398,7 +406,13 @@ public virtual async Task<SignInResult> CheckPasswordSignInAsync(TUser user, str
         if (UserManager.SupportsUserLockout && lockoutOnFailure)
         {
             // If lockout is requested, increment access failed count which might lock out the user
-            await UserManager.AccessFailedAsync(user);
+            var incrementLockoutResult = await UserManager.AccessFailedAsync(user) ?? IdentityResult.Success;
+            if (!incrementLockoutResult.Succeeded)
+            {
+                // Return the same failure we do when resetting the lockout fails after a correct password.
+                return SignInResult.Failed;
+            }
+
             if (await UserManager.IsLockedOutAsync(user))
             {
                 return await LockedOut(user);
@@ -467,18 +481,23 @@ public virtual async Task<SignInResult> TwoFactorRecoveryCodeSignInAsync(string
         var result = await UserManager.RedeemTwoFactorRecoveryCodeAsync(user, recoveryCode);
         if (result.Succeeded)
         {
-            await DoTwoFactorSignInAsync(user, twoFactorInfo, isPersistent: false, rememberClient: false);
-            return SignInResult.Success;
+            return await DoTwoFactorSignInAsync(user, twoFactorInfo, isPersistent: false, rememberClient: false);
         }
 
         // We don't protect against brute force attacks since codes are expected to be random.
         return SignInResult.Failed;
     }
 
-    private async Task DoTwoFactorSignInAsync(TUser user, TwoFactorAuthenticationInfo twoFactorInfo, bool isPersistent, bool rememberClient)
+    private async Task<SignInResult> DoTwoFactorSignInAsync(TUser user, TwoFactorAuthenticationInfo twoFactorInfo, bool isPersistent, bool rememberClient)
     {
-        // When token is verified correctly, clear the access failed count used for lockout
-        await ResetLockout(user);
+        var resetLockoutResult = await ResetLockoutWithResult(user);
+        if (!resetLockoutResult.Succeeded)
+        {
+            // ResetLockout got an unsuccessful result that could be caused by concurrency failures indicating an
+            // attacker could be trying to bypass the MaxFailedAccessAttempts limit. Return the same failure we do
+            // when failing to increment the lockout to avoid giving an attacker extra guesses at the two factor code.
+            return SignInResult.Failed;
+        }
 
         var claims = new List<Claim>();
         claims.Add(new Claim("amr", "mfa"));
@@ -496,6 +515,7 @@ private async Task DoTwoFactorSignInAsync(TUser user, TwoFactorAuthenticationInf
             await RememberTwoFactorClientAsync(user);
         }
         await SignInWithClaimsAsync(user, isPersistent, claims);
+        return SignInResult.Success;
     }
 
     /// <summary>
@@ -528,13 +548,18 @@ public virtual async Task<SignInResult> TwoFactorAuthenticatorSignInAsync(string
 
         if (await UserManager.VerifyTwoFactorTokenAsync(user, Options.Tokens.AuthenticatorTokenProvider, code))
         {
-            await DoTwoFactorSignInAsync(user, twoFactorInfo, isPersistent, rememberClient);
-            return SignInResult.Success;
+            return await DoTwoFactorSignInAsync(user, twoFactorInfo, isPersistent, rememberClient);
         }
         // If the token is incorrect, record the failure which also may cause the user to be locked out
         if (UserManager.SupportsUserLockout)
         {
-            await UserManager.AccessFailedAsync(user);
+            var incrementLockoutResult = await UserManager.AccessFailedAsync(user) ?? IdentityResult.Success;
+            if (!incrementLockoutResult.Succeeded)
+            {
+                // Return the same failure we do when resetting the lockout fails after a correct two factor code.
+                // This is currently redundant, but it's here in case the code gets copied elsewhere.
+                return SignInResult.Failed;
+            }
         }
         return SignInResult.Failed;
     }
@@ -569,13 +594,18 @@ public virtual async Task<SignInResult> TwoFactorSignInAsync(string provider, st
         }
         if (await UserManager.VerifyTwoFactorTokenAsync(user, provider, code))
         {
-            await DoTwoFactorSignInAsync(user, twoFactorInfo, isPersistent, rememberClient);
-            return SignInResult.Success;
+            return await DoTwoFactorSignInAsync(user, twoFactorInfo, isPersistent, rememberClient);
         }
         // If the token is incorrect, record the failure which also may cause the user to be locked out
         if (UserManager.SupportsUserLockout)
         {
-            await UserManager.AccessFailedAsync(user);
+            var incrementLockoutResult = await UserManager.AccessFailedAsync(user) ?? IdentityResult.Success;
+            if (!incrementLockoutResult.Succeeded)
+            {
+                // Return the same failure we do when resetting the lockout fails after a correct two factor code.
+                // This is currently redundant, but it's here in case the code gets copied elsewhere.
+                return SignInResult.Failed;
+            }
         }
         return SignInResult.Failed;
     }
@@ -867,13 +897,77 @@ protected virtual Task<SignInResult> LockedOut(TUser user)
     /// </summary>
     /// <param name="user">The user</param>
     /// <returns>The <see cref="Task"/> that represents the asynchronous operation, containing the <see cref="IdentityResult"/> of the operation.</returns>
-    protected virtual Task ResetLockout(TUser user)
+    protected virtual async Task ResetLockout(TUser user)
     {
         if (UserManager.SupportsUserLockout)
         {
-            return UserManager.ResetAccessFailedCountAsync(user);
+            // The IdentityResult should not be null according to the annotations, but our own tests return null and I'm trying to limit breakages.
+            var result = await UserManager.ResetAccessFailedCountAsync(user) ?? IdentityResult.Success;
+
+            if (!result.Succeeded)
+            {
+                throw new IdentityResultException(result);
+            }
+        }
+    }
+
+    private async Task<IdentityResult> ResetLockoutWithResult(TUser user)
+    {
+        // Avoid relying on throwing an exception if we're not in a derived class.
+        if (GetType() == typeof(SignInManager<TUser>))
+        {
+            if (!UserManager.SupportsUserLockout)
+            {
+                return IdentityResult.Success;
+            }
+
+            return await UserManager.ResetAccessFailedCountAsync(user) ?? IdentityResult.Success;
+        }
+
+        try
+        {
+            var resetLockoutTask = ResetLockout(user);
+
+            if (resetLockoutTask is Task<IdentityResult> resultTask)
+            {
+                return await resultTask ?? IdentityResult.Success;
+            }
+
+            await resetLockoutTask;
+            return IdentityResult.Success;
+        }
+        catch (IdentityResultException ex)
+        {
+            return ex.IdentityResult;
+        }
+    }
+
+    private sealed class IdentityResultException : Exception
+    {
+        internal IdentityResultException(IdentityResult result) : base()
+        {
+            IdentityResult = result;
+        }
+
+        internal IdentityResult IdentityResult { get; set; }
+
+        public override string Message
+        {
+            get
+            {
+                var sb = new StringBuilder("ResetLockout failed.");
+
+                foreach (var error in IdentityResult.Errors)
+                {
+                    sb.AppendLine();
+                    sb.Append(error.Code);
+                    sb.Append(": ");
+                    sb.Append(error.Description);
+                }
+
+                return sb.ToString();
+            }
         }
-        return Task.CompletedTask;
     }
 
     internal sealed class TwoFactorAuthenticationInfo