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) <noreply@anthropic.com>
This commit is contained in:
parent
d337cf4a1e
commit
e801a69554
2 changed files with 78 additions and 11 deletions
|
|
@ -123,10 +123,32 @@ public class AuthIdTests : IDisposable
|
|||
);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue