From 2ddbb8891c805cc3e90fb653e83160bafb3b7f91 Mon Sep 17 00:00:00 2001 From: miniduikboot Date: Sun, 7 Jul 2024 23:33:02 +0200 Subject: [PATCH] Refactor SetColor anticheat One issue with the ColorLimits checks is that it didn't account for mods that add extra colors, this is fixed by checking if the route followed is plausible instead of mimicking client logic and only accepting a single answer that it expected. A false negative is possible where the host decides to wrap back to the front but there are still custom colors available at the end of the list. This is acceptable because Impostor doesn't currently know the amount of colors available and this issue isn't major enough to ask all mod authors to declare their custom color amount to the server. As extra colors should now be handled by this code, it makes sense to change the category of the unknown colors check to ProtocolExtensions. For these mods an exemption of the ColorLimits category is no longer necessary. Note that mods that allow duplicate colors that do not use a custom rpc to set colors can only be supported by disabling the ColorLimits category, this was already the case and I don't think it can be unified with the existing check. --- .../Net/Inner/Objects/InnerPlayerControl.cs | 71 ++++++++++++++----- src/Impostor.Server/Net/State/Game.cs | 11 +++ 2 files changed, 63 insertions(+), 19 deletions(-) diff --git a/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs b/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs index 60340ce..3cd0295 100644 --- a/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs +++ b/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs @@ -700,6 +700,7 @@ namespace Impostor.Server.Net.Inner.Objects if (RequestedPlayerName.Any()) { var expected = RequestedPlayerName.Dequeue(); + var requested = expected; if (Game.Players.Any(x => x.Character != null && x.Character != this && x.Character.PlayerInfo.PlayerName == expected)) { @@ -720,7 +721,10 @@ namespace Impostor.Server.Net.Inner.Objects if (name != expected) { - if (await sender.Client.ReportCheatAsync(RpcCalls.SetName, CheatCategory.NameLimits, "Client sent SetName with incorrect name")) + if (await sender.Client.ReportCheatAsync( + RpcCalls.SetName, + CheatCategory.NameLimits, + $"Client sent SetName with incorrect name, got '{name}', requested '{requested}', expected '{expected}'")) { await SetNameAsync(expected); return false; @@ -751,9 +755,9 @@ namespace Impostor.Server.Net.Inner.Objects } } - if ((byte)color > ColorsCount) + if ((byte)color >= ColorsCount) { - if (await sender.Client.ReportCheatAsync(RpcCalls.CheckColor, CheatCategory.ColorLimits, "Client sent invalid color")) + if (await sender.Client.ReportCheatAsync(RpcCalls.CheckColor, CheatCategory.ProtocolExtension, "Client sent unknown color")) { return false; } @@ -774,9 +778,17 @@ namespace Impostor.Server.Net.Inner.Objects } } + if ((byte)color >= ColorsCount) + { + if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ProtocolExtension, "Client sent unknown color")) + { + return false; + } + } + if (sender.IsOwner(this)) { - if (Game.Players.Any(x => x.Character != null && x.Character != this && x.Character.PlayerInfo.CurrentOutfit.Color == color)) + if (Game.IsColorUsed(color, this)) { if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ColorLimits, "Client sent a color that is already used")) { @@ -788,33 +800,54 @@ namespace Impostor.Server.Net.Inner.Objects { if (RequestedColorId.Any()) { - var expected = RequestedColorId.Dequeue(); + var requested = RequestedColorId.Dequeue(); - for (var colorOffset = 0; colorOffset <= ColorsCount; colorOffset++) + if (Game.IsColorUsed(color, this)) { - var possibleColor = (ColorType)((byte)(expected + colorOffset) % ColorsCount); - if (!Game.Players.Any(x => x.Character != null && x.Character != this && x.Character.PlayerInfo.CurrentOutfit.Color == possibleColor)) + if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ColorLimits, "Client selected a color that is already used")) { - expected = possibleColor; - break; + return false; } + } + + var startFrom = requested; - if (colorOffset == ColorsCount) + // Among Us wraps to the front if all colors between the requested and the final color are in use, check for this + if (requested > color) + { + // Among Us wrapped, so check all colors between requested and the final color + for (var c = requested; (byte)c < ColorsCount; c++) { - if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ColorLimits, "Client sent SetColor but all colors are already in use")) + if (!Game.IsColorUsed(c, this)) { - await SetColorAsync(expected); - return false; + if (await sender.Client.ReportCheatAsync( + RpcCalls.SetColor, + CheatCategory.ColorLimits, + $"Client skipped color {c} that could be used, but wrapped instead. Player requested {requested} but was given {color}")) + { + await SetColorAsync(c); + return false; + } } } + + // Start checking from the first color, fallthrough to normal case + startFrom = ColorType.Red; } - if (color != expected) + // Check all colors between the requested color and the assigned color + for (var c = startFrom; c < color; c++) { - if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ColorLimits, "Client sent SetColor with incorrect color")) + if (!Game.IsColorUsed(c, this)) { - await SetColorAsync(expected); - return false; + if (await sender.Client.ReportCheatAsync( + RpcCalls.SetColor, + CheatCategory.ColorLimits, + $"Client skipped color {c} that could be used. Player requested {requested} but was given {color}")) + { + await SetColorAsync(c); + return false; + } } } } @@ -1019,7 +1052,7 @@ namespace Impostor.Server.Net.Inner.Objects if (!await ValidateRole(RpcCalls.ProtectPlayer, sender, PlayerInfo, RoleTypes.GuardianAngel)) { - return false; + return false; } ((InnerPlayerControl)target).Protect(this); diff --git a/src/Impostor.Server/Net/State/Game.cs b/src/Impostor.Server/Net/State/Game.cs index 918294a..5b5921d 100644 --- a/src/Impostor.Server/Net/State/Game.cs +++ b/src/Impostor.Server/Net/State/Game.cs @@ -9,8 +9,10 @@ using Impostor.Api.Config; using Impostor.Api.Events.Managers; using Impostor.Api.Games; using Impostor.Api.Innersloth; +using Impostor.Api.Innersloth.Customization; using Impostor.Api.Innersloth.GameOptions; using Impostor.Api.Net; +using Impostor.Api.Net.Inner.Objects; using Impostor.Api.Net.Manager; using Impostor.Api.Net.Messages.S2C; using Impostor.Server.Events; @@ -131,6 +133,15 @@ namespace Impostor.Server.Net.State } } + /// Check if there are players using a color. + /// The color to check for. + /// Exempt a player from being checked. + /// True if there is player other than exceptBy that uses that color. + internal bool IsColorUsed(ColorType color, IInnerPlayerControl? exceptBy = null) + { + return Players.Any(p => p.Character != null && p.Character != exceptBy && p.Character.PlayerInfo.CurrentOutfit.Color == color); + } + private ValueTask BroadcastJoinMessage(IMessageWriter message, bool clear, ClientPlayer player) { Message01JoinGameS2C.SerializeJoin(message, clear, Code, player, HostId); -- 2.39.5