From e801a695541c65c2c6221485ea37d6e5dbc33358 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sat, 8 Aug 2026 01:21:19 -0700 Subject: [PATCH] fix(network): do not consume an auth id until ownership is proven ConsumeAuthId removed the entry before checking the address or the account, so anyone landing on a live id burned it. Its owner then arrived to "unable to find auth id" and had to start the login over -- from a packet they had no part in. Look, then take. A mismatch leaves the id alone and yields no entry; only a presenter that matches both the address and the account spends it. The reason given for taking it unconditionally -- that a guessed id must not be reusable to probe for its owner -- does not survive checking the order. The address is compared first, so a guesser from anywhere else is rejected before a username is ever looked at, and one from the victim's own address is already on their network. Co-Authored-By: Claude Opus 5 (1M context) --- .../Tests/Network/Packets/AuthIdTests.cs | 69 +++++++++++++++++-- .../Network/Packets/IncomingAccountPackets.cs | 20 ++++-- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs index d2fee6a07..470741444 100644 --- a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs @@ -123,10 +123,32 @@ public class AuthIdTests : IDisposable ); } + /// + /// A rejected attempt must not consume the id. Anyone landing on a live id could otherwise burn + /// it, and its owner would arrive to "unable to find auth id" and have to log in again. + /// [Fact] - public void IsSpentEvenWhenTheAccountDoesNotMatch() + public void SurvivesAnAttemptFromTheWrongAddress() { - var account = CreateAccount("authid-spent-user"); + var account = CreateAccount("authid-not-burned-address-user"); + var authId = Register(account, AddressX); + + Assert.Equal( + IncomingAccountPackets.AuthIdResult.Rejected, + IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressY, out _) + ); + + Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount); + Assert.Equal( + IncomingAccountPackets.AuthIdResult.Vouched, + IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _) + ); + } + + [Fact] + public void SurvivesAnAttemptForTheWrongAccount() + { + var account = CreateAccount("authid-not-burned-account-user"); var authId = Register(account, AddressX); Assert.Equal( @@ -134,13 +156,52 @@ public class AuthIdTests : IDisposable IncomingAccountPackets.ConsumeAuthId(authId, "not-the-owner", AddressX, out _) ); - // Spent regardless, so a guessed id cannot be reused to probe for its owner. + Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount); Assert.Equal( - IncomingAccountPackets.AuthIdResult.Rejected, + IncomingAccountPackets.AuthIdResult.Vouched, IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _) ); } + [Fact] + public void ARejectedAttemptYieldsNoEntry() + { + var account = CreateAccount("authid-no-leak-user"); + var authId = Register(account, AddressX); + + IncomingAccountPackets.ConsumeAuthId(authId, "not-the-owner", AddressX, out var entry); + + Assert.Null(entry.Account); + } + + /// + /// An expired id is still spent by its owner: they fall back to the password verify, and the id + /// has done everything it is ever going to do. + /// + [Fact] + public void AnExpiredIdIsSpentByItsOwner() + { + var account = CreateAccount("authid-expired-spent-user"); + var authId = Register(account, AddressX); + + var now = Core._now; + + try + { + Core._now = now + TimeSpan.FromMinutes(30.0); + + Assert.Equal( + IncomingAccountPackets.AuthIdResult.Expired, + IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _) + ); + Assert.Equal(0, IncomingAccountPackets.AuthIdWindowCount); + } + finally + { + Core._now = now; + } + } + /// /// An expired id is not a lockout. A player can sit on the server list, and before any of this /// existed the game login always verified the password anyway -- so falling back to that verify diff --git a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs index 236deb362..a2217b24f 100644 --- a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs +++ b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs @@ -398,21 +398,27 @@ public static class IncomingAccountPackets int authId, string username, IPAddress address, out AuthIDPersistence entry ) { - if (!_authIDWindow.Remove(authId, out entry)) + if (!_authIDWindow.TryGetValue(authId, out entry)) { return AuthIdResult.Rejected; } - // Address before age: a different address is a different client, however fresh the id is. - if (!Utility.Intern(address).Equals(entry.Address)) + // Look, then take. Removing before the presenter has shown the id is theirs would let anyone + // who lands on a live id burn it, and its owner would arrive to "unable to find auth id" and + // have to log in again. Address first: a different address is a different client however + // fresh the id is, and checking it before the username means a remote guesser never learns + // whether a username matched. + if (!Utility.Intern(address).Equals(entry.Address) + || entry.Account == null + || !username.InsensitiveEquals(entry.Account.Username)) { + entry = default; return AuthIdResult.Rejected; } - if (entry.Account == null || !username.InsensitiveEquals(entry.Account.Username)) - { - return AuthIdResult.Rejected; - } + // Theirs, so spend it. Expired still counts as spent -- they fall back to the password + // verify and the id has done all it is ever going to do. + _authIDWindow.Remove(authId); return Core.Now - entry.Age > _authIDLifetime ? AuthIdResult.Expired : AuthIdResult.Vouched; }