From e7ab7e8738554b467acb73fa3df8165192b31cab Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 7 Aug 2026 17:00:12 -0700 Subject: [PATCH] feat(accounts): add a config-gated repair for mis-migrated passwords Accounts corrupted by the pre-fix SetPassword hold argon2(username + password) tagged with an algorithm whose phrase rule omits the username. Nothing in the hash distinguishes that from a forgotten password, so the only detection is to retry with the other phrase on a failed verify -- which doubles the cost of every failed login, and failed logins are the credential-stuffing surface. Hence accountSecurity.repairMigratedPasswords, default false: an operator turns it on for a migration window and off again. Repaired accounts are force-rehashed because their stored parameters may already be current, in which case NeedsRehash would decline and the account would verify once and stay broken. Co-Authored-By: Claude Opus 5 (1M context) --- .../Tests/Accounting/AccountPasswordTests.cs | 62 +++++++++++++++++++ Projects/UOContent/Accounting/Account.cs | 28 +++++++-- .../Accounting/Security/AccountSecurity.cs | 12 ++++ 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs index 42389cbef..5253d24de 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs @@ -73,4 +73,66 @@ public class AccountPasswordTests : IDisposable Assert.True(account.CheckPassword("hunter2")); Assert.Equal(afterFirst, account.Password); } + + /// + /// Builds an account whose stored hash is what the pre-fix SetPassword produced when a SHA2 + /// account was upgraded to Argon2: argon2(username + password), tagged Argon2, whose phrase + /// rule omits the username. Unrecoverable without the plaintext, hence the repair path. + /// + private static Account CreateMisMigratedAccount(string username) + { + AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + var account = new Account(username, Password); + account.Password = AccountSecurity.CurrentPasswordProtection + .EncryptPassword($"{username}{Password}"); + + return account; + } + + [Fact] + public void MisMigratedAccount_IsRejected_WhenRepairIsDisabled() + { + var account = CreateMisMigratedAccount("repair-off-user"); + AccountSecurity.RepairMigratedPasswords = false; + + Assert.False(account.CheckPassword(Password)); + } + + [Fact] + public void MisMigratedAccount_IsRepaired_WhenRepairIsEnabled() + { + var account = CreateMisMigratedAccount("repair-on-user"); + AccountSecurity.RepairMigratedPasswords = true; + + try + { + Assert.True(account.CheckPassword(Password)); + } + finally + { + AccountSecurity.RepairMigratedPasswords = false; + } + + // Repaired in place: it must now verify with the flag back off. + Assert.True(account.CheckPassword(Password)); + Assert.False(account.CheckPassword("wrong-password")); + } + + [Fact] + public void WrongPassword_IsStillRejected_WhenRepairIsEnabled() + { + AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + var account = new Account("repair-wrong-pass-user", Password); + AccountSecurity.RepairMigratedPasswords = true; + + try + { + Assert.False(account.CheckPassword("wrong-password")); + Assert.True(account.CheckPassword(Password)); + } + finally + { + AccountSecurity.RepairMigratedPasswords = false; + } + } } diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index a4246784f..d60f387d0 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -403,18 +403,36 @@ public partial class Account : IAccount, IComparable public bool CheckPassword(string plainPassword) { + var protection = AccountSecurity.GetPasswordProtection(_passwordAlgorithm); var phrase = UsesUsernamePhrase(_passwordAlgorithm) ? $"{_username}{plainPassword}" : plainPassword; + var forceRehash = false; - var ok = AccountSecurity.GetPasswordProtection(_passwordAlgorithm).ValidatePassword(Password, phrase); - if (!ok) + if (!protection.ValidatePassword(Password, phrase)) { - return false; + // A pre-fix SetPassword stored username + password under an algorithm whose phrase rule + // omits the username. Nothing in the hash distinguishes that from a forgotten password, + // so the only way to detect it is to try the other phrase -- which costs a second + // verify on every failed login. Off by default for exactly that reason. + if (!AccountSecurity.RepairMigratedPasswords || UsesUsernamePhrase(_passwordAlgorithm) || + !protection.ValidatePassword(Password, $"{_username}{plainPassword}")) + { + return false; + } + + logger.Warning( + "Account '{Username}' had a password mis-migrated by a pre-fix SetPassword; repairing it.", + _username + ); + + // The stored hash may already carry current parameters, so NeedsRehash would say no + // and the account would verify once and stay broken. + forceRehash = true; } // Rehash when either the algorithm or its cost parameters have moved on. The short-circuit // ordering is load-bearing: NeedsRehash must never be handed a hash produced by a different - // algorithm, and the first clause guarantees it is not. - if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm || + // algorithm, and the second clause guarantees it is not. + if (forceRehash || _passwordAlgorithm != AccountSecurity.CurrentAlgorithm || AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password)) { logger.Debug("Rehashing the password for account '{Username}'.", _username); diff --git a/Projects/UOContent/Accounting/Security/AccountSecurity.cs b/Projects/UOContent/Accounting/Security/AccountSecurity.cs index 9f7faeafc..c2c495c2f 100644 --- a/Projects/UOContent/Accounting/Security/AccountSecurity.cs +++ b/Projects/UOContent/Accounting/Security/AccountSecurity.cs @@ -35,6 +35,15 @@ public static class AccountSecurity { public static PasswordProtectionAlgorithm CurrentAlgorithm { get; set; } + /// + /// Enables a one-time repair for accounts whose password was corrupted by the pre-fix + /// SetPassword, which hashed username + password but tagged it with an algorithm whose phrase + /// rule omits the username. Off by default: the repair costs a second verify on every FAILED + /// login, and failed logins are the credential-stuffing surface. Turn it on for a migration + /// window, then off again. + /// + public static bool RepairMigratedPasswords { get; set; } + public static IPasswordProtection CurrentPasswordProtection => GetPasswordProtection(CurrentAlgorithm); public static void Configure() @@ -45,6 +54,9 @@ public static class AccountSecurity PasswordProtectionAlgorithm.Argon2 ); + RepairMigratedPasswords = + ServerConfiguration.GetOrUpdateSetting("accountSecurity.repairMigratedPasswords", false); + if (CurrentAlgorithm < PasswordProtectionAlgorithm.SHA2) { throw new Exception($"Security: {CurrentAlgorithm} is obsolete and not secure. Do not use it.");