diff --git a/.nuget/NuGet.targets b/.nuget/NuGet.targets index a3e3c4c..09304e3 100644 --- a/.nuget/NuGet.targets +++ b/.nuget/NuGet.targets @@ -50,7 +50,7 @@ -RequireConsent - $(NuGetCommand) install "$(PackagesConfig)" -source "$(PackageSources)" $(RequireConsentSwitch) -solutionDir "$(SolutionDir) " + $(NuGetCommand) install "$(PackagesConfig)" -source "$(PackageSources)" $(RequireConsentSwitch) -solutionDir $(SolutionDir) $(NuGetCommand) pack "$(ProjectPath)" -p Configuration=$(Configuration) -o "$(PackageOutputDir)" -symbols diff --git a/.nuget/nuget.exe b/.nuget/nuget.exe new file mode 100644 index 0000000..b5b8e07 Binary files /dev/null and b/.nuget/nuget.exe differ diff --git a/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs b/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs index af232ef..b8fe586 100644 --- a/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs +++ b/src/Microsoft.AspNet.Identity.Owin/SignInManager.cs @@ -183,12 +183,26 @@ public virtual async Task 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 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 index 03c815e..a0c4714 100644 --- 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>(Mock.Of>()); + 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(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(); + } } }