From 69f232b12a5aa4cc6e0b6621bb155f2f744eff14 Mon Sep 17 00:00:00 2001 From: Patrikkk Date: Fri, 19 Jun 2020 14:07:39 +0200 Subject: [PATCH 1/6] Adding DisplayDollItemSync event. An event that is called when a player modifies the slot of a DisplayDoll (Mannequin). I was trying to think from a developer friendly perspective here. Instead of passing seperate variables for type/stack/prefix, thought I pass an Item object. As well as, instead of having devs who work with this hook figure out and implement how to get the Item of the DisplayDoll, I just provide it in the hook. I can imagine this being used for creative purposes in plugins. --- CHANGELOG.md | 1 + TShockAPI/GetDataHandlers.cs | 99 +++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45456eb2..790b6e71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Cleaned up a check in Bouner OnTileEdit where it checks for using the respective item when placing a tile to make it clearer. This change also fixed the issue in a previous commit where valid replace action was caught. Moved the check for max tile/wall types to the beginning of the method. (@Patrikkk) * Improved clarity for insufficient permission related error messages. (@moisterrific) * Remove redundant Boulder placement check that prevented placing chests on them, as it is no longer possible to place a chest on a boulder, so nothing crashes the server. "1.2.3: Boulders with Chests on them no longer crash the game if the boulder is hit." (@kevzhao2, @Patrikkk) +* Adding DisplayDollItemSync event. An event that is called when a player modifies the slot of a DisplayDoll (Mannequin). This event provides information about the current item in the displaydoll, as well as the item that the player is about to set. (@Patrikkk) ## TShock 4.4.0 (Pre-release 11) * New permission `tshock.tp.pylon` to enable teleporting via Teleportation Pylons (@QuiCM) diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index 4a1c90fd..a98385e4 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -152,6 +152,7 @@ namespace TShockAPI { PacketTypes.PlayerHurtV2, HandlePlayerDamageV2 }, { PacketTypes.PlayerDeathV2, HandlePlayerKillMeV2 }, { PacketTypes.Emoji, HandleEmoji }, + { PacketTypes.TileEntityDisplayDollItemSync, HandleTileEntityDisplayDollItemSync }, { PacketTypes.SyncTilePicking, HandleSyncTilePicking }, { PacketTypes.SyncRevengeMarker, HandleSyncRevengeMarker }, { PacketTypes.LandGolfBallInCup, HandleLandGolfBallInCup }, @@ -1945,7 +1946,60 @@ namespace TShockAPI Emoji.Invoke(null, args); return args.Handled; } - + + /// For use in a TileEntityDisplayDollItemSync event. + /// + public class DisplayDollItemSyncEventArgs : GetDataHandledEventArgs + { + /// + /// The player index in the packet who modifies the DisplayDoll item slot. + /// + public byte PlayerIndex { get; set; } + /// + /// The ID of the TileEntity that is being modified. + /// + public int TileEntityID { get; set; } + /// + /// The slot of the DisplayDoll that is being modified. + /// + public int Slot { get; set; } + /// + /// Wether or not the slot that is being modified is a Dye slot. + /// + public bool IsDye { get; set; } + /// + /// The current item that is present in the slot before the modification. + /// + public Item OldItem { get; set; } + /// + /// The item that is about to replace the OldItem in the slot that is being modified. + /// + public Item NewItem { get; set; } + } + /// + /// Called when a player modifies a DisplayDoll (Mannequin) item slot. + /// + public static HandlerList DisplayDollItemSync = new HandlerList(); + private static bool OnDisplayDollItemSync(TSPlayer player, MemoryStream data, byte playerIndex, int tileEntityID, int slot, bool isDye, Item oldItem, Item newItem) + { + if (DisplayDollItemSync == null) + return false; + + var args = new DisplayDollItemSyncEventArgs + { + Player = player, + Data = data, + PlayerIndex = playerIndex, + TileEntityID = tileEntityID, + Slot = slot, + IsDye = isDye, + OldItem = oldItem, + NewItem = newItem + }; + DisplayDollItemSync.Invoke(null, args); + return args.Handled; + } + /// /// For use in a LandBallInCup event. /// @@ -3703,6 +3757,49 @@ namespace TShockAPI return false; } + private static bool HandleTileEntityDisplayDollItemSync(GetDataHandlerArgs args) + { + byte playerIndex = args.Data.ReadInt8(); + int tileEntityID = args.Data.ReadInt32(); + int slot = args.Data.ReadByte(); + bool isDye = false; + if (slot >= 8) + { + isDye = true; + slot -= 8; + } + + Item newItem = new Item(); + Item oldItem = new Item(); + + if (!TileEntity.ByID.TryGetValue(tileEntityID, out TileEntity tileEntity)) + return false; + + TEDisplayDoll displayDoll = tileEntity as TEDisplayDoll; + if (displayDoll != null) + { + oldItem = displayDoll._items[slot]; + if (isDye) + oldItem = displayDoll._dyes[slot]; + + ushort itemType = args.Data.ReadUInt16(); + ushort stack = args.Data.ReadUInt16(); + int prefix = args.Data.ReadByte(); + + newItem.SetDefaults(itemType); + newItem.stack = stack; + newItem.Prefix(prefix); + + if (oldItem.type == 0 && newItem.type == 0) + return false; + } + + if (OnDisplayDollItemSync(args.Player, args.Data, playerIndex, tileEntityID, slot, isDye, oldItem, newItem)) + return true; + + return false; + } + private static bool HandleSyncTilePicking(GetDataHandlerArgs args) { byte playerIndex = args.Data.ReadInt8(); From 408eaf4383a117050e8961c808594d6fe895c179 Mon Sep 17 00:00:00 2001 From: Patrikkk Date: Fri, 19 Jun 2020 14:16:27 +0200 Subject: [PATCH 2/6] Add TEDisplayDoll object to the event args. --- TShockAPI/GetDataHandlers.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index a98385e4..08b851df 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -1960,6 +1960,10 @@ namespace TShockAPI /// public int TileEntityID { get; set; } /// + /// The TEDisplayDoll object that is being modified. + /// + public TEDisplayDoll DisplayDollEntity { get; set; } + /// /// The slot of the DisplayDoll that is being modified. /// public int Slot { get; set; } @@ -1980,7 +1984,7 @@ namespace TShockAPI /// Called when a player modifies a DisplayDoll (Mannequin) item slot. /// public static HandlerList DisplayDollItemSync = new HandlerList(); - private static bool OnDisplayDollItemSync(TSPlayer player, MemoryStream data, byte playerIndex, int tileEntityID, int slot, bool isDye, Item oldItem, Item newItem) + private static bool OnDisplayDollItemSync(TSPlayer player, MemoryStream data, byte playerIndex, int tileEntityID, TEDisplayDoll displayDollEntity, int slot, bool isDye, Item oldItem, Item newItem) { if (DisplayDollItemSync == null) return false; @@ -1991,6 +1995,7 @@ namespace TShockAPI Data = data, PlayerIndex = playerIndex, TileEntityID = tileEntityID, + DisplayDollEntity = displayDollEntity, Slot = slot, IsDye = isDye, OldItem = oldItem, @@ -3794,7 +3799,7 @@ namespace TShockAPI return false; } - if (OnDisplayDollItemSync(args.Player, args.Data, playerIndex, tileEntityID, slot, isDye, oldItem, newItem)) + if (OnDisplayDollItemSync(args.Player, args.Data, playerIndex, tileEntityID, displayDoll, slot, isDye, oldItem, newItem)) return true; return false; From 64e61b8ed9a3e948dcc91701a4c476829570b5bb Mon Sep 17 00:00:00 2001 From: Patrikkk Date: Fri, 19 Jun 2020 14:21:45 +0200 Subject: [PATCH 3/6] Add DisplayDollItemSyncHandler In a previous PR I have added the tile entity request packet handler which checks for building permissions to prevent the unauthorized player to open a DisplayDoll and see its content. This Handler is being added to prevent *Hackers* from modifying a DisplayDoll through direct/crafted packet sending, or by sending raw byte data to the server. In a valid enviroment, the player couldn't even get to see the content of the doll in the first place, to then try to modify it's items. Because of this, I do not bother with making sure the player gets their item back. --- .../Handlers/DisplayDollItemSyncHandler.cs | 27 +++++++++++++++++++ TShockAPI/TShockAPI.csproj | 1 + 2 files changed, 28 insertions(+) create mode 100644 TShockAPI/Handlers/DisplayDollItemSyncHandler.cs diff --git a/TShockAPI/Handlers/DisplayDollItemSyncHandler.cs b/TShockAPI/Handlers/DisplayDollItemSyncHandler.cs new file mode 100644 index 00000000..71cdc2eb --- /dev/null +++ b/TShockAPI/Handlers/DisplayDollItemSyncHandler.cs @@ -0,0 +1,27 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using static TShockAPI.GetDataHandlers; + +namespace TShockAPI.Handlers +{ + /// + /// Handles the TileEntityDisplayDollItemSync packets and checks for permissions. + /// + public class DisplayDollItemSyncHandler : IPacketHandler + { + public void OnReceive(object sender, DisplayDollItemSyncEventArgs args) + { + /// If the player has no building permissions means that they couldn't even see the content of the doll in the first place. + /// Thus, they would not be able to modify its content. This means that a hacker attempted to send this packet directly, or through raw bytes to tamper with the DisplayDoll. This is why I do not bother with making sure the player gets their item back. + if (!args.Player.HasBuildPermission(args.DisplayDollEntity.Position.X, args.DisplayDollEntity.Position.Y, false)) + { + args.Player.SendErrorMessage("You do not have permission to modify a Mannequin in a protected area!"); + args.Handled = true; + return; + } + } + } +} diff --git a/TShockAPI/TShockAPI.csproj b/TShockAPI/TShockAPI.csproj index f9885393..3afe3d0d 100644 --- a/TShockAPI/TShockAPI.csproj +++ b/TShockAPI/TShockAPI.csproj @@ -88,6 +88,7 @@ + From 10a9ee399adc9a021e0343109e47a458c56de52e Mon Sep 17 00:00:00 2001 From: Patrikkk Date: Fri, 19 Jun 2020 14:26:25 +0200 Subject: [PATCH 4/6] Update CHANGELOG.md Loggin latest change of adding DisplayDollItemSyncHandler. Updating missing author for previous changes. --- CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 790b6e71..9b367253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,13 +27,14 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Improved clarity for insufficient permission related error messages. (@moisterrific) * Remove redundant Boulder placement check that prevented placing chests on them, as it is no longer possible to place a chest on a boulder, so nothing crashes the server. "1.2.3: Boulders with Chests on them no longer crash the game if the boulder is hit." (@kevzhao2, @Patrikkk) * Adding DisplayDollItemSync event. An event that is called when a player modifies the slot of a DisplayDoll (Mannequin). This event provides information about the current item in the displaydoll, as well as the item that the player is about to set. (@Patrikkk) +* Adding DisplayDollItemSyncHandler, which checks for building permissions of the player at the position of the DisplayDoll. (If they do not have permissions, it means they are hacking as they could not even open the doll in the first place.) (@Patrikkk) ## TShock 4.4.0 (Pre-release 11) * New permission `tshock.tp.pylon` to enable teleporting via Teleportation Pylons (@QuiCM) * New permission `tshock.journey.research` to enable sharing research via item sacrifice (@QuiCM) -* Add Emoji event to GetDataHandler. This packet is received when a player tries to display an emote. (who?) - * Adding EmojiHandler to handle an exploit. Adding `tshock.sendemoji` permission and checks. Added this permission to guest group by default. (who?) -* Handled SyncCavernMonsterType packet to prevent an exploit where players could modify the server's cavern monster types and make the server spawn any NPCs - including bosses - onto other players. (who?) +* Add Emoji event to GetDataHandler. This packet is received when a player tries to display an emote. (@Patrikkk) + * Adding EmojiHandler to handle an exploit. Adding `tshock.sendemoji` permission and checks. Added this permission to guest group by default. (@Patrikkk) +* Handled SyncCavernMonsterType packet to prevent an exploit where players could modify the server's cavern monster types and make the server spawn any NPCs - including bosses - onto other players. (@Patrikkk) * Added LandGolfBallInCup event which is accessible for developers to work with, as well as LandGolfBallInCup handler to handle exploits where players could send direct packets to trigger and imitate golf ball cup landing anywhere in the game world. Added two public lists in Handlers.LandGolfBallInCupHandler: GolfBallProjectileIDs and GolfClubItemIDs. (@Patrikkk) * Added SyncTilePicking event. This is called when a player damages a tile. Implementing SyncTilePickingHandler and patching tile damaging related exploits. (Preventing player sending invalid world position data which disconnects other players.) * Fixed the issue where mobs could not be fished out during bloodmoon because of Bouncer checks. (@Patrikkk) From 90dd61e668c1a048ceef4d17191819560d5c5ac3 Mon Sep 17 00:00:00 2001 From: Patrikkk Date: Sun, 21 Jun 2020 15:03:08 +0200 Subject: [PATCH 5/6] Moving the DisplayDollItemSync hook inside null check. In crafted (hacked) packet sending, people could send the DisplayDollItemSync packet with an entity ID that is not actually a display doll. This would not happen in a normal scenario. If they send the crafted packet, our hook would have been invoked and it would have contain a null TEDisplayDoll object since we tried to get the TileEntity object as a TEDisplayDoll object. --- TShockAPI/GetDataHandlers.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index 08b851df..02fd32b0 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -3797,11 +3797,10 @@ namespace TShockAPI if (oldItem.type == 0 && newItem.type == 0) return false; + + if (OnDisplayDollItemSync(args.Player, args.Data, playerIndex, tileEntityID, displayDoll, slot, isDye, oldItem, newItem)) + return true; } - - if (OnDisplayDollItemSync(args.Player, args.Data, playerIndex, tileEntityID, displayDoll, slot, isDye, oldItem, newItem)) - return true; - return false; } From 38a1351d38e1d87a7e7a5fef65f29a22798ab584 Mon Sep 17 00:00:00 2001 From: Patrikkk Date: Thu, 25 Jun 2020 01:42:22 +0200 Subject: [PATCH 6/6] Register DisplayDollItemSyncHandler. Moved the type check for 0 before itemSetDefaults to have less code run if the empty slots are clicked. --- TShockAPI/Bouncer.cs | 6 +++++- TShockAPI/GetDataHandlers.cs | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/TShockAPI/Bouncer.cs b/TShockAPI/Bouncer.cs index 86ccb00e..f3814388 100644 --- a/TShockAPI/Bouncer.cs +++ b/TShockAPI/Bouncer.cs @@ -39,6 +39,7 @@ namespace TShockAPI internal Handlers.SendTileSquareHandler STSHandler { get; set; } internal Handlers.NetModules.NetModulePacketHandler NetModuleHandler { get; set; } internal Handlers.EmojiHandler EmojiHandler { get; set; } + internal Handlers.DisplayDollItemSyncHandler DisplayDollItemSyncHandler { get; set; } internal Handlers.LandGolfBallInCupHandler LandGolfBallInCupHandler { get; set; } internal Handlers.SyncTilePickingHandler SyncTilePickingHandler { get; set; } @@ -52,9 +53,12 @@ namespace TShockAPI NetModuleHandler = new Handlers.NetModules.NetModulePacketHandler(); GetDataHandlers.ReadNetModule += NetModuleHandler.OnReceive; + DisplayDollItemSyncHandler = new Handlers.DisplayDollItemSyncHandler(); + GetDataHandlers.DisplayDollItemSync += DisplayDollItemSyncHandler.OnReceive; + EmojiHandler = new Handlers.EmojiHandler(); GetDataHandlers.Emoji += EmojiHandler.OnReceive; - + LandGolfBallInCupHandler = new Handlers.LandGolfBallInCupHandler(); GetDataHandlers.LandGolfBallInCup += LandGolfBallInCupHandler.OnReceive; diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index 02fd32b0..ace294d6 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -3791,13 +3791,13 @@ namespace TShockAPI ushort stack = args.Data.ReadUInt16(); int prefix = args.Data.ReadByte(); + if (oldItem.type == 0 && newItem.type == 0) + return false; + newItem.SetDefaults(itemType); newItem.stack = stack; newItem.Prefix(prefix); - if (oldItem.type == 0 && newItem.type == 0) - return false; - if (OnDisplayDollItemSync(args.Player, args.Data, playerIndex, tileEntityID, displayDoll, slot, isDye, oldItem, newItem)) return true; }