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

@@ -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;
             }

src/CNProtocol.hpp

@@ -1,6 +1,5 @@
 #pragma once
 
-#define MAX_PACKETSIZE 8192
 #define DEBUGLOG(x) if (settings::VERBOSITY) {x};
 
 #include <iostream>
@@ -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;

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) {
+    // sanity check
-        std::cout << "bad sP_CL2FE_REQ_PC_ATTACK_NPCs packet size\n";
+    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