Skip to content

Commit 14fbd20

Browse files
authored
Merge commit from fork
* Backport user enumeration fix. * Supress warning on use of .NET 6 with TimeProvider dependency. * Replace TimeProvider with Stopwatch, as the former isn't tested against .NET 6.0 and generates warnings. * Remove full path details from exception when requesting a path outside of the physical file system's root. * Added randomness to login duration. * Ensured against negative duration.
1 parent 2d8b5e8 commit 14fbd20

File tree

6 files changed

+214
-29
lines changed

6 files changed

+214
-29
lines changed

Directory.Build.props

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,5 @@
4949
<PropertyGroup>
5050
<GitVersionBaseDirectory>$(MSBuildThisFileDirectory)</GitVersionBaseDirectory>
5151
</PropertyGroup>
52+
5253
</Project>

src/Umbraco.Core/Configuration/Models/SecuritySettings.cs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// See LICENSE for more details.
33

44
using System.ComponentModel;
5+
using System.ComponentModel.DataAnnotations;
56

67
namespace Umbraco.Cms.Core.Configuration.Models;
78

@@ -24,6 +25,8 @@ public class SecuritySettings
2425

2526
internal const int StaticMemberDefaultLockoutTimeInMinutes = 30 * 24 * 60;
2627
internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60;
28+
private const long StaticUserDefaultFailedLoginDurationInMilliseconds = 1000;
29+
private const long StaticUserMinimumFailedLoginDurationInMilliseconds = 250;
2730

2831
/// <summary>
2932
/// Gets or sets a value indicating whether to keep the user logged in.
@@ -109,4 +112,28 @@ public class SecuritySettings
109112
[Obsolete("Use ContentSettings.AllowEditFromInvariant instead")]
110113
[DefaultValue(StaticAllowEditInvariantFromNonDefault)]
111114
public bool AllowEditInvariantFromNonDefault { get; set; } = StaticAllowEditInvariantFromNonDefault;
115+
116+
/// <summary>
117+
/// Gets or sets the default duration (in milliseconds) of failed login attempts.
118+
/// </summary>
119+
/// <value>
120+
/// The default duration (in milliseconds) of failed login attempts.
121+
/// </value>
122+
/// <remarks>
123+
/// The user login endpoint ensures that failed login attempts take at least as long as the average successful login.
124+
/// However, if no successful logins have occurred, this value is used as the default duration.
125+
/// </remarks>
126+
[Range(0, long.MaxValue)]
127+
[DefaultValue(StaticUserDefaultFailedLoginDurationInMilliseconds)]
128+
public long UserDefaultFailedLoginDurationInMilliseconds { get; set; } = StaticUserDefaultFailedLoginDurationInMilliseconds;
129+
130+
/// <summary>
131+
/// Gets or sets the minimum duration (in milliseconds) of failed login attempts.
132+
/// </summary>
133+
/// <value>
134+
/// The minimum duration (in milliseconds) of failed login attempts.
135+
/// </value>
136+
[Range(0, long.MaxValue)]
137+
[DefaultValue(StaticUserMinimumFailedLoginDurationInMilliseconds)]
138+
public long UserMinimumFailedLoginDurationInMilliseconds { get; set; } = StaticUserMinimumFailedLoginDurationInMilliseconds;
112139
}

src/Umbraco.Core/IO/PhysicalFileSystem.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ public string GetFullPath(string path)
358358

359359
// nothing prevents us to reach the file, security-wise, yet it is outside
360360
// this filesystem's root - throw
361-
throw new UnauthorizedAccessException($"File original: [{originalPath}] full: [{path}] is outside this filesystem's root.");
361+
throw new UnauthorizedAccessException($"Requested path {originalPath} is outside this filesystem's root.");
362362
}
363363

364364
/// <summary>

src/Umbraco.Core/TimedScope.cs

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
using System.Diagnostics;
2+
3+
namespace Umbraco.Cms.Core;
4+
5+
/// <summary>
6+
/// Makes a code block timed (take at least a certain amount of time). This class cannot be inherited.
7+
/// </summary>
8+
public sealed class TimedScope : IDisposable, IAsyncDisposable
9+
{
10+
private readonly TimeSpan _duration;
11+
private readonly CancellationTokenSource _cancellationTokenSource;
12+
private readonly Stopwatch _stopwatch;
13+
14+
/// <summary>
15+
/// Gets the elapsed time.
16+
/// </summary>
17+
/// <value>
18+
/// The elapsed time.
19+
/// </value>
20+
public TimeSpan Elapsed => _stopwatch.Elapsed;
21+
22+
/// <summary>
23+
/// Gets the remaining time.
24+
/// </summary>
25+
/// <value>
26+
/// The remaining time.
27+
/// </value>
28+
public TimeSpan Remaining
29+
=> TryGetRemaining(out TimeSpan remaining) ? remaining : TimeSpan.Zero;
30+
31+
/// <summary>
32+
/// Initializes a new instance of the <see cref="TimedScope" /> class.
33+
/// </summary>
34+
/// <param name="millisecondsDuration">The number of milliseconds the scope should at least take.</param>
35+
public TimedScope(long millisecondsDuration)
36+
: this(TimeSpan.FromMilliseconds(millisecondsDuration))
37+
{ }
38+
39+
/// <summary>
40+
/// Initializes a new instance of the <see cref="TimedScope" /> class.
41+
/// </summary>
42+
/// <param name="millisecondsDuration">The number of milliseconds the scope should at least take.</param>
43+
/// <param name="cancellationToken">The cancellation token.</param>
44+
public TimedScope(long millisecondsDuration, CancellationToken cancellationToken)
45+
: this(TimeSpan.FromMilliseconds(millisecondsDuration), cancellationToken)
46+
{ }
47+
48+
/// <summary>
49+
/// Initializes a new instance of the <see cref="TimedScope"/> class.
50+
/// </summary>
51+
/// <param name="duration">The duration the scope should at least take.</param>
52+
public TimedScope(TimeSpan duration)
53+
: this(duration, new CancellationTokenSource())
54+
{ }
55+
56+
/// <summary>
57+
/// Initializes a new instance of the <see cref="TimedScope" /> class.
58+
/// </summary>
59+
/// <param name="duration">The duration the scope should at least take.</param>
60+
/// <param name="cancellationToken">The cancellation token.</param>
61+
public TimedScope(TimeSpan duration, CancellationToken cancellationToken)
62+
: this(duration, CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
63+
{ }
64+
65+
private TimedScope(TimeSpan duration, CancellationTokenSource cancellationTokenSource)
66+
{
67+
_duration = duration;
68+
_cancellationTokenSource = cancellationTokenSource;
69+
_stopwatch = new Stopwatch();
70+
_stopwatch.Start();
71+
}
72+
73+
/// <summary>
74+
/// Cancels the timed scope.
75+
/// </summary>
76+
public void Cancel()
77+
=> _cancellationTokenSource.Cancel();
78+
79+
/// <summary>
80+
/// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
81+
/// </summary>
82+
/// <remarks>
83+
/// This will block using <see cref="Thread.Sleep(TimeSpan)" /> until the remaining time has elapsed, if not cancelled.
84+
/// </remarks>
85+
public void Dispose()
86+
{
87+
if (_cancellationTokenSource.IsCancellationRequested is false &&
88+
TryGetRemaining(out TimeSpan remaining))
89+
{
90+
Thread.Sleep(remaining);
91+
}
92+
}
93+
94+
/// <summary>
95+
/// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources asynchronously.
96+
/// </summary>
97+
/// <returns>
98+
/// A task that represents the asynchronous dispose operation.
99+
/// </returns>
100+
/// <remarks>
101+
/// This will delay using <see cref="Task.Delay(TimeSpan, CancellationToken)" /> until the remaining time has elapsed, if not cancelled.
102+
/// </remarks>
103+
public async ValueTask DisposeAsync()
104+
{
105+
if (_cancellationTokenSource.IsCancellationRequested is false &&
106+
TryGetRemaining(out TimeSpan remaining))
107+
{
108+
await Task.Delay(remaining, _cancellationTokenSource.Token).ConfigureAwait(false);
109+
}
110+
}
111+
112+
private bool TryGetRemaining(out TimeSpan remaining)
113+
{
114+
remaining = _duration.Subtract(Elapsed);
115+
116+
return remaining > TimeSpan.Zero;
117+
}
118+
}

src/Umbraco.Core/Umbraco.Core.csproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<Project Sdk="Microsoft.NET.Sdk">
1+
<Project Sdk="Microsoft.NET.Sdk">
22
<PropertyGroup>
33
<PackageId>Umbraco.Cms.Core</PackageId>
44
<Title>Umbraco CMS - Core</Title>

src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs

Lines changed: 66 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ public class AuthenticationController : UmbracoApiControllerBase
7474
private readonly IUserService _userService;
7575
private readonly WebRoutingSettings _webRoutingSettings;
7676

77+
private const int FailedLoginDurationRandomOffsetInMilliseconds = 100;
78+
private static long? _loginDurationAverage;
79+
7780
// TODO: We need to review all _userManager.Raise calls since many/most should be on the usermanager or signinmanager, very few should be here
7881
[ActivatorUtilitiesConstructor]
7982
public AuthenticationController(
@@ -342,47 +345,83 @@ public async Task<bool> IsAuthenticated()
342345
[Authorize(Policy = AuthorizationPolicies.DenyLocalLoginIfConfigured)]
343346
public async Task<ActionResult<UserDetail?>> PostLogin(LoginModel loginModel)
344347
{
348+
// Start a timed scope to ensure failed responses return is a consistent time
349+
await using var timedScope = new TimedScope(GetLoginDuration(), CancellationToken.None);
350+
345351
// Sign the user in with username/password, this also gives a chance for developers to
346352
// custom verify the credentials and auto-link user accounts with a custom IBackOfficePasswordChecker
347353
SignInResult result = await _signInManager.PasswordSignInAsync(
348354
loginModel.Username, loginModel.Password, true, true);
349355

350-
if (result.Succeeded)
356+
if (result.Succeeded is false)
351357
{
352-
// return the user detail
353-
return GetUserDetail(_userService.GetByUsername(loginModel.Username));
354-
}
358+
BackOfficeIdentityUser? user = await _userManager.FindByNameAsync(loginModel.Username.Trim());
355359

356-
if (result.RequiresTwoFactor)
357-
{
358-
var twofactorView = _backOfficeTwoFactorOptions.GetTwoFactorView(loginModel.Username);
359-
if (twofactorView.IsNullOrWhiteSpace())
360+
if (user is not null &&
361+
await _userManager.CheckPasswordAsync(user, loginModel.Password))
360362
{
361-
return new ValidationErrorResult(
362-
$"The registered {typeof(IBackOfficeTwoFactorOptions)} of type {_backOfficeTwoFactorOptions.GetType()} did not return a view for two factor auth ");
363-
}
364-
365-
IUser? attemptedUser = _userService.GetByUsername(loginModel.Username);
363+
// The credentials were correct, so cancel timed scope and provide a more detailed failure response
364+
timedScope.Cancel();
366365

367-
// create a with information to display a custom two factor send code view
368-
var verifyResponse =
369-
new ObjectResult(new { twoFactorView = twofactorView, userId = attemptedUser?.Id })
366+
if (result.RequiresTwoFactor)
370367
{
371-
StatusCode = StatusCodes.Status402PaymentRequired
372-
};
368+
var twofactorView = _backOfficeTwoFactorOptions.GetTwoFactorView(loginModel.Username);
369+
if (twofactorView.IsNullOrWhiteSpace())
370+
{
371+
return new ValidationErrorResult(
372+
$"The registered {typeof(IBackOfficeTwoFactorOptions)} of type {_backOfficeTwoFactorOptions.GetType()} did not return a view for two factor auth ");
373+
}
374+
375+
IUser? attemptedUser = _userService.GetByUsername(loginModel.Username);
376+
377+
// create a with information to display a custom two factor send code view
378+
var verifyResponse =
379+
new ObjectResult(new { twoFactorView = twofactorView, userId = attemptedUser?.Id })
380+
{
381+
StatusCode = StatusCodes.Status402PaymentRequired
382+
};
383+
384+
return verifyResponse;
385+
}
386+
387+
// TODO: We can check for these and respond differently if we think it's important
388+
// result.IsLockedOut
389+
// result.IsNotAllowed
390+
}
373391

374-
return verifyResponse;
392+
// Return BadRequest (400), we don't want to return a 401 because that get's intercepted
393+
// by our angular helper because it thinks that we need to re-perform the request once we are
394+
// authorized and we don't want to return a 403 because angular will show a warning message indicating
395+
// that the user doesn't have access to perform this function, we just want to return a normal invalid message.
396+
return BadRequest();
375397
}
376398

377-
// TODO: We can check for these and respond differently if we think it's important
378-
// result.IsLockedOut
379-
// result.IsNotAllowed
399+
// Set initial or update average (successful) login duration
400+
_loginDurationAverage = _loginDurationAverage is long average
401+
? (average + (long)timedScope.Elapsed.TotalMilliseconds) / 2
402+
: (long)timedScope.Elapsed.TotalMilliseconds;
403+
404+
// Cancel the timed scope (we don't want to unnecessarily wait on a successful response)
405+
timedScope.Cancel();
406+
407+
// Return the user detail
408+
return GetUserDetail(_userService.GetByUsername(loginModel.Username));
409+
}
410+
411+
private long GetLoginDuration()
412+
{
413+
var loginDuration = Math.Max(_loginDurationAverage ?? _securitySettings.UserDefaultFailedLoginDurationInMilliseconds, _securitySettings.UserMinimumFailedLoginDurationInMilliseconds);
414+
var random = new Random();
415+
var randomDelay = random.Next(-FailedLoginDurationRandomOffsetInMilliseconds, FailedLoginDurationRandomOffsetInMilliseconds);
416+
loginDuration += randomDelay;
417+
418+
// Just be sure we don't get a negative number - possible if someone has configured a very low UserMinimumFailedLoginDurationInMilliseconds value.
419+
if (loginDuration < 0)
420+
{
421+
loginDuration = 0;
422+
}
380423

381-
// return BadRequest (400), we don't want to return a 401 because that get's intercepted
382-
// by our angular helper because it thinks that we need to re-perform the request once we are
383-
// authorized and we don't want to return a 403 because angular will show a warning message indicating
384-
// that the user doesn't have access to perform this function, we just want to return a normal invalid message.
385-
return BadRequest();
424+
return loginDuration;
386425
}
387426

388427
/// <summary>

0 commit comments

Comments
 (0)