From 046d52ad2e96efe829d5faeb7216baf906659d87 Mon Sep 17 00:00:00 2001 From: Lucas Nicodemus Date: Wed, 26 May 2021 23:39:16 -0700 Subject: [PATCH] Move emoji player index check into IllegalPerSe This is the first commit in a pattern that I'd like to follow. The concept is that we specifically create handlers for things that are "illegal per se." That is, there are no possible situations (in the current protocol) where a packet of this type is received from a client. In this case, I moved the emoji handler out of the Handler just for emoji, since it seemed like an obvious case. The rule of thumb is simple: if something is illegal per se, there should be no possible way in the vanilla client to achieve this result. If a player sends this combination of packets they *must* be hacking. Not that there is a 99.9% chance they're hacking, but that there is a 100% unambiguous chance that they're hacking. Something is illegal per se if it can only be created by a hacked client. If there's a crashing bug that a normal player can do with a complex series of vanilla events, that is not illegal per se. The goal of this namespace and class of handlers is to handle exactly one type of protocol violation, and remove the packet accordingly. If it is ever reported that the packet can be sent from a vanilla client, the check must be removed as it is no longer a per se violation of the protocol. --- CHANGELOG.md | 1 + TShockAPI/Bouncer.cs | 4 +++ TShockAPI/Handlers/EmojiHandler.cs | 11 ++------ .../IllegalPerSe/EmojiPlayerMismatch.cs | 25 +++++++++++++++++++ TShockAPI/TShockAPI.csproj | 3 ++- 5 files changed, 34 insertions(+), 10 deletions(-) create mode 100644 TShockAPI/Handlers/IllegalPerSe/EmojiPlayerMismatch.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 246db4f5..92cf8760 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Fixed torchgod settings to include whether or not torchgod has been fought by the player before and respect `usingBiomeTorches` setting. (@Quinci135) * Fixed /worldmode not synchronising data to players after updating the world state (@bartico6, @Arthri) * Added `OnSendNetData` hook to TSAPI, which enables developers to intercept traffic being sent from the server to clients. (@Stealownz) +* Moved the emoji player index check into a new class of handlers called `IllegalPerSe`, which is designed to help isolate parts of TShock and make it so that "protocol violations" are treated separately from heuristic based anti-cheat checks. (@hakusaro) ## TShock 4.5.3 * Added permissions for using Teleportation Potions, Magic Conch, and Demon Conch. (@drunderscore) diff --git a/TShockAPI/Bouncer.cs b/TShockAPI/Bouncer.cs index ed4c031a..5a6e6307 100644 --- a/TShockAPI/Bouncer.cs +++ b/TShockAPI/Bouncer.cs @@ -39,6 +39,7 @@ namespace TShockAPI internal Handlers.SendTileRectHandler STSHandler { get; set; } internal Handlers.NetModules.NetModulePacketHandler NetModuleHandler { get; set; } internal Handlers.EmojiHandler EmojiHandler { get; set; } + internal Handlers.IllegalPerSe.EmojiPlayerMismatch EmojiPlayerMismatch { get; set; } internal Handlers.DisplayDollItemSyncHandler DisplayDollItemSyncHandler { get; set; } internal Handlers.RequestTileEntityInteractionHandler RequestTileEntityInteractionHandler { get; set; } internal Handlers.LandGolfBallInCupHandler LandGolfBallInCupHandler { get; set; } @@ -54,6 +55,9 @@ namespace TShockAPI NetModuleHandler = new Handlers.NetModules.NetModulePacketHandler(); GetDataHandlers.ReadNetModule += NetModuleHandler.OnReceive; + EmojiPlayerMismatch = new Handlers.IllegalPerSe.EmojiPlayerMismatch(); + GetDataHandlers.Emoji += EmojiPlayerMismatch.OnReceive; + EmojiHandler = new Handlers.EmojiHandler(); GetDataHandlers.Emoji += EmojiHandler.OnReceive; diff --git a/TShockAPI/Handlers/EmojiHandler.cs b/TShockAPI/Handlers/EmojiHandler.cs index f32993d1..ab0fb92e 100644 --- a/TShockAPI/Handlers/EmojiHandler.cs +++ b/TShockAPI/Handlers/EmojiHandler.cs @@ -3,24 +3,17 @@ namespace TShockAPI.Handlers { /// - /// Handles emoji packets and checks for validity and permissions + /// Handles emoji packets and checks for permissions /// public class EmojiHandler : IPacketHandler { /// - /// Invoked when an emoji is sent in chat. Rejects the emoji packet if the player is spoofing IDs or does not have emoji permissions + /// Invoked when an emoji is sent in chat. Rejects the emoji packet if the player does not have emoji permissions /// /// /// public void OnReceive(object sender, EmojiEventArgs args) { - if (args.PlayerIndex != args.Player.Index) - { - TShock.Log.ConsoleError($"EmojiHandler: Emoji packet rejected for ID spoofing. Expected {args.Player.Index}, received {args.PlayerIndex} from {args.Player.Name}."); - args.Handled = true; - return; - } - if (!args.Player.HasPermission(Permissions.sendemoji)) { args.Player.SendErrorMessage("You do not have permission to send emotes!"); diff --git a/TShockAPI/Handlers/IllegalPerSe/EmojiPlayerMismatch.cs b/TShockAPI/Handlers/IllegalPerSe/EmojiPlayerMismatch.cs new file mode 100644 index 00000000..79fb6c9b --- /dev/null +++ b/TShockAPI/Handlers/IllegalPerSe/EmojiPlayerMismatch.cs @@ -0,0 +1,25 @@ +using static TShockAPI.GetDataHandlers; + +namespace TShockAPI.Handlers.IllegalPerSe +{ + /// + /// Rejects emoji packets with mismatched identifiers + /// + public class EmojiPlayerMismatch : IPacketHandler + { + /// + /// Invoked on emoji send. Rejects packets that are impossible. + /// + /// + /// + public void OnReceive(object sender, EmojiEventArgs args) + { + if (args.PlayerIndex != args.Player.Index) + { + TShock.Log.ConsoleError($"IllegalPerSe: Emoji packet rejected for ID spoofing. Expected {args.Player.Index}, received {args.PlayerIndex} from {args.Player.Name}."); + args.Handled = true; + return; + } + } + } +} diff --git a/TShockAPI/TShockAPI.csproj b/TShockAPI/TShockAPI.csproj index add34f1e..59a7b9a4 100644 --- a/TShockAPI/TShockAPI.csproj +++ b/TShockAPI/TShockAPI.csproj @@ -106,6 +106,7 @@ + @@ -239,4 +240,4 @@ --> - \ No newline at end of file +