diff --git a/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs b/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs index 4b4412ef8..d25af1d46 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/PasswordWorkerTests.cs +++ b/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 }; diff --git a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs index b86df418d..a0ef97a56 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs @@ -75,6 +75,32 @@ public class PasswordProtectionTest Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password")); } + /// + /// Literal digests of , 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. + /// + [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 = diff --git a/Projects/UOContent/Accounting/AccountHandler.cs b/Projects/UOContent/Accounting/AccountHandler.cs index 0fe215836..17ee2f809 100644 --- a/Projects/UOContent/Accounting/AccountHandler.cs +++ b/Projects/UOContent/Accounting/AccountHandler.cs @@ -397,18 +397,13 @@ public static class AccountHandler } /// - /// Hands an Argon2 verify to the verification thread. - /// - /// Argon2-stored accounts only: SHA/MD5 protections share a HashAlgorithm whose - /// ComputeHash 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. /// 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, diff --git a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs index 3676dba7d..0a952117d 100644 --- a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs @@ -21,14 +21,6 @@ public class Argon2PasswordProtection : IPasswordProtection { public static IPasswordProtection Instance = new Argon2PasswordProtection(); - /// - /// An instance sharing no state with . Verification is static-backed and - /// safe to call from anywhere, but hashing draws its salt from a per-instance - /// , so a thread that hashes off the game loop takes its own - /// rather than racing the loop for that one field. - /// - 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( diff --git a/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs b/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs index 6d6e579cd..63d0ac7a8 100644 --- a/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/HashAlgorithmPasswordProtection.cs @@ -19,19 +19,47 @@ using Server.Text; namespace Server.Accounting.Security; +/// +/// 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 . +/// A 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. +/// 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 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) => diff --git a/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs b/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs index 7ae464021..d1c7c381e 100644 --- a/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/PBKDF2PasswordProtection.cs @@ -32,7 +32,10 @@ public class PBKDF2PasswordProtection : IPasswordProtection public string EncryptPassword(string plainPassword) { Span 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); diff --git a/Projects/UOContent/Accounting/Security/PasswordWorker.cs b/Projects/UOContent/Accounting/Security/PasswordWorker.cs index 0a1d44089..0ef3b956f 100644 --- a/Projects/UOContent/Accounting/Security/PasswordWorker.cs +++ b/Projects/UOContent/Accounting/Security/PasswordWorker.cs @@ -39,6 +39,10 @@ internal sealed class PasswordJob /// Hash to verify against, with . public string StoredHash; + /// Algorithm was written with. Resolved on the loop, because + /// AccountSecurity.CurrentAlgorithm is mutable state the worker must not read. + public PasswordProtectionAlgorithm StoredAlgorithm; + /// Phrase to verify, or null to skip verification. public string VerifyPhrase; @@ -108,10 +112,6 @@ internal sealed class PasswordWorker private readonly AutoResetEvent _work = new(false); private readonly ConcurrentQueue _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 /// internal static void SetPassword(Account account, string plainPassword, Action onDone) { - if (!Enabled || AccountSecurity.CurrentAlgorithm != PasswordProtectionAlgorithm.Argon2) + if (!Enabled) { account.SetPassword(plainPassword); onDone?.Invoke(true); @@ -272,8 +276,7 @@ internal sealed class PasswordWorker } /// Runs a job on the calling thread. The seam the tests drive. - internal static PasswordOutcome ComputeInline(PasswordJob job) => - Instance.Compute(job); + internal static PasswordOutcome ComputeInline(PasswordJob job) => Compute(job); /// /// 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) {