From 2f542105b8cc5f6f81b2e780036979769ccdf89d Mon Sep 17 00:00:00 2001 From: artem-dudarev Date: Fri, 20 Sep 2024 14:43:33 +0200 Subject: [PATCH 1/6] Make the login time the same for registered and unregistered users --- .../Api/AuthorizationController.cs | 8 +++ .../Controllers/Api/SecurityController.cs | 62 +++++++++++-------- .../Controllers/Api/SecurityStopwatch.cs | 45 ++++++++++++++ 3 files changed, 88 insertions(+), 27 deletions(-) create mode 100644 src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs diff --git a/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs b/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs index 4f0d5dbfd9..81b543d785 100644 --- a/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs +++ b/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs @@ -117,6 +117,9 @@ public async Task Exchange() if (openIdConnectRequest.IsPasswordGrantType()) { + // Measure the duration of successful sign in and delay the failed response to prevent timing attacks + var securityStopwatch = SecurityStopwatch.StartNew(nameof(AuthorizationController), nameof(Exchange), "Password"); + var user = await _userManager.FindByNameAsync(openIdConnectRequest.Username); // Allows signin to back office by either username (login) or email if IdentityOptions.User.RequireUniqueEmail is True. @@ -127,11 +130,13 @@ public async Task Exchange() if (user is null) { + await securityStopwatch.DelayAsync(); return BadRequest(SecurityErrorDescriber.LoginFailed()); } if (!_passwordLoginOptions.Enabled && !user.IsAdministrator) { + await securityStopwatch.DelayAsync(); return BadRequest(SecurityErrorDescriber.PasswordLoginDisabled()); } @@ -144,6 +149,7 @@ public async Task Exchange() var errors = await requestValidator.ValidateAsync(context); if (errors.Count > 0) { + await securityStopwatch.DelayAsync(); return BadRequest(errors.First()); } } @@ -156,6 +162,8 @@ public async Task Exchange() await SetLastLoginDate(user); await _eventPublisher.Publish(new UserLoginEvent(user)); + securityStopwatch.Stop(); + return SignIn(ticket.Principal, ticket.Properties, ticket.AuthenticationScheme); } diff --git a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs b/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs index 84dd80dc98..f2aa9e6b56 100644 --- a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs +++ b/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs @@ -97,41 +97,44 @@ public SecurityController( [AllowAnonymous] public async Task> Login([FromBody] LoginRequest request) { - var userName = request.UserName; + // Measure the duration of successful sign in and delay the failed response to prevent timing attacks + var securityStopwatch = SecurityStopwatch.StartNew(nameof(SecurityController), nameof(Login)); + + var user = await UserManager.FindByNameAsync(request.UserName); // Allows signin to back office by either username (login) or email if IdentityOptions.User.RequireUniqueEmail is True. - if (_identityOptions.User.RequireUniqueEmail) + if (user == null && _identityOptions.User.RequireUniqueEmail) { - var userByName = await UserManager.FindByNameAsync(userName); - - if (userByName == null) - { - var userByEmail = await UserManager.FindByEmailAsync(userName); - if (userByEmail != null) - { - userName = userByEmail.UserName; - } - } + user = await UserManager.FindByEmailAsync(request.UserName); } - var user = await UserManager.FindByNameAsync(userName); + if (user == null) + { + await securityStopwatch.DelayAsync(); + return Ok(SignInResult.Failed); + } await _eventPublisher.Publish(new BeforeUserLoginEvent(user)); - var loginResult = await _signInManager.PasswordSignInAsync(userName, request.Password, request.RememberMe, true); + var loginResult = await _signInManager.PasswordSignInAsync(user, request.Password, request.RememberMe, lockoutOnFailure: true); - if (loginResult.Succeeded) + if (!loginResult.Succeeded) { - await SetLastLoginDate(user); - await _eventPublisher.Publish(new UserLoginEvent(user)); + await securityStopwatch.DelayAsync(); + return Ok(loginResult); + } - //Do not allow login to admin customers and rejected users - if (await UserManager.IsInRoleAsync(user, PlatformConstants.Security.SystemRoles.Customer)) - { - return Ok(SignInResult.NotAllowed); - } + await SetLastLoginDate(user); + await _eventPublisher.Publish(new UserLoginEvent(user)); + + //Do not allow login to admin customers and rejected users + if (await UserManager.IsInRoleAsync(user, PlatformConstants.Security.SystemRoles.Customer)) + { + loginResult = SignInResult.NotAllowed; } + securityStopwatch.Stop(); + return Ok(loginResult); } @@ -611,8 +614,12 @@ public async Task> ValidatePasswordResetToken(string userId, [AllowAnonymous] public async Task RequestPasswordReset(string loginOrEmail) { + // Measure the duration of successful request and delay the failed response to prevent timing attacks + var securityStopwatch = SecurityStopwatch.StartNew(nameof(SecurityController), nameof(RequestPasswordReset)); + var user = await UserManager.FindByNameAsync(loginOrEmail); - if (user == null) + + if (user == null && _identityOptions.User.RequireUniqueEmail) { user = await UserManager.FindByEmailAsync(loginOrEmail); } @@ -620,16 +627,15 @@ public async Task RequestPasswordReset(string loginOrEmail) // Return 200 to prevent potential user name/email harvesting if (user == null) { + await securityStopwatch.DelayAsync(); return Ok(); } var nextRequestDate = user.LastPasswordChangeRequestDate + _passwordOptions.RepeatedResetPasswordTimeLimit; if (nextRequestDate != null && nextRequestDate > DateTime.UtcNow) { - return Ok(new - { - NextRequestAt = nextRequestDate, - }); + await securityStopwatch.DelayAsync(); + return Ok(new { NextRequestAt = nextRequestDate }); } //Do not permit rejected users and customers @@ -652,6 +658,8 @@ public async Task RequestPasswordReset(string loginOrEmail) await UserManager.UpdateAsync(user); } + securityStopwatch.Stop(); + return Ok(); } diff --git a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs b/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs new file mode 100644 index 0000000000..0b66d610ed --- /dev/null +++ b/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs @@ -0,0 +1,45 @@ +using System.Collections.Concurrent; +using System.Diagnostics; +using System.Threading.Tasks; + +namespace VirtoCommerce.Platform.Web.Controllers.Api; + +// This class allows to measure the duration of successful sign in and delay the failed response to prevent timing attacks. +public class SecurityStopwatch +{ + private static readonly ConcurrentDictionary _delaysByName = new(); + + public static SecurityStopwatch StartNew(params string[] nameParts) + { + return new SecurityStopwatch(string.Join(".", nameParts)); + } + + private bool _isRunning; + private readonly string _name; + private readonly Stopwatch _stopwatch; + private readonly Task _delayTask; + + private SecurityStopwatch(string name) + { + _isRunning = true; + _name = name; + _stopwatch = Stopwatch.StartNew(); + _delaysByName.TryAdd(name, 0L); + _delayTask = Task.Delay((int)_delaysByName[name]); + } + + public Task DelayAsync() + { + return _delayTask; + } + + public void Stop() + { + if (_isRunning) + { + _stopwatch.Stop(); + _delaysByName[_name] = _stopwatch.ElapsedMilliseconds; + _isRunning = false; + } + } +} From 8809ca04820eebdf4b4bafb46d94bd62e6546629 Mon Sep 17 00:00:00 2001 From: artem-dudarev Date: Fri, 20 Sep 2024 15:00:39 +0200 Subject: [PATCH 2/6] Fix unit tests --- .../Controllers/Api/SecurityControllerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/VirtoCommerce.Platform.Web.Tests/Controllers/Api/SecurityControllerTests.cs b/tests/VirtoCommerce.Platform.Web.Tests/Controllers/Api/SecurityControllerTests.cs index fc624f2c38..7f4a5e3dc2 100644 --- a/tests/VirtoCommerce.Platform.Web.Tests/Controllers/Api/SecurityControllerTests.cs +++ b/tests/VirtoCommerce.Platform.Web.Tests/Controllers/Api/SecurityControllerTests.cs @@ -141,7 +141,7 @@ public async Task Login_SignInSuccess() var user = _fixture.Create(); var request = _fixture.Create(); _signInManagerMock - .Setup(x => x.PasswordSignInAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Setup(x => x.PasswordSignInAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .ReturnsAsync(SignInResult.Success); _userManagerMock .Setup(x => x.FindByNameAsync(It.IsAny())) @@ -168,7 +168,7 @@ public async Task Login_UserRoleIsCustomer() var user = _fixture.Create(); var request = _fixture.Create(); _signInManagerMock - .Setup(x => x.PasswordSignInAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Setup(x => x.PasswordSignInAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .ReturnsAsync(SignInResult.Success); _userManagerMock .Setup(x => x.FindByNameAsync(It.IsAny())) From 2e13e98e3a828be8694074cba86dc8285ea751d3 Mon Sep 17 00:00:00 2001 From: artem-dudarev Date: Fri, 20 Sep 2024 17:03:10 +0200 Subject: [PATCH 3/6] Move SecurityStopwatch to VirtoCommerce.Platform.Core --- .../Security}/SecurityStopwatch.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/{VirtoCommerce.Platform.Web/Controllers/Api => VirtoCommerce.Platform.Core/Security}/SecurityStopwatch.cs (95%) diff --git a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs b/src/VirtoCommerce.Platform.Core/Security/SecurityStopwatch.cs similarity index 95% rename from src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs rename to src/VirtoCommerce.Platform.Core/Security/SecurityStopwatch.cs index 0b66d610ed..f5ce3ed287 100644 --- a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityStopwatch.cs +++ b/src/VirtoCommerce.Platform.Core/Security/SecurityStopwatch.cs @@ -2,7 +2,7 @@ using System.Diagnostics; using System.Threading.Tasks; -namespace VirtoCommerce.Platform.Web.Controllers.Api; +namespace VirtoCommerce.Platform.Core.Security; // This class allows to measure the duration of successful sign in and delay the failed response to prevent timing attacks. public class SecurityStopwatch From 746a39e11f2ce5f4b410a0e01abfe7e863ffa5aa Mon Sep 17 00:00:00 2001 From: artem-dudarev Date: Mon, 23 Sep 2024 12:37:59 +0200 Subject: [PATCH 4/6] Rename SecurityStopwatch to DelayedResponse --- ...ecurityStopwatch.cs => DelayedResponse.cs} | 27 +++++++++---------- .../Api/AuthorizationController.cs | 12 ++++----- .../Controllers/Api/SecurityController.cs | 20 +++++++------- 3 files changed, 29 insertions(+), 30 deletions(-) rename src/VirtoCommerce.Platform.Core/Security/{SecurityStopwatch.cs => DelayedResponse.cs} (58%) diff --git a/src/VirtoCommerce.Platform.Core/Security/SecurityStopwatch.cs b/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs similarity index 58% rename from src/VirtoCommerce.Platform.Core/Security/SecurityStopwatch.cs rename to src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs index f5ce3ed287..d51940d5d1 100644 --- a/src/VirtoCommerce.Platform.Core/Security/SecurityStopwatch.cs +++ b/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs @@ -4,42 +4,41 @@ namespace VirtoCommerce.Platform.Core.Security; -// This class allows to measure the duration of successful sign in and delay the failed response to prevent timing attacks. -public class SecurityStopwatch +// This class allows to measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks. +public class DelayedResponse { private static readonly ConcurrentDictionary _delaysByName = new(); - public static SecurityStopwatch StartNew(params string[] nameParts) - { - return new SecurityStopwatch(string.Join(".", nameParts)); - } - - private bool _isRunning; private readonly string _name; private readonly Stopwatch _stopwatch; private readonly Task _delayTask; - private SecurityStopwatch(string name) + public static DelayedResponse Create(params string[] nameParts) + { + return new DelayedResponse(string.Join(".", nameParts)); + } + + public DelayedResponse(string name) { - _isRunning = true; _name = name; _stopwatch = Stopwatch.StartNew(); _delaysByName.TryAdd(name, 0L); _delayTask = Task.Delay((int)_delaysByName[name]); } - public Task DelayAsync() + public Task FailAsync() { return _delayTask; } - public void Stop() + public Task SucceedAsync() { - if (_isRunning) + if (_stopwatch.IsRunning) { _stopwatch.Stop(); _delaysByName[_name] = _stopwatch.ElapsedMilliseconds; - _isRunning = false; } + + return Task.CompletedTask; } } diff --git a/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs b/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs index 81b543d785..6c1240c669 100644 --- a/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs +++ b/src/VirtoCommerce.Platform.Web/Controllers/Api/AuthorizationController.cs @@ -117,8 +117,8 @@ public async Task Exchange() if (openIdConnectRequest.IsPasswordGrantType()) { - // Measure the duration of successful sign in and delay the failed response to prevent timing attacks - var securityStopwatch = SecurityStopwatch.StartNew(nameof(AuthorizationController), nameof(Exchange), "Password"); + // Measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks + var delayedResponse = DelayedResponse.Create(nameof(AuthorizationController), nameof(Exchange), "Password"); var user = await _userManager.FindByNameAsync(openIdConnectRequest.Username); @@ -130,13 +130,13 @@ public async Task Exchange() if (user is null) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return BadRequest(SecurityErrorDescriber.LoginFailed()); } if (!_passwordLoginOptions.Enabled && !user.IsAdministrator) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return BadRequest(SecurityErrorDescriber.PasswordLoginDisabled()); } @@ -149,7 +149,7 @@ public async Task Exchange() var errors = await requestValidator.ValidateAsync(context); if (errors.Count > 0) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return BadRequest(errors.First()); } } @@ -162,7 +162,7 @@ public async Task Exchange() await SetLastLoginDate(user); await _eventPublisher.Publish(new UserLoginEvent(user)); - securityStopwatch.Stop(); + await delayedResponse.SucceedAsync(); return SignIn(ticket.Principal, ticket.Properties, ticket.AuthenticationScheme); } diff --git a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs b/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs index f2aa9e6b56..eb26c7a921 100644 --- a/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs +++ b/src/VirtoCommerce.Platform.Web/Controllers/Api/SecurityController.cs @@ -97,8 +97,8 @@ public SecurityController( [AllowAnonymous] public async Task> Login([FromBody] LoginRequest request) { - // Measure the duration of successful sign in and delay the failed response to prevent timing attacks - var securityStopwatch = SecurityStopwatch.StartNew(nameof(SecurityController), nameof(Login)); + // Measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks + var delayedResponse = DelayedResponse.Create(nameof(SecurityController), nameof(Login)); var user = await UserManager.FindByNameAsync(request.UserName); @@ -110,7 +110,7 @@ public async Task> Login([FromBody] LoginRequest requ if (user == null) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return Ok(SignInResult.Failed); } @@ -120,7 +120,7 @@ public async Task> Login([FromBody] LoginRequest requ if (!loginResult.Succeeded) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return Ok(loginResult); } @@ -133,7 +133,7 @@ public async Task> Login([FromBody] LoginRequest requ loginResult = SignInResult.NotAllowed; } - securityStopwatch.Stop(); + await delayedResponse.SucceedAsync(); return Ok(loginResult); } @@ -614,8 +614,8 @@ public async Task> ValidatePasswordResetToken(string userId, [AllowAnonymous] public async Task RequestPasswordReset(string loginOrEmail) { - // Measure the duration of successful request and delay the failed response to prevent timing attacks - var securityStopwatch = SecurityStopwatch.StartNew(nameof(SecurityController), nameof(RequestPasswordReset)); + // Measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks + var delayedResponse = DelayedResponse.Create(nameof(SecurityController), nameof(RequestPasswordReset)); var user = await UserManager.FindByNameAsync(loginOrEmail); @@ -627,14 +627,14 @@ public async Task RequestPasswordReset(string loginOrEmail) // Return 200 to prevent potential user name/email harvesting if (user == null) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return Ok(); } var nextRequestDate = user.LastPasswordChangeRequestDate + _passwordOptions.RepeatedResetPasswordTimeLimit; if (nextRequestDate != null && nextRequestDate > DateTime.UtcNow) { - await securityStopwatch.DelayAsync(); + await delayedResponse.FailAsync(); return Ok(new { NextRequestAt = nextRequestDate }); } @@ -658,7 +658,7 @@ public async Task RequestPasswordReset(string loginOrEmail) await UserManager.UpdateAsync(user); } - securityStopwatch.Stop(); + await delayedResponse.SucceedAsync(); return Ok(); } From a9a70e5bd7e7ae608f95f4f35fcf297cc0c8f36b Mon Sep 17 00:00:00 2001 From: artem-dudarev Date: Mon, 23 Sep 2024 12:46:07 +0200 Subject: [PATCH 5/6] Add minimal delay --- .../Security/DelayedResponse.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs b/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs index d51940d5d1..458c1f6f22 100644 --- a/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs +++ b/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Concurrent; using System.Diagnostics; using System.Threading.Tasks; @@ -7,11 +8,13 @@ namespace VirtoCommerce.Platform.Core.Security; // This class allows to measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks. public class DelayedResponse { - private static readonly ConcurrentDictionary _delaysByName = new(); + private const int _minDelay = 200; // milliseconds + private static readonly ConcurrentDictionary _delaysByName = new(); private readonly string _name; private readonly Stopwatch _stopwatch; - private readonly Task _delayTask; + private readonly Task _failedDelayTask; + private readonly Task _succeededDelayTask; public static DelayedResponse Create(params string[] nameParts) { @@ -22,13 +25,15 @@ public DelayedResponse(string name) { _name = name; _stopwatch = Stopwatch.StartNew(); - _delaysByName.TryAdd(name, 0L); - _delayTask = Task.Delay((int)_delaysByName[name]); + _delaysByName.TryAdd(name, 0); + var failedDelay = Math.Max(_minDelay, _delaysByName[name]); + _failedDelayTask = Task.Delay(failedDelay); + _succeededDelayTask = Task.Delay(_minDelay); } public Task FailAsync() { - return _delayTask; + return _failedDelayTask; } public Task SucceedAsync() @@ -36,9 +41,9 @@ public Task SucceedAsync() if (_stopwatch.IsRunning) { _stopwatch.Stop(); - _delaysByName[_name] = _stopwatch.ElapsedMilliseconds; + _delaysByName[_name] = (int)_stopwatch.ElapsedMilliseconds; } - return Task.CompletedTask; + return _succeededDelayTask; } } From c8ac62ab9665fcd2e7e934447d43d46e7ea89c18 Mon Sep 17 00:00:00 2001 From: artem-dudarev Date: Wed, 25 Sep 2024 15:09:43 +0200 Subject: [PATCH 6/6] Reduce minimal delay to 150 ms --- src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs b/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs index 458c1f6f22..f5e8cf958c 100644 --- a/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs +++ b/src/VirtoCommerce.Platform.Core/Security/DelayedResponse.cs @@ -8,7 +8,7 @@ namespace VirtoCommerce.Platform.Core.Security; // This class allows to measure the duration of a succeeded response and delay subsequent failed responses to prevent timing attacks. public class DelayedResponse { - private const int _minDelay = 200; // milliseconds + private const int _minDelay = 150; // milliseconds private static readonly ConcurrentDictionary _delaysByName = new(); private readonly string _name;