From 67d899efe63d0a4118cd4311dfde258143e454cc Mon Sep 17 00:00:00 2001 From: dongresource Date: Fri, 28 Aug 2020 16:09:26 +0200 Subject: [PATCH] Implemented proper validation of variable-length packets. Also changed output buffer in pcAttackNpcs() from dynamically to statically allocated. This in itself is temporary as I have a better idea as to how we can allocate buffers with a bit less boilerplate. --- src/CNProtocol.cpp | 2 +- src/CNProtocol.hpp | 42 +++++++++++++++++++++++++++++++++++++++--- src/CombatManager.cpp | 25 +++++++++++++++---------- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/CNProtocol.cpp b/src/CNProtocol.cpp index 097d333..fd568b8 100644 --- a/src/CNProtocol.cpp +++ b/src/CNProtocol.cpp @@ -170,7 +170,7 @@ void CNSocket::step() { // we got out packet size!!!! readSize = *((int32_t*)readBuffer); // sanity check - if (readSize > MAX_PACKETSIZE) { + if (readSize > CN_PACKET_BUFFER_SIZE) { kill(); return; } diff --git a/src/CNProtocol.hpp b/src/CNProtocol.hpp index d2f9843..b24959b 100644 --- a/src/CNProtocol.hpp +++ b/src/CNProtocol.hpp @@ -1,6 +1,5 @@ #pragma once -#define MAX_PACKETSIZE 8192 #define DEBUGLOG(x) if (settings::VERBOSITY) {x}; #include @@ -56,7 +55,8 @@ [4 bytes] - size of packet including the 4 byte packet type [size bytes] - Encrypted packet (byte swapped && xor'd with 8 byte key; see CNSocketEncryption) [4 bytes] - packet type (which is a combination of the first 4 bytes of the packet and a checksum in some versions) - [structure] + [structure] - one member contains length of trailing data (expressed in packet-dependant structures) + [trailing data] - optional variable-length data that only some packets make use of */ // error checking calloc wrapper @@ -71,6 +71,42 @@ inline void* xmalloc(size_t sz) { return res; } +// overflow-safe validation of variable-length packets +// for outbound packets +inline bool validOutVarPacket(size_t base, int32_t npayloads, size_t plsize) { + // check for multiplication overflow + if (npayloads > 0 && CN_PACKET_BUFFER_SIZE / (size_t)npayloads < plsize) + return false; + + // it's safe to multiply + size_t trailing = npayloads * plsize; + + // does it fit in a packet? + if (base + trailing <= CN_PACKET_BUFFER_SIZE) + return false; + + // everything is a-ok! + return true; +} + +// for inbound packets +inline bool validInVarPacket(size_t base, int32_t npayloads, size_t plsize, size_t datasize) { + // check for multiplication overflow + if (npayloads > 0 && CN_PACKET_BUFFER_SIZE / (size_t)npayloads < plsize) + return false; + + // it's safe to multiply + size_t trailing = npayloads * plsize; + + // make sure size is exact + // datasize has already been validated against CN_PACKET_BUFFER_SIZE + if (datasize != base + trailing) + return false; + + // everything is a-ok! + return true; +} + namespace CNSocketEncryption { // you won't believe how complicated they made it in the client :facepalm: static constexpr const char* defaultKey = "m@rQn~W#"; @@ -104,7 +140,7 @@ private: uint64_t EKey; uint64_t FEKey; int32_t readSize = 0; - uint8_t readBuffer[MAX_PACKETSIZE]; + uint8_t readBuffer[CN_PACKET_BUFFER_SIZE]; int readBufferIndex = 0; bool activelyReading = false; bool alive = true; diff --git a/src/CombatManager.cpp b/src/CombatManager.cpp index 73fde1d..845f879 100644 --- a/src/CombatManager.cpp +++ b/src/CombatManager.cpp @@ -13,22 +13,31 @@ void CombatManager::init() { } void CombatManager::pcAttackNpcs(CNSocket *sock, CNPacketData *data) { - // generic malformed packet checks are not applicable to variable-length packets sP_CL2FE_REQ_PC_ATTACK_NPCs* pkt = (sP_CL2FE_REQ_PC_ATTACK_NPCs*)data->buf; - if (data->size != sizeof(sP_CL2FE_REQ_PC_ATTACK_NPCs) + pkt->iNPCCnt * 4) { - std::cout << "bad sP_CL2FE_REQ_PC_ATTACK_NPCs packet size\n"; + // sanity check + if (!validInVarPacket(sizeof(sP_CL2FE_REQ_PC_ATTACK_NPCs), pkt->iNPCCnt, sizeof(int32_t), data->size)) { + std::cout << "[WARN] bad sP_CL2FE_REQ_PC_ATTACK_NPCs packet size\n"; return; } + int32_t *pktdata = (int32_t*)((uint8_t*)data->buf + sizeof(sP_CL2FE_REQ_PC_ATTACK_NPCs)); - std::printf("iNPCCnt: %d\n", pkt->iNPCCnt); + /* + * Due to the possibility of multiplication overflow (and regular buffer overflow), + * both incoming and outgoing variable-length packets must be validated. + */ + if (!validOutVarPacket(sizeof(sP_FE2CL_PC_ATTACK_NPCs_SUCC), pkt->iNPCCnt, sizeof(sAttackResult))) { + std::cout << "[WARN] bad sP_CL2FE_REQ_PC_ATTACK_NPCs packet size\n"; + return; + } // initialize response struct - // IMPORTANT TODO: verify that resplen doesn't overflow!!! size_t resplen = sizeof(sP_FE2CL_PC_ATTACK_NPCs_SUCC) + pkt->iNPCCnt * sizeof(sAttackResult); - uint8_t *respbuf = (uint8_t*)xmalloc(resplen); + uint8_t respbuf[4096]; + memset(respbuf, 0, resplen); + sP_FE2CL_PC_ATTACK_NPCs_SUCC *resp = (sP_FE2CL_PC_ATTACK_NPCs_SUCC*)respbuf; sAttackResult *respdata = (sAttackResult*)(respbuf+sizeof(sP_FE2CL_PC_ATTACK_NPCs_SUCC)); @@ -45,7 +54,6 @@ void CombatManager::pcAttackNpcs(CNSocket *sock, CNPacketData *data) { BaseNPC& mob = NPCManager::NPCs[pktdata[i]]; mob.appearanceData.iHP -= 100; - std::cout << "mob health is now " << mob.appearanceData.iHP << std::endl; if (mob.appearanceData.iHP <= 0) giveReward(sock); @@ -56,10 +64,7 @@ void CombatManager::pcAttackNpcs(CNSocket *sock, CNPacketData *data) { respdata[i].iHitFlag = 2; } - std::cout << "sending packet of length " << resplen << std::endl; sock->sendPacket((void*)respbuf, P_FE2CL_PC_ATTACK_NPCs_SUCC, resplen); - - free(respbuf); } void CombatManager::combatBegin(CNSocket *sock, CNPacketData *data) {} // stub