case 4710 - 2FA step requires the temp token from the password step
The 2FA step matched the user by temp token alone, and the token was left empty after a successful 2FA login (null right after turning 2FA on), so an empty or missing token skipped the password step (security review F5). Now: an empty or missing temp token is refused before the lookup; the token is cleared to null after success; after 5 wrong codes the token is cleared, so the password must be entered again (counted in memory per user). Changelog: general security notice for self-hosted upgrades, and the one behaviour change users can see. SECURITY-REVIEW.md: F5 fixed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,3 +1,4 @@
|
||||
using System.Collections.Concurrent;
|
||||
using System.Collections.Generic;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
@@ -34,6 +35,10 @@ namespace AyaNova.Api.Controllers
|
||||
private readonly ApiServerState serverState;
|
||||
private const int JWT_LIFETIME_DAYS = 5;
|
||||
|
||||
//case 4710: wrong 2fa codes allowed per password entry, counted per user (in memory, a restart only resets the count)
|
||||
private const int MAX_TFA_FAILURES = 5;
|
||||
private static readonly ConcurrentDictionary<long, int> TfaFailures = new ConcurrentDictionary<long, int>();
|
||||
|
||||
/// <summary>
|
||||
/// ctor
|
||||
/// </summary>
|
||||
@@ -103,6 +108,7 @@ namespace AyaNova.Api.Controllers
|
||||
//Generate a temporary token to identify and verify this is the same user
|
||||
u.TempToken = Hasher.GenerateSalt().Replace("=", "").Replace("+", "");
|
||||
await ct.SaveChangesAsync();
|
||||
TfaFailures.TryRemove(u.Id, out _);
|
||||
var UOpt = await ct.UserOptions.AsNoTracking().FirstAsync(z => z.UserId == u.Id);
|
||||
|
||||
List<string> TranslationKeysToFetch = new List<string> { "AuthTwoFactor", "AuthEnterPin", "AuthVerifyCode", "Cancel", "AuthPinInvalid" };
|
||||
@@ -167,7 +173,14 @@ namespace AyaNova.Api.Controllers
|
||||
return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
|
||||
}
|
||||
|
||||
//Match to temp token that would have been set by initial credentialed login for 2fa User
|
||||
//case 4710: the temp token is the only proof the password step was passed, so it must be present
|
||||
if (string.IsNullOrWhiteSpace(pin.TempToken))
|
||||
{
|
||||
await Task.Delay(AyaNova.Util.ServerBootConfig.FAILED_AUTH_DELAY);
|
||||
return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
|
||||
}
|
||||
|
||||
//Match to temp token that would have been set by initial credentialed login for 2fa User
|
||||
var user = await ct.User.Where(z => z.TempToken == pin.TempToken && z.Active == true && z.AllowLogin==true && z.TwoFactorEnabled == true).FirstOrDefaultAsync();
|
||||
|
||||
|
||||
@@ -184,14 +197,22 @@ namespace AyaNova.Api.Controllers
|
||||
var tfa = new TwoFactorAuth("AyaNova");
|
||||
if (!tfa.VerifyCode(user.TotpSecret, pin.Pin.Replace(" ", "").Trim()))
|
||||
{
|
||||
//case 4710: after too many wrong codes the password must be entered again
|
||||
if (TfaFailures.AddOrUpdate(user.Id, 1, (k, n) => n + 1) >= MAX_TFA_FAILURES)
|
||||
{
|
||||
TfaFailures.TryRemove(user.Id, out _);
|
||||
user.TempToken = null;
|
||||
await ct.SaveChangesAsync();
|
||||
}
|
||||
await Task.Delay(AyaNova.Util.ServerBootConfig.FAILED_AUTH_DELAY);
|
||||
return StatusCode(401, new ApiErrorResponse(ApiErrorCode.AUTHENTICATION_FAILED));
|
||||
}
|
||||
|
||||
//User is valid and authenticated
|
||||
//clear temp token
|
||||
user.TempToken = string.Empty;
|
||||
//clear temp token (case 4710: null, not empty, so no value is left to match)
|
||||
user.TempToken = null;
|
||||
await ct.SaveChangesAsync();
|
||||
TfaFailures.TryRemove(user.Id, out _);
|
||||
|
||||
return await ReturnUserCredsOnSuccessfulAuthentication(user);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user