warrentyexpired/ModernUO
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

refactor(accounts): drop the migrated-password repair path

Browse source 
Every ModernUO shard has always run Argon2, so the RunUO/ServUO migration
shapes the repair path existed to recover do not occur in practice. It was
never free: it retried a failed verify against the other phrase rule, and
nothing in a stored hash separates a mis-migrated credential from a password
that merely begins with the username -- which is why it needed a per-account
tag on top of the config switch to be safe at all.

Removing it takes accountSecurity.repairMigratedPasswords, the RepairPasswordTag
opt-in and the forced rehash with it. The rehash on a successful login is now
implicit: stale algorithm or stale parameters, nothing else.

Also trims the development narrative out of the comments left behind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Kamron Batman 2026-08-07 23:51:43 -07:00
parent da0d993186
commit c25a644a33
7 changed files with 35 additions and 282 deletions
Download patch file Download diff file Expand all files Collapse all files

5
Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs
View file

@ -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();

178
Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs
View file

@ -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);
}
/// <summary>
/// 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.
/// </summary>
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;
}
/// <summary>
/// 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.
/// </summary>
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));
}
/// <summary>
/// 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.
/// </summary>
[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));
}
}

12
Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs
View file

@ -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")]

82
Projects/UOContent/Accounting/Account.cs
View file

@ -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<Account>
{
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<Account>
return true;
}
/// <summary>
/// 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.
/// </summary>
private static bool UsesUsernamePhrase(PasswordProtectionAlgorithm algorithm) =>
algorithm is PasswordProtectionAlgorithm.SHA1 or PasswordProtectionAlgorithm.SHA2;
/// <summary>
/// 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
/// <see cref="AccountSecurity.RepairMigratedPasswords"/> switch must be on as well.
/// </summary>
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);
}

18
Projects/UOContent/Accounting/Security/AccountSecurity.cs
View file

@ -35,21 +35,6 @@ public static class AccountSecurity
{
public static PasswordProtectionAlgorithm CurrentAlgorithm { get; set; }
/// <summary>
/// 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
/// <see cref="Account.RepairPasswordTag"/> 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.
/// </summary>
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.");

21
Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs
View file

@ -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

1
dev-docs/configuration.md
View file

@ -99,7 +99,6 @@ accountHandler.enableAutoAccountCreation
accountHandler.enablePlayerPasswordCommand
accountHandler.maxAccountsPerIP
accountSecurity.encryptionAlgorithm
accountSecurity.repairMigratedPasswords
autosave.enabled
autosave.saveDelay
world.savePath