Merged PR 31853: Always return SignInResult.Failed if updating Access…

…FailedCount fails

This addresses MSRC Case 7810.

Before this change, an attacker could leverage a race condition to exceed the MaxFailedAccessAttempts enforced by SignInManager. This change addresses the issue by updating SignInManager to always return the same SignInResult.Failed status when updating AccessFailedCount fails even if it was attempting to reset the count after an otherwise successful login.

As Levi Broderick  pointed out on the associated email thread,

> The point of account lockout policy isn't necessarily to prevent the attacker from sending lots of requests to the server. Rather, it's to prevent the attacker from using the server as an endless "is this password correct?" oracle. This is achieved by placing an upper a limit on the number of answers the oracle provides. The insight from today's meeting is that it's ok for the server to process a number of requests which exceeds this limit, as long as the oracle never provides more than the configured number of answers.

## Customer Impact

This addresses an MSRC case related to password stuffing.

## Regression?

- [ ] Yes
- [x] No

## Risk

- [ ] High
- [ ] Medium
- [x] Low

This only impacts login and 2fa attempts when updating the AccessFailedCount returns a failed IdentityResult. Failed IdentityResults when persisting other changes to the user store already causes numerous issues.

## Verification

- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A

.nuget/NuGet.targets

@@ -50,7 +50,7 @@
 
         <RequireConsentSwitch Condition=" $(RequireRestoreConsent) == 'true' ">-RequireConsent</RequireConsentSwitch>
         <!-- Commands -->
-        <RestoreCommand>$(NuGetCommand) install "$(PackagesConfig)" -source "$(PackageSources)"  $(RequireConsentSwitch) -solutionDir "$(SolutionDir) "</RestoreCommand>
+        <RestoreCommand>$(NuGetCommand) install "$(PackagesConfig)" -source "$(PackageSources)"  $(RequireConsentSwitch) -solutionDir $(SolutionDir)</RestoreCommand>
         <BuildCommand>$(NuGetCommand) pack "$(ProjectPath)" -p Configuration=$(Configuration) -o "$(PackageOutputDir)" -symbols</BuildCommand>
 
         <!-- We need to ensure packages are restored prior to assembly resolve -->

src/Microsoft.AspNet.Identity.Owin/SignInManager.cs

@@ -183,12 +183,26 @@ public virtual async Task<SignInStatus> TwoFactorSignInAsync(string provider, st
             if (await UserManager.VerifyTwoFactorTokenAsync(user.Id, provider, code).WithCurrentCulture())
             {
                 // When token is verified correctly, clear the access failed count used for lockout
-                await UserManager.ResetAccessFailedCountAsync(user.Id).WithCurrentCulture();
+                var resetLockoutResult = await UserManager.ResetAccessFailedCountAsync(user.Id).WithCurrentCulture();
+                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 SignInStatus.Failure;
+                }
+
                 await SignInAsync(user, isPersistent, rememberBrowser).WithCurrentCulture();
                 return SignInStatus.Success;
             }
             // If the token is incorrect, record the failure which also may cause the user to be locked out
-            await UserManager.AccessFailedAsync(user.Id).WithCurrentCulture();
+            var incrementLockoutResult = await UserManager.AccessFailedAsync(user.Id).WithCurrentCulture();
+            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 SignInStatus.Failure;
+            }
             return SignInStatus.Failure;
         }
 
@@ -259,14 +273,27 @@ public virtual async Task<SignInStatus> PasswordSignInAsync(string userName, str
             {
                 if (!await IsTwoFactorEnabled(user))
                 {
-                    await UserManager.ResetAccessFailedCountAsync(user.Id).WithCurrentCulture();
+                    var resetLockoutResult = await UserManager.ResetAccessFailedCountAsync(user.Id).WithCurrentCulture();
+                    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 SignInStatus.Failure;
+                    }
                 }
                 return await SignInOrTwoFactor(user, isPersistent).WithCurrentCulture();
             }
             if (shouldLockout)
             {
                 // If lockout is requested, increment access failed count which might lock out the user
-                await UserManager.AccessFailedAsync(user.Id).WithCurrentCulture();
+                var incrementLockoutResult = await UserManager.AccessFailedAsync(user.Id).WithCurrentCulture();
+                if (!incrementLockoutResult.Succeeded)
+                {
+                    // Return the same failure we do when resetting the lockout fails after a correct password.
+                    return SignInStatus.Failure;
+                }
+
                 if (await UserManager.IsLockedOutAsync(user.Id).WithCurrentCulture())
                 {
                     return SignInStatus.LockedOut;

test/Identity.Test/SignInManagerTest.cs

@@ -8,6 +8,7 @@
 using Microsoft.AspNet.Identity.Owin;
 using Microsoft.Owin;
 using Microsoft.Owin.Security.DataProtection;
+using Moq;
 using Xunit;
 using Xunit.Extensions;
 
@@ -33,6 +34,29 @@ public async Task SignInAsyncCookiePersistenceTest(bool isPersistent, bool remem
 
             Assert.Equal(isPersistent, owinContext.Authentication.AuthenticationResponseGrant.Properties.IsPersistent);
         }
-
+
+        [Fact]
+        public async Task PasswordSignInFailsWhenResetLockoutFails()
+        {
+            // Setup
+            var owinContext = new OwinContext();
+            await TestUtil.CreateManager(owinContext);
+            var manager = new Mock<UserManager<IdentityUser>>(Mock.Of<IUserStore<IdentityUser>>());
+            var user = new IdentityUser("SignInTest");
+            manager.Setup(m => m.FindByNameAsync(user.Id)).Returns(Task.FromResult(user)).Verifiable();
+            manager.Setup(m => m.IsLockedOutAsync(user.Id)).Returns(Task.FromResult(false)).Verifiable();
+            manager.Setup(m => m.CheckPasswordAsync(user, "[PLACEHOLDER]-1a")).Returns(Task.FromResult(true)).Verifiable();
+            manager.Setup(m => m.GetTwoFactorEnabledAsync(user.Id)).Returns(Task.FromResult(false)).Verifiable();
+            manager.Setup(m => m.ResetAccessFailedCountAsync(user.Id)).Returns(Task.FromResult(IdentityResult.Failed())).Verifiable();
+
+            var signInManager = new SignInManager<IdentityUser, string>(manager.Object, owinContext.Authentication);
+
+            // Act
+            var result = await signInManager.PasswordSignInAsync(user.Id, "[PLACEHOLDER]-1a", isPersistent: false, shouldLockout: false);
+
+            // Assert
+            Assert.Equal(SignInStatus.Failure, result);
+            manager.Verify();
+        }
     }
 }