refactor(accounts): make every password protection thread safe, drop the Argon2 carve-out

The worker was Argon2-only and kept its own protection instance. Both are now
unnecessary, but not for the reason the code gave.

CreateIsolated() was justified by the RNG, which was wrong. Argon2's Verify
is static-backed and stackalloc throughout, and the salt RNG is a stateless
syscall wrapper -- neither has state to race over. The real blocker was
HashAlgorithmPasswordProtection, which retains a HashAlgorithm carrying the
running digest across HashCore/HashFinal, shared through process-wide
singletons. Two threads there corrupt each other.

That is fixed at the source: hashing now goes through the one-shot static
APIs, which have no such state, allocate nothing, and produce identical
bytes. Literal digests are pinned in a test first, because these are compared
as strings against every account database -- any drift would lock out every
SHA and MD5 account at once.

PBKDF2 drew its iteration count from Utility.RandomMinMax, a shared
System.Random that is both thread-unsafe and game state. It now uses the
cryptographic RNG, matching the salt beside it.

With all three safe, the worker no longer needs to know which algorithm it is
running, and the dispatch conditions collapse to "is off-loop available". A
cheap digest now pays a thread hop it does not need, which costs login
latency we have already decided not to care about, and saves loop time we do.

Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs

@@ -26,6 +26,7 @@ public class PasswordWorkerTests : IDisposable
             StoredHash = account.Password,
             VerifyPhrase = account.GetVerifyPhrase(submitted),
             HashPhrase = account.NeedsPasswordUpgrade() ? account.GetRehashPhrase(submitted) : null,
+            StoredAlgorithm = account.PasswordAlgorithm,
             TargetAlgorithm = AccountSecurity.CurrentAlgorithm
         };
 

Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs

@@ -75,6 +75,32 @@ public class PasswordProtectionTest
         Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password"));
     }
 
+    /// <summary>
+    /// Literal digests of <see cref="plainPassword"/>, so the stored format cannot drift. These are
+    /// compared as strings against what is already in every account database -- a casing or encoding
+    /// change would lock out every SHA and MD5 account on the shard at once.
+    /// </summary>
+    [Theory]
+    [InlineData("MD5", "52284053181040AC90DBDE74A0E7FF5E")]
+    [InlineData("SHA1", "9AC635509803AAE2D8312BA1879289259A50C5F0")]
+    [InlineData(
+        "SHA2",
+        "5A727BFF8F8E08A24BDF6B0CD5065F30A1F8E0060B857BB8AFD6955BE0ACBC489DA63F19B8F4CF08D73DE4069CF4B" +
+        "29D94B353F31513B2FB2D9382EFE15AE975"
+    )]
+    public void HashAlgorithm_StoredFormatIsStable(string algorithmType, string expected)
+    {
+        var protection = algorithmType switch
+        {
+            "SHA1" => HashAlgorithmPasswordProtection.SHA1Instance,
+            "SHA2" => HashAlgorithmPasswordProtection.SHA2Instance,
+            _      => HashAlgorithmPasswordProtection.MD5Instance,
+        };
+
+        Assert.Equal(expected, protection.EncryptPassword(plainPassword));
+        Assert.True(protection.ValidatePassword(expected, plainPassword));
+    }
+
     // The shipping default before this change, as a literal so it cannot drift with the configured
     // defaults. Password: "hunter2".
     private const string LegacyArgon2iHash =

Projects/UOContent/Accounting/AccountHandler.cs

@@ -397,18 +397,13 @@ public static class AccountHandler
     }
 
     /// <summary>
-    /// Hands an Argon2 verify to the verification thread.
-    ///
-    /// Argon2-stored accounts only: SHA/MD5 protections share a <c>HashAlgorithm</c> whose
-    /// <c>ComputeHash</c> is not thread safe, and cost microseconds anyway. Their one-time rehash
-    /// into Argon2 also stays here, costing a migrating account one 8.9 ms login exactly as today --
-    /// moving it would mean waiting on an upgrade that does not gate the verdict.
+    /// Hands the password check to the worker, whatever algorithm it uses. Every protection is safe
+    /// to run off the loop, so there is no carve-out; a cheap digest pays a thread hop it does not
+    /// need, but login latency is not what this is protecting.
     /// </summary>
     private static PasswordCheckDispatch DispatchPasswordCheck(AccountLoginEventArgs e, Account acct, string pw)
     {
-        if (!PasswordWorker.Enabled ||
-            AccountSecurity.CurrentAlgorithm != PasswordProtectionAlgorithm.Argon2 ||
-            acct.PasswordAlgorithm != PasswordProtectionAlgorithm.Argon2)
+        if (!PasswordWorker.Enabled)
         {
             return PasswordCheckDispatch.Inline;
         }
@@ -418,6 +413,7 @@ public static class AccountHandler
             Account = acct,
             State = e.State,
             StoredHash = acct.Password,
+            StoredAlgorithm = acct.PasswordAlgorithm,
             VerifyPhrase = acct.GetVerifyPhrase(pw),
             HashPhrase = acct.NeedsPasswordUpgrade() ? acct.GetRehashPhrase(pw) : null,
             TargetAlgorithm = AccountSecurity.CurrentAlgorithm,

Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs

@@ -21,14 +21,6 @@ public class Argon2PasswordProtection : IPasswordProtection
 {
     public static IPasswordProtection Instance = new Argon2PasswordProtection();
 
-    /// <summary>
-    /// An instance sharing no state with <see cref="Instance"/>. Verification is static-backed and
-    /// safe to call from anywhere, but hashing draws its salt from a per-instance
-    /// <see cref="RandomNumberGenerator"/>, so a thread that hashes off the game loop takes its own
-    /// rather than racing the loop for that one field.
-    /// </summary>
-    public static IPasswordProtection CreateIsolated() => new Argon2PasswordProtection();
-
     // 16 MiB at t=1 is cheaper than 8 MiB at t=3 (8.5 ms vs 10.1 ms) and twice as memory-hard, which
     // is what resists GPU and ASIC cracking. p=1: native argon2 spawns a thread per lane.
     private readonly Argon2PasswordHasher _passwordHasher = new(

Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs

@@ -19,19 +19,47 @@ using Server.Text;
 
 namespace Server.Accounting.Security;
 
+/// <summary>
+/// The obsolete unsalted digests, kept only so imported accounts can log in once and be upgraded.
+///
+/// Hashing goes through the one-shot static APIs rather than a retained <see cref="HashAlgorithm"/>.
+/// A <see cref="HashAlgorithm"/> instance carries the running digest across HashCore/HashFinal, so
+/// two threads sharing one corrupt each other's result -- and these are process-wide singletons.
+/// The static form has no such state, allocates nothing, and produces identical bytes.
+/// </summary>
 public class HashAlgorithmPasswordProtection : IPasswordProtection
 {
-    public static IPasswordProtection MD5Instance = new HashAlgorithmPasswordProtection(MD5.Create());
-    public static IPasswordProtection SHA1Instance = new HashAlgorithmPasswordProtection(SHA1.Create());
-    public static IPasswordProtection SHA2Instance = new HashAlgorithmPasswordProtection(SHA512.Create());
-    private readonly HashAlgorithm _hashAlgorithm;
+    private enum Kind
+    {
+        MD5,
+        SHA1,
+        SHA512
+    }
 
-    public HashAlgorithmPasswordProtection(HashAlgorithm hashAlgorithm) => _hashAlgorithm = hashAlgorithm;
+    public static readonly IPasswordProtection MD5Instance = new HashAlgorithmPasswordProtection(Kind.MD5);
+    public static readonly IPasswordProtection SHA1Instance = new HashAlgorithmPasswordProtection(Kind.SHA1);
+    public static readonly IPasswordProtection SHA2Instance = new HashAlgorithmPasswordProtection(Kind.SHA512);
+
+    private const int MaxDigestLength = 64; // SHA512, the largest of the three.
+
+    private readonly Kind _kind;
+
+    private HashAlgorithmPasswordProtection(Kind kind) => _kind = kind;
 
     public string EncryptPassword(string plainPassword)
     {
         var bytes = plainPassword.AsSpan(0, Math.Min(256, plainPassword.Length)).GetBytesAscii();
-        return _hashAlgorithm.ComputeHash(bytes).ToHexString();
+
+        Span<byte> digest = stackalloc byte[MaxDigestLength];
+
+        var written = _kind switch
+        {
+            Kind.MD5  => MD5.HashData(bytes, digest),
+            Kind.SHA1 => SHA1.HashData(bytes, digest),
+            _         => SHA512.HashData(bytes, digest)
+        };
+
+        return digest[..written].ToHexString();
     }
 
     public bool ValidatePassword(string encryptedPassword, string plainPassword) =>

Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs

@@ -32,7 +32,10 @@ public class PBKDF2PasswordProtection : IPasswordProtection
     public string EncryptPassword(string plainPassword)
     {
         Span<byte> output = stackalloc byte[m_OutputSize];
-        var iterations = Utility.RandomMinMax(m_MinIterations, m_MaxIterations);
+
+        // The cryptographic RNG, not Utility's. The game RNG is a shared System.Random -- unsafe to
+        // touch from another thread, and game state besides.
+        var iterations = RandomNumberGenerator.GetInt32(m_MinIterations, m_MaxIterations + 1);
         BinaryPrimitives.WriteUInt16LittleEndian(output[..2], (ushort)iterations);
 
         var salt = output.Slice(2, m_SaltSize);

Projects/UOContent/Accounting/Security/PasswordWorker.cs

@@ -39,6 +39,10 @@ internal sealed class PasswordJob
     /// <summary>Hash to verify against, with <see cref="VerifyPhrase"/>.</summary>
     public string StoredHash;
 
+    /// <summary>Algorithm <see cref="StoredHash"/> was written with. Resolved on the loop, because
+    /// AccountSecurity.CurrentAlgorithm is mutable state the worker must not read.</summary>
+    public PasswordProtectionAlgorithm StoredAlgorithm;
+
     /// <summary>Phrase to verify, or null to skip verification.</summary>
     public string VerifyPhrase;
 
@@ -108,10 +112,6 @@ internal sealed class PasswordWorker
     private readonly AutoResetEvent _work = new(false);
     private readonly ConcurrentQueue<PasswordJob> _queue = new();
 
-    // Its own Argon2: verification is static-backed and safe to share, hashing draws from a
-    // per-instance RNG and is not.
-    private readonly IPasswordProtection _argon2 = Argon2PasswordProtection.CreateIsolated();
-
     private int _pending;
     private volatile bool _exit;
 
@@ -207,16 +207,20 @@ internal sealed class PasswordWorker
         }
     }
 
-    private PasswordOutcome Compute(PasswordJob job)
+    private static PasswordOutcome Compute(PasswordJob job)
     {
-        if (job.VerifyPhrase != null && !_argon2.ValidatePassword(job.StoredHash, job.VerifyPhrase))
+        if (job.VerifyPhrase != null &&
+            !AccountSecurity.GetPasswordProtection(job.StoredAlgorithm)
+                .ValidatePassword(job.StoredHash, job.VerifyPhrase))
         {
             return new PasswordOutcome(false, null);
         }
 
         return new PasswordOutcome(
             true,
-            job.HashPhrase == null ? null : _argon2.EncryptPassword(job.HashPhrase)
+            job.HashPhrase == null
+                ? null
+                : AccountSecurity.GetPasswordProtection(job.TargetAlgorithm).EncryptPassword(job.HashPhrase)
         );
     }
 
@@ -247,7 +251,7 @@ internal sealed class PasswordWorker
     /// </summary>
     internal static void SetPassword(Account account, string plainPassword, Action<bool> onDone)
     {
-        if (!Enabled || AccountSecurity.CurrentAlgorithm != PasswordProtectionAlgorithm.Argon2)
+        if (!Enabled)
         {
             account.SetPassword(plainPassword);
             onDone?.Invoke(true);
@@ -272,8 +276,7 @@ internal sealed class PasswordWorker
     }
 
     /// <summary>Runs a job on the calling thread. The seam the tests drive.</summary>
-    internal static PasswordOutcome ComputeInline(PasswordJob job) =>
-        Instance.Compute(job);
+    internal static PasswordOutcome ComputeInline(PasswordJob job) => Compute(job);
 
     /// <summary>
     /// Normal shutdown. The loop has stopped but this runs on the game thread, so pending work can
@@ -305,7 +308,7 @@ internal sealed class PasswordWorker
                 continue;
             }
 
-            var outcome = instance.Compute(job);
+            var outcome = Compute(job);
 
             if (outcome.Verified && outcome.Hash != null)
             {