From e284227557ee13d091bf3454c80f0cbf32df7719 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:50:40 -0700 Subject: [PATCH] feat(accounts): rehash stale Argon2 parameters on successful login Argon2's PHC string embeds m, t and p, so verification uses the parameters stored with each account rather than the configured ones -- and verify is the hot path. CheckPassword only rehashed when the ALGORITHM changed, never when its cost parameters did, so changing the defaults reached nobody on an established shard and the change was cosmetic. IPasswordProtection.NeedsRehash defaults to false, leaving PBKDF2 and the HashAlgorithm protections untouched; only Argon2 carries cost in its stored value. Logged at Debug because a restart migrates the whole population at once. Co-Authored-By: Claude Opus 5 (1M context) --- .../Tests/Accounting/AccountPasswordTests.cs | 19 +++++++++++ .../Security/PasswordProtectionTest.cs | 34 +++++++++++++++++++ Projects/UOContent/Accounting/Account.cs | 11 ++++-- .../Accounting/IPasswordProtection.cs | 7 ++++ .../Security/Argon2PasswordProtection.cs | 18 ++++++++++ 5 files changed, 87 insertions(+), 2 deletions(-) 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; + } }