diff --git a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs index 5253d24de..7bd1213e8 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs @@ -10,14 +10,19 @@ public class AccountPasswordTests : IDisposable { private const string Password = "hunter2"; - // AccountSecurity.CurrentAlgorithm is process-wide static state, shared with every other - // class in the "Sequential UOContent Tests" collection. xUnit constructs/disposes this class - // once per test case, so capturing and restoring it here means every case -- current and any - // added later to this file -- starts from and leaves behind the ambient value, instead of - // bleeding whatever algorithm it last set into the rest of the collection. + // AccountSecurity.CurrentAlgorithm and AccountSecurity.RepairMigratedPasswords are process-wide + // static state, shared with every other class in the "Sequential UOContent Tests" collection. + // xUnit constructs/disposes this class once per test case, so capturing and restoring them here + // means every case -- current and any added later to this file -- starts from and leaves behind + // the ambient values, instead of bleeding whatever it last set into the rest of the collection. private readonly PasswordProtectionAlgorithm _originalAlgorithm = AccountSecurity.CurrentAlgorithm; + private readonly bool _originalRepairMigratedPasswords = AccountSecurity.RepairMigratedPasswords; - public void Dispose() => AccountSecurity.CurrentAlgorithm = _originalAlgorithm; + public void Dispose() + { + AccountSecurity.CurrentAlgorithm = _originalAlgorithm; + AccountSecurity.RepairMigratedPasswords = _originalRepairMigratedPasswords; + } [Theory] [InlineData(PasswordProtectionAlgorithm.SHA1)] @@ -89,50 +94,141 @@ public class AccountPasswordTests : IDisposable return account; } + /// + /// The other corruption shape: an account *created* while CurrentAlgorithm was SHA1 or SHA2. + /// The pre-fix SetPassword ran from the constructor before _passwordAlgorithm was assigned, so + /// it took the None branch and stored H(bare password) under an algorithm whose phrase rule + /// adds the username. Those accounts could never log in at all. + /// + private static Account CreateMisCreatedShaAccount(string username, PasswordProtectionAlgorithm algorithm) + { + AccountSecurity.CurrentAlgorithm = algorithm; + var account = new Account(username, Password); + account.Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword(Password); + + return account; + } + [Fact] public void MisMigratedAccount_IsRejected_WhenRepairIsDisabled() { var account = CreateMisMigratedAccount("repair-off-user"); + account.SetTag(Account.RepairPasswordTag, "yes"); AccountSecurity.RepairMigratedPasswords = false; Assert.False(account.CheckPassword(Password)); } + [Fact] + public void MisMigratedAccount_IsRejected_WhenTheAccountIsNotTagged() + { + var account = CreateMisMigratedAccount("repair-untagged-user"); + AccountSecurity.RepairMigratedPasswords = true; + + Assert.Null(account.GetTag(Account.RepairPasswordTag)); + Assert.False(account.CheckPassword(Password)); + } + [Fact] public void MisMigratedAccount_IsRepaired_WhenRepairIsEnabled() { var account = CreateMisMigratedAccount("repair-on-user"); + account.SetTag(Account.RepairPasswordTag, "yes"); AccountSecurity.RepairMigratedPasswords = true; - try - { - Assert.True(account.CheckPassword(Password)); - } - finally - { - AccountSecurity.RepairMigratedPasswords = false; - } + Assert.True(account.CheckPassword(Password)); + + 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")); } + [Theory] + [InlineData(PasswordProtectionAlgorithm.SHA1)] + [InlineData(PasswordProtectionAlgorithm.SHA2)] + public void MisCreatedShaAccount_IsRepaired_WhenBothGatesAreSet(PasswordProtectionAlgorithm algorithm) + { + var account = CreateMisCreatedShaAccount($"repair-created-{algorithm}-user", algorithm); + account.SetTag(Account.RepairPasswordTag, "yes"); + AccountSecurity.RepairMigratedPasswords = true; + + Assert.True(account.CheckPassword(Password)); + + AccountSecurity.RepairMigratedPasswords = false; + + // Repaired in place under the username-salted rule its algorithm actually uses. + Assert.True(account.CheckPassword(Password)); + Assert.False(account.CheckPassword("wrong-password")); + } + + [Theory] + [InlineData(PasswordProtectionAlgorithm.SHA1)] + [InlineData(PasswordProtectionAlgorithm.SHA2)] + public void MisCreatedShaAccount_IsRejected_WhenTheAccountIsNotTagged(PasswordProtectionAlgorithm algorithm) + { + var account = CreateMisCreatedShaAccount($"reject-created-{algorithm}-user", algorithm); + AccountSecurity.RepairMigratedPasswords = true; + + Assert.False(account.CheckPassword(Password)); + } + + [Fact] + public void RepairTag_IsClearedAfterASuccessfulRepair() + { + var account = CreateMisMigratedAccount("repair-one-shot-user"); + account.SetTag(Account.RepairPasswordTag, "yes"); + AccountSecurity.RepairMigratedPasswords = true; + + Assert.True(account.CheckPassword(Password)); + Assert.Null(account.GetTag(Account.RepairPasswordTag)); + + // The window is closed for this account: corrupting it again is no longer repairable, + // even with the shard-wide flag still on. + account.Password = AccountSecurity.CurrentPasswordProtection + .EncryptPassword($"repair-one-shot-user{Password}"); + + Assert.False(account.CheckPassword(Password)); + } + [Fact] public void WrongPassword_IsStillRejected_WhenRepairIsEnabled() { AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; var account = new Account("repair-wrong-pass-user", Password); + account.SetTag(Account.RepairPasswordTag, "yes"); AccountSecurity.RepairMigratedPasswords = true; - try - { - Assert.False(account.CheckPassword("wrong-password")); - Assert.True(account.CheckPassword(Password)); - } - finally - { - AccountSecurity.RepairMigratedPasswords = false; - } + Assert.False(account.CheckPassword("wrong-password")); + Assert.True(account.CheckPassword(Password)); + } + + /// + /// The repair cannot distinguish a mis-migrated hash from a password that merely begins with + /// the username, so a shard-wide repair window would let anyone log into such an account with + /// only the suffix -- and the rehash that follows would rewrite the stored credential down to + /// that suffix, locking the real owner out permanently. The per-account tag is what stops it: + /// with the flag on but the account untagged, the attack must fail and the hash must not move. + /// + [Fact] + public void TruncatedPassword_IsRejected_AndDoesNotRewriteTheHash() + { + const string username = "trunc-attack-user"; + const string realPassword = $"{username}123"; + + AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + + // Stored correctly: argon2("trunc-attack-user123"), no corruption anywhere. + var account = new Account(username, realPassword); + var storedBefore = account.Password; + + AccountSecurity.RepairMigratedPasswords = true; + + Assert.False(account.CheckPassword("123")); + Assert.Equal(storedBefore, account.Password); + + // And the owner is still able to log in afterwards. + Assert.True(account.CheckPassword(realPassword)); } } diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index d60f387d0..9c63a95a5 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -386,6 +386,21 @@ public partial class Account : IAccount, IComparable private static bool UsesUsernamePhrase(PasswordProtectionAlgorithm algorithm) => algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2; + /// + /// Account tag that opts a single account into the mis-migrated-password repair. Set it from the + /// admin gump (Account Details -> Tags -> Add Tag) on an account whose owner has actually + /// reported being locked out, and never on one that can still log in: the repair cannot tell a + /// mis-migrated hash from a password that merely begins with the username, so marking a working + /// account risks rewriting its credential down to whatever was submitted. Cleared automatically + /// once a repair succeeds. + /// + public const string RepairPasswordTag = "RepairMigratedPassword"; + + // The pre-fix SetPassword left the credential hashed under the *other* family's phrase rule, in + // both directions, so the repair tries whichever rule the stored algorithm does not use. + private string RepairPhrase(string plainPassword) => + UsesUsernamePhrase(_passwordAlgorithm) ? plainPassword : $"{_username}{plainPassword}"; + public void SetPassword(string plainPassword) { // The phrase must match how CheckPassword will rebuild it *after* the algorithm changes, @@ -409,12 +424,15 @@ public partial class Account : IAccount, IComparable if (!protection.ValidatePassword(Password, phrase)) { - // 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}")) + // Nothing in the stored hash distinguishes a mis-migrated credential from a forgotten + // password, and the retry is not free: it costs a second full verify, and failed logins + // are the credential-stuffing surface. Worse, it cannot tell "stored is + // H(username + password)" from "the password simply starts with the username" -- and + // repairing the latter would rewrite a correct credential down to the submitted suffix + // and lock the owner out for good. Hence two gates: a shard-wide switch, and a per-account + // tag an operator sets only for someone who has reported the lockout. + if (!AccountSecurity.RepairMigratedPasswords || GetTag(RepairPasswordTag) == null || + !protection.ValidatePassword(Password, RepairPhrase(plainPassword))) { return false; } @@ -424,8 +442,11 @@ public partial class Account : IAccount, IComparable _username ); - // The stored hash may already carry current parameters, so NeedsRehash would say no - // and the account would verify once and stay broken. + // One-shot: the window closes for this account as soon as it is repaired. + RemoveTag(RepairPasswordTag); + + // The stored hash may already carry current parameters, so NeedsRehash would decline and + // the account would verify once and stay broken. forceRehash = true; }