Merge pull request #67 from aspnet/wtgodbe/MergeInternal

Merge internal commits
aspnet · Jun 12, 2024 · 4eed61d · 4eed61d
2 parents aef8505 + c74e422
commit 4eed61d
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 6 deletions.
diff --git a/.nuget/NuGet.targets b/.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 -->

diff --git a/.nuget/nuget.exe b/.nuget/nuget.exe
diff --git a/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs b/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;

diff --git a/test/Identity.Test/SignInManagerTest.cs b/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();
+        }
     }
 }