[Hacking] ChipNBTSet packet
TheAndrey opened this issue ยท 10 comments
I found unsafe packet handler.
https://github.com/MrTJP/ProjectRed/blob/master/src/mrtjp/projectred/transportation/packethandlers.scala#L178-L179
private def setChipNBT(packet:PacketCustom, player:EntityPlayerMP)
{
val slot = packet.readUByte()
val stack = packet.readItemStack()
player.inventory.setInventorySlotContents(slot, stack)
player.inventory.markDirty()
}
This puts received from client item to player inventory. Hackers can use this to get ANY item on the server!
You can never trust data that you receive from a client. You should always check what you have received from the client.
In this case, you only need to transfer the NBT tag, but not the entire item.
@TheAndrey, I can say that developer doesn`t think about servers, because I finded this bug an year ago, but then I join into discord i was kicked.
ProjectRed doesn't have a discord, I've added this to my list and will check it out this weekend.
Since i believe there won't be a patch soon and i don't have that good of a knowledge of how to fix it could you tell me how you did it?
I apparently might have a fix just have no idea of how to use it
The current code
https://github.com/MrTJP/ProjectRed/blob/master/src/mrtjp/projectred/transportation/packethandlers.scala#L178-L179
GitHub
MrTJP/ProjectRed
ProjectRed - Extending redstone functionality to FMP
Lines 178 and 179
It can be fixed by replacing line's 175 below with this code
private def setChipNBT(packet:PacketCustom, player:EntityPlayerMP)
{
val slot = packet.readUByte()
val stack = packet.readItemStack()
val playerstack = player.inventory.getStackInSlot(slot)
if (stack.getUnlocalizedName == playerstack.getUnlocalizedName)
{
player.inventory.setInventorySlotContents(slot, stack)
player.inventory.markDirty()
}
else
{
player.addChatMessage(new ChatComponentText("YOU DIRTY HACKER"))
}
}
@man-of-stone50 this fix is not safe. Hackers can write any NBT tag to item (like item name, lore, enchantments). Also using getUnlocalizedName() is not good idea, use getItem()
Oh damn that sounds bad, we might have to remove ProjectRed Transportation temporarily.
@MrTJP Will the exploit be fixed?