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.

diff --git a/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs b/src/Impostor.Server/Net/Inner/Objects/InnerPlayerControl.cs
index 60340ce1f9806c7f75b8b3bb7190b068b1c43be2..3cd0295945e9338df605a3de470893b9a9ffb687 100644 (file)
--- 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 918294a71bd46813ac31e4d4ee1308423db461a1..5b5921d3a851b588f8d1ad7527161200ebd9fb9c 100644 (file)
--- 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
             }
         }
 
+        /// <summary>Check if there are players using a color.</summary>
+        /// <param name="color">The color to check for.</param>
+        /// <param name="exceptBy">Exempt a player from being checked.</param>
+        /// <returns>True if there is player other than exceptBy that uses that color.</returns>
+        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);