diff --git a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs
index c98fe65d6..1325700d5 100644
--- a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs
+++ b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs
@@ -100,9 +100,8 @@ internal static class TestServerInitializer
}
World.Configure();
- // Registers the Accounts entity persistence. Production reaches this through
- // AssemblyHandler.Invoke("Configure"); the curated subset here must call it so that
- // Accounts.NewAccount resolves and tests can construct an Account.
+ // 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();
diff --git a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs
index 7bd1213e8..14e6089cd 100644
--- a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs
+++ b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs
@@ -10,19 +10,10 @@ public class AccountPasswordTests : IDisposable
{
private const string Password = "hunter2";
- // AccountSecurity.CurrentAlgorithm and AccountSecurity.RepairMigratedPasswords are process-wide
- // static state, shared with every other class in the "Sequential UOContent Tests" collection.
- // xUnit constructs/disposes this class once per test case, so capturing and restoring them here
- // means every case -- current and any added later to this file -- starts from and leaves behind
- // the ambient values, instead of bleeding whatever it last set into the rest of the collection.
+ // CurrentAlgorithm is process-wide state shared with the rest of the collection.
private readonly PasswordProtectionAlgorithm _originalAlgorithm = AccountSecurity.CurrentAlgorithm;
- private readonly bool _originalRepairMigratedPasswords = AccountSecurity.RepairMigratedPasswords;
- public void Dispose()
- {
- AccountSecurity.CurrentAlgorithm = _originalAlgorithm;
- AccountSecurity.RepairMigratedPasswords = _originalRepairMigratedPasswords;
- }
+ public void Dispose() => AccountSecurity.CurrentAlgorithm = _originalAlgorithm;
[Theory]
[InlineData(PasswordProtectionAlgorithm.SHA1)]
@@ -39,6 +30,9 @@ public class AccountPasswordTests : IDisposable
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)]
@@ -51,11 +45,10 @@ public class AccountPasswordTests : IDisposable
AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2;
- // First login verifies under the old algorithm and rehashes under the new one.
Assert.True(account.CheckPassword(Password));
Assert.Equal(PasswordProtectionAlgorithm.Argon2, account.PasswordAlgorithm);
- // Second login must verify against what the first one wrote.
+ // Must verify against what the rehash wrote.
Assert.True(account.CheckPassword(Password));
Assert.False(account.CheckPassword("wrong-password"));
}
@@ -66,169 +59,16 @@ public class AccountPasswordTests : IDisposable
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.
+ // 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("hunter2"));
+ 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("hunter2"));
+ Assert.True(account.CheckPassword(Password));
Assert.Equal(afterFirst, account.Password);
}
-
- ///
- /// Builds an account whose stored hash is what the pre-fix SetPassword produced when a SHA2
- /// account was upgraded to Argon2: argon2(username + password), tagged Argon2, whose phrase
- /// rule omits the username. Unrecoverable without the plaintext, hence the repair path.
- ///
- private static Account CreateMisMigratedAccount(string username)
- {
- AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2;
- var account = new Account(username, Password);
- account.Password = AccountSecurity.CurrentPasswordProtection
- .EncryptPassword($"{username}{Password}");
-
- return account;
- }
-
- ///
- /// The other corruption shape: an account *created* while CurrentAlgorithm was SHA1 or SHA2.
- /// The pre-fix SetPassword ran from the constructor before _passwordAlgorithm was assigned, so
- /// it took the None branch and stored H(bare password) under an algorithm whose phrase rule
- /// adds the username. Those accounts could never log in at all.
- ///
- private static Account CreateMisCreatedShaAccount(string username, PasswordProtectionAlgorithm algorithm)
- {
- AccountSecurity.CurrentAlgorithm = algorithm;
- var account = new Account(username, Password);
- account.Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword(Password);
-
- return account;
- }
-
- [Fact]
- public void MisMigratedAccount_IsRejected_WhenRepairIsDisabled()
- {
- var account = CreateMisMigratedAccount("repair-off-user");
- account.SetTag(Account.RepairPasswordTag, "yes");
- AccountSecurity.RepairMigratedPasswords = false;
-
- Assert.False(account.CheckPassword(Password));
- }
-
- [Fact]
- public void MisMigratedAccount_IsRejected_WhenTheAccountIsNotTagged()
- {
- var account = CreateMisMigratedAccount("repair-untagged-user");
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.Null(account.GetTag(Account.RepairPasswordTag));
- Assert.False(account.CheckPassword(Password));
- }
-
- [Fact]
- public void MisMigratedAccount_IsRepaired_WhenRepairIsEnabled()
- {
- var account = CreateMisMigratedAccount("repair-on-user");
- account.SetTag(Account.RepairPasswordTag, "yes");
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.True(account.CheckPassword(Password));
-
- AccountSecurity.RepairMigratedPasswords = false;
-
- // Repaired in place: it must now verify with the flag back off.
- Assert.True(account.CheckPassword(Password));
- Assert.False(account.CheckPassword("wrong-password"));
- }
-
- [Theory]
- [InlineData(PasswordProtectionAlgorithm.SHA1)]
- [InlineData(PasswordProtectionAlgorithm.SHA2)]
- public void MisCreatedShaAccount_IsRepaired_WhenBothGatesAreSet(PasswordProtectionAlgorithm algorithm)
- {
- var account = CreateMisCreatedShaAccount($"repair-created-{algorithm}-user", algorithm);
- account.SetTag(Account.RepairPasswordTag, "yes");
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.True(account.CheckPassword(Password));
-
- AccountSecurity.RepairMigratedPasswords = false;
-
- // Repaired in place under the username-salted rule its algorithm actually uses.
- Assert.True(account.CheckPassword(Password));
- Assert.False(account.CheckPassword("wrong-password"));
- }
-
- [Theory]
- [InlineData(PasswordProtectionAlgorithm.SHA1)]
- [InlineData(PasswordProtectionAlgorithm.SHA2)]
- public void MisCreatedShaAccount_IsRejected_WhenTheAccountIsNotTagged(PasswordProtectionAlgorithm algorithm)
- {
- var account = CreateMisCreatedShaAccount($"reject-created-{algorithm}-user", algorithm);
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.False(account.CheckPassword(Password));
- }
-
- [Fact]
- public void RepairTag_IsClearedAfterASuccessfulRepair()
- {
- var account = CreateMisMigratedAccount("repair-one-shot-user");
- account.SetTag(Account.RepairPasswordTag, "yes");
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.True(account.CheckPassword(Password));
- Assert.Null(account.GetTag(Account.RepairPasswordTag));
-
- // The window is closed for this account: corrupting it again is no longer repairable,
- // even with the shard-wide flag still on.
- account.Password = AccountSecurity.CurrentPasswordProtection
- .EncryptPassword($"repair-one-shot-user{Password}");
-
- Assert.False(account.CheckPassword(Password));
- }
-
- [Fact]
- public void WrongPassword_IsStillRejected_WhenRepairIsEnabled()
- {
- AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2;
- var account = new Account("repair-wrong-pass-user", Password);
- account.SetTag(Account.RepairPasswordTag, "yes");
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.False(account.CheckPassword("wrong-password"));
- Assert.True(account.CheckPassword(Password));
- }
-
- ///
- /// The repair cannot distinguish a mis-migrated hash from a password that merely begins with
- /// the username, so a shard-wide repair window would let anyone log into such an account with
- /// only the suffix -- and the rehash that follows would rewrite the stored credential down to
- /// that suffix, locking the real owner out permanently. The per-account tag is what stops it:
- /// with the flag on but the account untagged, the attack must fail and the hash must not move.
- ///
- [Fact]
- public void TruncatedPassword_IsRejected_AndDoesNotRewriteTheHash()
- {
- const string username = "trunc-attack-user";
- const string realPassword = $"{username}123";
-
- AccountSecurity.CurrentAlgorithm = PasswordProtectionAlgorithm.Argon2;
-
- // Stored correctly: argon2("trunc-attack-user123"), no corruption anywhere.
- var account = new Account(username, realPassword);
- var storedBefore = account.Password;
-
- AccountSecurity.RepairMigratedPasswords = true;
-
- Assert.False(account.CheckPassword("123"));
- Assert.Equal(storedBefore, account.Password);
-
- // And the owner is still able to log in afterwards.
- Assert.True(account.CheckPassword(realPassword));
- }
}
diff --git a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs
index a84557f7c..333c35692 100644
--- a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs
+++ b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs
@@ -75,8 +75,8 @@ public class PasswordProtectionTest
Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password"));
}
- // Produced by ModernUO's shipping default before this change: Argon2i, m=8192, t=3, p=1.
- // Pinned as a literal so it cannot drift with the configured defaults. Password: "hunter2".
+ // 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";
@@ -105,11 +105,9 @@ public class PasswordProtectionTest
Assert.Equal(expected, Argon2PasswordProtection.Instance.NeedsRehash(hash));
}
- // The digest and salt lengths are not in the parameter list -- they are the decoded sizes of the
- // two base64 segments -- so they cannot be varied through the theory template above. Both hashes
- // here carry the current type and cost; only a segment length differs from the library defaults
- // (32-byte digest, 16-byte salt). The "current defaults" row of the theory above is the negative
- // control: it uses those default lengths and must stay false.
+ // 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")]
diff --git a/Projects/UOContent/Accounting/Account.cs b/Projects/UOContent/Accounting/Account.cs
index 206099768..45ede29bb 100644
--- a/Projects/UOContent/Accounting/Account.cs
+++ b/Projects/UOContent/Accounting/Account.cs
@@ -6,7 +6,6 @@ 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;
@@ -19,8 +18,6 @@ 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);
@@ -379,85 +376,32 @@ public partial class Account : IAccount, IComparable
return true;
}
- ///
- /// SHA1 and SHA2 are the ServUO-compatible algorithms; they salt the password with the
- /// username. Argon2 and PBKDF2 carry their own salt and do not.
- ///
- private static bool UsesUsernamePhrase(PasswordProtectionAlgorithm algorithm) =>
- algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2;
-
- ///
- /// Account tag that opts a single account into the mis-migrated-password repair. Set it from the
- /// admin gump (Account Details -> Tags -> Add Tag) on an account whose owner has actually
- /// reported being locked out, and never on one that can still log in: the repair cannot tell a
- /// mis-migrated hash from a password that merely begins with the username, so marking a working
- /// account risks rewriting its credential down to whatever was submitted. Cleared automatically
- /// once a repair succeeds. Necessary but not sufficient: the shard-wide
- /// switch must be on as well.
- ///
- public const string RepairPasswordTag = "RepairMigratedPassword";
-
- // The pre-fix SetPassword left the credential hashed under the *other* family's phrase rule, in
- // both directions, so the repair tries whichever rule the stored algorithm does not use.
- private string RepairPhrase(string plainPassword) =>
- UsesUsernamePhrase(_passwordAlgorithm) ? plainPassword : $"{_username}{plainPassword}";
-
public void SetPassword(string plainPassword)
{
- // The phrase must match how CheckPassword will rebuild it *after* the algorithm changes,
- // so it is derived from the target algorithm rather than the outgoing one. Deriving it
- // from _passwordAlgorithm stored a username-salted hash under an algorithm that never
- // re-adds the username, locking the account out on its next login -- and, because this
- // runs from the constructor before _passwordAlgorithm is assigned, it also produced
- // brand-new SHA1/SHA2 accounts that could never log in at all.
- var algorithm = AccountSecurity.CurrentAlgorithm;
- var phrase = UsesUsernamePhrase(algorithm) ? $"{_username}{plainPassword}" : plainPassword;
+ PasswordAlgorithm = AccountSecurity.CurrentAlgorithm;
+ var phrase = PasswordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2
+ ? $"{_username}{plainPassword}"
+ : plainPassword;
- Password = AccountSecurity.GetPasswordProtection(algorithm).EncryptPassword(phrase);
- PasswordAlgorithm = algorithm;
+ Password = AccountSecurity.CurrentPasswordProtection.EncryptPassword(phrase);
}
public bool CheckPassword(string plainPassword)
{
- var protection = AccountSecurity.GetPasswordProtection(_passwordAlgorithm);
- var phrase = UsesUsernamePhrase(_passwordAlgorithm) ? $"{_username}{plainPassword}" : plainPassword;
- var forceRehash = false;
+ var phrase = _passwordAlgorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2
+ ? $"{_username}{plainPassword}"
+ : plainPassword;
- if (!protection.ValidatePassword(Password, phrase))
+ var ok = AccountSecurity.GetPasswordProtection(_passwordAlgorithm).ValidatePassword(Password, phrase);
+ if (!ok)
{
- // Nothing in the stored hash distinguishes a mis-migrated credential from a forgotten
- // password, and the retry is not free: it costs a second full verify, and failed logins
- // are the credential-stuffing surface. Worse, it cannot tell "stored is
- // H(username + password)" from "the password simply starts with the username" -- and
- // repairing the latter would rewrite a correct credential down to the submitted suffix
- // and lock the owner out for good. Hence two gates: a shard-wide switch, and a per-account
- // tag an operator sets only for someone who has reported the lockout.
- if (!AccountSecurity.RepairMigratedPasswords || GetTag(RepairPasswordTag) == null ||
- !protection.ValidatePassword(Password, RepairPhrase(plainPassword)))
- {
- return false;
- }
-
- logger.Warning(
- "Account '{Username}' had a password mis-migrated by a pre-fix SetPassword; repairing it.",
- _username
- );
-
- // One-shot: the window closes for this account as soon as it is repaired.
- RemoveTag(RepairPasswordTag);
-
- // The stored hash may already carry current parameters, so NeedsRehash would decline and
- // the account would verify once and stay broken.
- forceRehash = true;
+ return false;
}
- // 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 second clause guarantees it is not.
- if (forceRehash || _passwordAlgorithm != AccountSecurity.CurrentAlgorithm ||
+ // Upgrade the password protection in case we change the algorithm
+ 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/Security/AccountSecurity.cs b/Projects/UOContent/Accounting/Security/AccountSecurity.cs
index a156f4a61..9f7faeafc 100644
--- a/Projects/UOContent/Accounting/Security/AccountSecurity.cs
+++ b/Projects/UOContent/Accounting/Security/AccountSecurity.cs
@@ -35,21 +35,6 @@ public static class AccountSecurity
{
public static PasswordProtectionAlgorithm CurrentAlgorithm { get; set; }
- ///
- /// Shard-wide master switch for the one-time repair of accounts whose password was corrupted by
- /// the pre-fix SetPassword, which stored the credential under the wrong family's phrase rule.
- /// This flag alone repairs nothing: an account is only repaired when it *also* carries the
- /// tag, which an operator adds from the admin gump
- /// (Account Details -> Tags -> Add Tag) and which is cleared automatically once the repair
- /// succeeds. Tag only an account whose owner has actually reported being locked out, never one
- /// that can still log in: the repair cannot distinguish a mis-migrated hash from a password that
- /// merely begins with the username, so tagging a working account risks rewriting its credential
- /// down to whatever was submitted. Off by default because the repair costs a second verify on a
- /// failed login -- for tagged accounts only, so the credential-stuffing surface is unaffected by
- /// the flag on its own. Turn it on for a migration window, then off again.
- ///
- public static bool RepairMigratedPasswords { get; set; }
-
public static IPasswordProtection CurrentPasswordProtection => GetPasswordProtection(CurrentAlgorithm);
public static void Configure()
@@ -60,9 +45,6 @@ public static class AccountSecurity
PasswordProtectionAlgorithm.Argon2
);
- RepairMigratedPasswords =
- ServerConfiguration.GetOrUpdateSetting("accountSecurity.repairMigratedPasswords", false);
-
if (CurrentAlgorithm < PasswordProtectionAlgorithm.SHA2)
{
throw new Exception($"Security: {CurrentAlgorithm} is obsolete and not secure. Do not use it.");
diff --git a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs
index 9f0221e43..3b509e099 100644
--- a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs
+++ b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs
@@ -21,12 +21,8 @@ public class Argon2PasswordProtection : IPasswordProtection
{
public static IPasswordProtection Instance = new Argon2PasswordProtection();
- // Argon2id over Argon2i: RFC 9106 recommends Argon2i only where side-channel resistance is
- // required and memory is scarce. 16 MiB at t=1 measures cheaper than the old 8 MiB at t=3
- // (8.5 ms vs 10.1 ms, measured by ModernUO-Benchmarks/Benchmarks/Argon2Hashing) while doubling
- // memory-hardness, which is the property that resists GPU and ASIC cracking; iterations mostly
- // buy wall-clock. p=1 because native argon2 spawns a thread per lane, which is oversubscription
- // on the 1-2 core hosts this path exists to serve.
+ // 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,
@@ -41,22 +37,17 @@ public class Argon2PasswordProtection : IPasswordProtection
public bool ValidatePassword(string encryptedPassword, string 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)
{
- // 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.
+ // Unparseable but verified: a format this build does not understand, so rewrite it.
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;
}
- // The digest and salt lengths live in the base64 segments rather than the parameter list,
- // but they are just as much a part of "was this produced with the parameters we configure
- // today". Casting the hasher's uint properties keeps the comparison signed-vs-signed; both
- // are small byte counts, so the narrowing cannot lose anything.
return values.ArgonType != _passwordHasher.ArgonType
|| values.MemoryCost != _passwordHasher.MemoryCost
|| values.TimeCost != _passwordHasher.TimeCost
diff --git a/dev-docs/configuration.md b/dev-docs/configuration.md
index 84229821b..b69349451 100644
--- a/dev-docs/configuration.md
+++ b/dev-docs/configuration.md
@@ -99,7 +99,6 @@ accountHandler.enableAutoAccountCreation
accountHandler.enablePlayerPasswordCommand
accountHandler.maxAccountsPerIP
accountSecurity.encryptionAlgorithm
-accountSecurity.repairMigratedPasswords
autosave.enabled
autosave.saveDelay
world.savePath