From 609c30ffb4b3b3505e81db2b5ebc7503e3017010 Mon Sep 17 00:00:00 2001 From: miniduikboot Date: Sun, 7 Jul 2024 23:33:02 +0200 Subject: [PATCH] Refactor SetColor/Name anticheat Turns out they were broken when I disabled them as I wasn't tracking playerinfo's properly, but this is now fixed, so the checks can be re-enabled. 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 | 89 +++++++++++++------ src/Impostor.Server/Net/State/Game.cs | 11 +++ 2 files changed, 71 insertions(+), 29 deletions(-) diff --git a/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs b/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs index e722f4b2..c7f00d0d 100644 --- a/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs +++ b/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs @@ -703,6 +703,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)) { @@ -723,12 +724,14 @@ namespace Impostor.Server.Net.Inner.Objects if (name != expected) { - // TODO fix - // if (await sender.Client.ReportCheatAsync(RpcCalls.SetName, CheatCategory.NameLimits, $"Client sent SetName with incorrect name, got '{name}', expected '{expected}'")) - // { - // await SetNameAsync(expected); - // return false; - // } + 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; + } } } else @@ -755,9 +758,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; } @@ -778,9 +781,17 @@ namespace Impostor.Server.Net.Inner.Objects } } + if ((byte)color >= ColorsCount) + { + if (await sender.Client.ReportCheatAsync(RpcCalls.CheckColor, 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")) { @@ -792,36 +803,56 @@ 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; + + // 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 = (ColorType)requested; c < (ColorType)ColorsCount; c++) + { + if (!Game.IsColorUsed(c, this)) + { + 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; + } + } } - if (colorOffset == ColorsCount) + // Start checking from the first color, fallthrough to normal case + startFrom = ColorType.Red; + } + + // Check all colors between the requested color and the assigned color + for (var c = startFrom; c < color; c++) + { + if (!Game.IsColorUsed(c, this)) { - if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ColorLimits, "Client sent SetColor but all colors are already in use")) + 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(expected); + await SetColorAsync(c); return false; } } } - - // TODO fix - /*if (color != expected) - { - if (await sender.Client.ReportCheatAsync(RpcCalls.SetColor, CheatCategory.ColorLimits, "Client sent SetColor with incorrect color")) - { - await SetColorAsync(expected); - return false; - } - }*/ } else { @@ -1011,7 +1042,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 918294a7..5b5921d3 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);