diff --git a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs index b41e876cd..42389cbef 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs @@ -54,4 +54,23 @@ public class AccountPasswordTests : IDisposable 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); + + // Simulate an account stored under the pre-1.20.0 default: 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("hunter2")); + 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("hunter2")); + 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 17a25cd3b..3fe83c9ac 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs @@ -86,4 +86,38 @@ public class PasswordProtectionTest 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 + 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)); + } + + [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 03ce9a2af..a4246784f 100644 --- a/Projects/UOContent/Accounting/Account.cs +++ b/Projects/UOContent/Accounting/Account.cs @@ -6,6 +6,7 @@ using System.Xml; using ModernUO.CodeGeneratedEvents; using ModernUO.Serialization; using Server.Accounting.Security; +using Server.Logging; using Server.Misc; using Server.Mobiles; using Server.Multis; @@ -18,6 +19,8 @@ namespace Server.Accounting; [SerializationGenerator(6)] public partial class Account : IAccount, IComparable { + private static readonly ILogger logger = LogFactory.GetLogger(typeof(Account)); + public static readonly TimeSpan YoungDuration = TimeSpan.FromHours(40.0); public static readonly TimeSpan InactiveDuration = TimeSpan.FromDays(180.0); public static readonly TimeSpan EmptyInactiveDuration = TimeSpan.FromDays(30.0); @@ -408,9 +411,13 @@ public partial class Account : IAccount, IComparable return false; } - // Upgrade the password protection in case we change the algorithm - if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm) + // Rehash when either the algorithm or its cost parameters have moved on. The short-circuit + // ordering is load-bearing: NeedsRehash must never be handed a hash produced by a different + // algorithm, and the first clause guarantees it is not. + if (_passwordAlgorithm != AccountSecurity.CurrentAlgorithm || + AccountSecurity.CurrentPasswordProtection.NeedsRehash(Password)) { + logger.Debug("Rehashing the password for account '{Username}'.", _username); 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 a008de11b..f89150aa5 100644 --- a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs @@ -39,4 +39,22 @@ public class Argon2PasswordProtection : IPasswordProtection public bool ValidatePassword(string encryptedPassword, string plainPassword) => _passwordHasher.Verify(encryptedPassword, plainPassword); + + public bool NeedsRehash(string encryptedPassword) + { + // Argon2's PHC string embeds m, t and p, so verification uses the parameters stored with + // each account rather than the configured ones. Without this comparison a parameter change + // would reach nobody: CheckPassword only rehashed when the *algorithm* changed. + if (!Argon2PasswordHasher.TryExtractMetadataValues(encryptedPassword, out var values)) + { + // Only reached after a successful verify, so an unparseable string means a format this + // build does not understand. Rewriting it into the current one is strictly better. + return true; + } + + return values.ArgonType != _passwordHasher.ArgonType + || values.MemoryCost != _passwordHasher.MemoryCost + || values.TimeCost != _passwordHasher.TimeCost + || values.Parallelism != _passwordHasher.Parallelism; + } }