diff --git a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs index 640c05837..1325700d5 100644 --- a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs +++ b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs @@ -100,6 +100,9 @@ internal static class TestServerInitializer } World.Configure(); + // Registers the Accounts entity persistence, without which Accounts.NewAccount cannot + // resolve and no test can construct an Account. + Server.Accounting.Accounts.Configure(); RaceDefinitions.Configure(); MovementImpl.Configure(); PathFollower.Configure(); diff --git a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs new file mode 100644 index 000000000..14e6089cd --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs @@ -0,0 +1,74 @@ +using System; +using Server.Accounting; +using Server.Accounting.Security; +using Xunit; + +namespace Server.Tests.Accounting; + +[Collection("Sequential UOContent Tests")] +public class AccountPasswordTests : IDisposable +{ + private const string Password = "hunter2"; + + // CurrentAlgorithm is process-wide state shared with the rest of the collection. + private readonly PasswordProtectionAlgorithm _originalAlgorithm = AccountSecurity.CurrentAlgorithm; + + public void Dispose() => AccountSecurity.CurrentAlgorithm = _originalAlgorithm; + + [Theory] + [InlineData(PasswordProtectionAlgorithm.SHA1)] + [InlineData(PasswordProtectionAlgorithm.SHA2)] + [InlineData(PasswordProtectionAlgorithm.PBKDF2)] + [InlineData(PasswordProtectionAlgorithm.Argon2)] + public void NewAccount_CanLogIn(PasswordProtectionAlgorithm algorithm) + { + AccountSecurity.CurrentAlgorithm = algorithm; + var account = new Account($"new-{algorithm}-user", Password); + + Assert.Equal(algorithm, account.PasswordAlgorithm); + Assert.True(account.CheckPassword(Password)); + Assert.False(account.CheckPassword("wrong-password")); + } + + // SetPassword assigns PasswordAlgorithm before deriving the phrase from it. Reversing those two + // lines salts the hash by the outgoing algorithm's rule and stores it under the incoming one, + // which verifies once and then never again. + [Theory] + [InlineData(PasswordProtectionAlgorithm.SHA1)] + [InlineData(PasswordProtectionAlgorithm.SHA2)] + [InlineData(PasswordProtectionAlgorithm.PBKDF2)] + public void UpgradingAlgorithm_DoesNotLockTheAccountOut(PasswordProtectionAlgorithm from) + { + AccountSecurity.CurrentAlgorithm = from; + var account = new Account($"upgrade-{from}-user", Password); + Assert.True(account.CheckPassword(Password)); + + AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + + Assert.True(account.CheckPassword(Password)); + Assert.Equal(PasswordProtectionAlgorithm.Argon2, account.PasswordAlgorithm); + + // Must verify against what the rehash wrote. + Assert.True(account.CheckPassword(Password)); + Assert.False(account.CheckPassword("wrong-password")); + } + + [Fact] + public void StaleArgon2Parameters_AreRehashedOnLogin() + { + AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2; + var account = new Account("stale-params-user", Password); + + // The shipping default before this change: Argon2i, m=8192, t=3, p=1. + account.Password = + "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; + + Assert.True(account.CheckPassword(Password)); + Assert.StartsWith("$argon2id$v=19$m=16384,t=1,p=1$", account.Password); + + // Already current: verifying again must not rewrite the hash. + var afterFirst = account.Password; + Assert.True(account.CheckPassword(Password)); + Assert.Equal(afterFirst, account.Password); + } +} diff --git a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs index effd2da88..333c35692 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs @@ -74,4 +74,64 @@ public class PasswordProtectionTest Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password")); } + + // The shipping default before this change. A literal, so it cannot drift with the configured + // defaults. Password: "hunter2". + private const string LegacyArgon2iHash = + "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; + + [Fact] + public void Argon2_ValidatesLegacyArgon2iHash() + { + Assert.True(Argon2PasswordProtection.Instance.ValidatePassword(LegacyArgon2iHash, "hunter2")); + Assert.False(Argon2PasswordProtection.Instance.ValidatePassword(LegacyArgon2iHash, "wrong")); + } + + [Theory] + // type, memory, time, parallelism -> expected NeedsRehash + [InlineData("argon2id", 16384, 1, 1, false)] // current defaults + [InlineData("argon2i", 8192, 3, 1, true)] // the old shipping default + [InlineData("argon2id", 8192, 1, 1, true)] // right type, stale memory + [InlineData("argon2id", 16384, 3, 1, true)] // right type, stale iterations + [InlineData("argon2id", 16384, 1, 2, true)] // right type, stale parallelism + [InlineData("argon2i", 16384, 1, 1, true)] // right cost, stale type + public void Argon2_NeedsRehash_ComparesTypeAndCost( + string type, int memory, int time, int parallelism, bool expected + ) + { + var hash = $"${type}$v=19$m={memory},t={time},p={parallelism}$" + + "LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; + + Assert.Equal(expected, Argon2PasswordProtection.Instance.NeedsRehash(hash)); + } + + // Digest and salt lengths are the decoded sizes of the base64 segments, not parameter-list + // entries, so they need their own literals. Current type and cost throughout; only a length + // differs from the defaults. The theory above is the negative control at default lengths. + [Theory] + // 16-byte digest: 22 base64 chars instead of the 43 a 32-byte digest encodes to. + [InlineData("$argon2id$v=19$m=16384,t=1,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4g")] + // 8-byte salt: 11 base64 chars instead of the 22 a 16-byte salt encodes to. + [InlineData("$argon2id$v=19$m=16384,t=1,p=1$LD1XJz7P3wQ$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw")] + public void Argon2_NeedsRehash_ComparesSaltAndDigestLengths(string hash) + { + Assert.True(Argon2PasswordProtection.Instance.NeedsRehash(hash)); + } + + [Theory] + [InlineData("")] + [InlineData("not-a-hash")] + public void Argon2_NeedsRehash_IsTrueForUnparseableHashes(string hash) + { + Assert.True(Argon2PasswordProtection.Instance.NeedsRehash(hash)); + } + + [Fact] + public void NonArgon2Protections_NeverNeedRehash() + { + Assert.False(PBKDF2PasswordProtection.Instance.NeedsRehash("anything")); + Assert.False(HashAlgorithmPasswordProtection.SHA2Instance.NeedsRehash("anything")); + Assert.False(HashAlgorithmPasswordProtection.SHA1Instance.NeedsRehash("anything")); + Assert.False(HashAlgorithmPasswordProtection.MD5Instance.NeedsRehash("anything")); + } } diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs index d79d70b1a..45ede29bb 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -378,12 +378,12 @@ public partial class Account : IAccount, IComparable public void SetPassword(string plainPassword) { - var phrase = _passwordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 + PasswordAlgorithm = AccountSecurity.CurrentAlgorithm; + var phrase = PasswordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2 ? $"{_username}{plainPassword}" : plainPassword; Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword(phrase); - PasswordAlgorithm = AccountSecurity.CurrentAlgorithm; } public bool CheckPassword(string plainPassword) @@ -399,7 +399,8 @@ public partial class Account : IAccount, IComparable } // Upgrade the password protection in case we change the algorithm - if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm) + if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm || + AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password)) { SetPassword(plainPassword); } diff --git a/Projects/UOContent/Accounting/IPasswordProtection.cs b/Projects/UOContent/Accounting/IPasswordProtection.cs index 0fdd6ced9..f5590a148 100644 --- a/Projects/UOContent/Accounting/IPasswordProtection.cs +++ b/Projects/UOContent/Accounting/IPasswordProtection.cs @@ -4,5 +4,12 @@ namespace Server.Accounting { string EncryptPassword(string plainPassword); bool ValidatePassword(string encryptedPassword, string plainPassword); + + /// + /// True when was produced with parameters that differ + /// from the ones this protection currently uses, so a successful login should rewrite it. + /// Algorithms whose cost is not embedded in the stored value never need this. + /// + bool NeedsRehash(string encryptedPassword) => false; } } diff --git a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs index 99f320a78..3b509e099 100644 --- a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs @@ -21,11 +21,38 @@ public class Argon2PasswordProtection : IPasswordProtection { public static IPasswordProtection Instance = new Argon2PasswordProtection(); - private readonly Argon2PasswordHasher m_PasswordHasher = new(rng: RandomNumberGenerator.Create()); + // 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( + time: 1, + memory: 16384, + parallel: 1, + type: Argon2Type.Argon2id, + rng: RandomNumberGenerator.Create() + ); public string EncryptPassword(string plainPassword) => - m_PasswordHasher.Hash(plainPassword); + _passwordHasher.Hash(plainPassword); public bool ValidatePassword(string encryptedPassword, string plainPassword) => - m_PasswordHasher.Verify(encryptedPassword, plainPassword); + _passwordHasher.Verify(encryptedPassword, plainPassword); + + // The PHC string carries the parameters it was hashed with, so verification uses those rather + // than the configured ones. Comparing them is what lets a parameter change reach existing + // accounts. + public bool NeedsRehash(string encryptedPassword) + { + // Unparseable but verified: a format this build does not understand, so rewrite it. + if (!Argon2PasswordHasher.TryExtractMetadataValues(encryptedPassword, out var values)) + { + return true; + } + + return values.ArgonType != _passwordHasher.ArgonType + || values.MemoryCost != _passwordHasher.MemoryCost + || values.TimeCost != _passwordHasher.TimeCost + || values.Parallelism != _passwordHasher.Parallelism + || values.HashLength != (int)_passwordHasher.HashLength + || values.SaltLength != (int)_passwordHasher.SaltLength; + } } diff --git a/Projects/UOContent/UOContent.csproj b/Projects/UOContent/UOContent.csproj index bf288e629..d9961f38d 100644 --- a/Projects/UOContent/UOContent.csproj +++ b/Projects/UOContent/UOContent.csproj @@ -45,7 +45,7 @@ - + diff --git a/dev-docs/configuration.md b/dev-docs/configuration.md index 9ddc348a7..b69349451 100644 --- a/dev-docs/configuration.md +++ b/dev-docs/configuration.md @@ -98,6 +98,7 @@ Examples from the codebase: accountHandler.enableAutoAccountCreation accountHandler.enablePlayerPasswordCommand accountHandler.maxAccountsPerIP +accountSecurity.encryptionAlgorithm autosave.enabled autosave.saveDelay world.savePath