fix(accounts): gate the password repair on a per-account tag

The repair retried a failed verify against username + plainPassword, and
nothing in a stored hash distinguishes "this is H(username + password)
because SetPassword was buggy" from "this password merely begins with the
username". With repairMigratedPasswords on, that is a cheap targeted attack
against every account on the shard:

  Account 'bob', real password 'bob123', stored correctly as argon2("bob123").
  Someone submits '123'. The primary verify against "123" fails. The repair
  verifies "bob" + "123" == "bob123" and succeeds, forceRehash rewrites the
  credential to argon2("123"), and the real owner can never log in again --
  their password has been silently replaced by the attacker's guess.

Passwords that start with the username are common enough that this is worth
doing at scale, and it is silent: the victim only finds out later.

Add a second gate on top of the shard-wide switch. The account must also
carry a RepairMigratedPassword tag, which an operator sets from the admin
gump (Account Details -> Tags -> Add Tag) for someone who has actually
reported being locked out. That reduces exposure from every account on the
shard to one an operator deliberately marked, and in the real workflow the
collision cannot arise at all: a colliding account still logs in fine, so
nobody would ever mark it. The tag is removed on a successful repair, so the
window closes for that account immediately.

Also repair the other corruption shape. The old guard short-circuited on
UsesUsernamePhrase(_passwordAlgorithm), so SHA1/SHA2 accounts were never
repaired -- but SetPassword also runs from the Account(string, string)
constructor before _passwordAlgorithm is assigned, so an account created
while CurrentAlgorithm was SHA1 or SHA2 holds H(bare password) tagged
SHA1/SHA2, whose rule adds the username. Those accounts could never log in
at all and the repair declined to rescue them. RepairPhrase now tries
whichever rule the stored algorithm does not use, covering both directions.
That direction has no truncation collision -- exploiting it would need the
attacker to already know the real password -- but it stays behind both gates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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;
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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));
     }
 }

Projects/UOContent/Accounting/Account.cs

@@ -386,6 +386,21 @@ public partial class Account : IAccount, IComparable<Account>
     private static bool UsesUsernamePhrase(PasswordProtectionAlgorithm algorithm) =>
         algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2;
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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<Account>
 
         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<Account>
                 _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;
         }