diff --git a/src/common/network/PacketBuffer.cpp b/src/common/network/PacketBuffer.cpp index 7adc1172..b2a107fc 100644 --- a/src/common/network/PacketBuffer.cpp +++ b/src/common/network/PacketBuffer.cpp @@ -16,13 +16,14 @@ using namespace network; using namespace compress; #include +#include // --------------------------------------------------------------------------- // Constants // --------------------------------------------------------------------------- #define MAX_FRAGMENT_SIZE 8192 * 1024 // 8MB max -#define MAX_BLOCK_CNT 254 +#define MAX_BLOCK_CNT 256U // --------------------------------------------------------------------------- // Public Class Members @@ -50,6 +51,7 @@ PacketBuffer::~PacketBuffer() bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outLength) { assert(data != nullptr); + assert(message != nullptr); *message = nullptr; if (outLength != nullptr) { @@ -59,6 +61,12 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL uint8_t curBlock = data[8U]; uint8_t blockCnt = data[9U]; + if (curBlock > blockCnt) { + LogError(LOG_NET, "%s, invalid fragment block %u of %u", m_name, curBlock, blockCnt); + clear(); + return false; + } + Fragment* frag = new Fragment(); frag->compressedSize = 0U; frag->size = 0U; @@ -73,9 +81,31 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL // make sure we can't exceed max fragment size -- prevent potential DOS attack by sending // enormous fragment sizes - if (size > MAX_FRAGMENT_SIZE || compressedSize > MAX_FRAGMENT_SIZE) { + if (size == 0U || compressedSize == 0U || size > MAX_FRAGMENT_SIZE || compressedSize > MAX_FRAGMENT_SIZE || + compressedSize > (MAX_BLOCK_CNT * FRAG_BLOCK_SIZE)) { LogError(LOG_NET, "%s, fragment size exceeds maximum. BUGBUG.", m_name); delete frag; + clear(); + return false; + } + + uint32_t expectedBlocks = (compressedSize + FRAG_BLOCK_SIZE - 1U) / FRAG_BLOCK_SIZE; + if (expectedBlocks != (uint32_t)blockCnt + 1U) { + LogError(LOG_NET, "%s, fragment block count mismatch, expected %u, got %u", m_name, + expectedBlocks, (uint32_t)blockCnt + 1U); + delete frag; + clear(); + return false; + } + + // Nonzero blocks may arrive before block zero. Reject the transfer if + // accepting block zero would leave more fragments than its metadata + // declares, rather than retaining an assembly that can never finish. + if (fragments.size() >= expectedBlocks) { + LogError(LOG_NET, "%s, fragment count exceeds block zero metadata, expected %u, already buffered %u", + m_name, expectedBlocks, fragments.size()); + delete frag; + clear(); return false; } @@ -90,6 +120,14 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL ::memcpy(frag->data, data + FRAG_HDR_SIZE, FRAG_BLOCK_SIZE); // Utils::dump(1U, "PacketBuffer::decode(), Block Payload", frag->data, FRAG_BLOCK_SIZE); + + if (fragments.find(curBlock) != fragments.end()) { + LogWarning(LOG_NET, "%s, duplicate fragment block %u ignored", m_name, curBlock); + delete[] frag->data; + delete frag; + return false; + } + fragments.insert(curBlock, frag); } @@ -132,6 +170,15 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL return false; } + uint32_t expectedBlocks = (compressedLen + FRAG_BLOCK_SIZE - 1U) / FRAG_BLOCK_SIZE; + if (fragments.size() != expectedBlocks || expectedBlocks != (uint32_t)blockCnt + 1U) { + LogError(LOG_NET, "%s, Packet Fragment, inconsistent block count, expected %u, got %u", m_name, + expectedBlocks, fragments.size()); + fragments.unlock(); + clear(); + return false; + } + // Reassembly buffer must be compressed length, not uncompressed length. DECLARE_UINT8_ARRAY(buffer, compressedLen + 1U); ::memset(buffer, 0x00U, compressedLen + 1U); @@ -153,9 +200,7 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL if (offs >= compressedLen) break; - uint32_t copyLen = FRAG_BLOCK_SIZE; - if (offs + FRAG_BLOCK_SIZE > compressedLen) - copyLen = compressedLen - offs; + uint32_t copyLen = std::min(FRAG_BLOCK_SIZE, compressedLen - offs); ::memcpy(buffer + offs, it->second->data, copyLen); } @@ -185,8 +230,21 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL clear(); return true; } + + delete[] *message; + *message = nullptr; + LogError(LOG_NET, "%s, Packet Fragment, decompressed size mismatch, expected %u, got %u", m_name, + len, decompressedLen); } else { + if (compressedLen != len) { + LogError(LOG_NET, "%s, Packet Fragment, uncompressed size mismatch, expected %u, got %u", m_name, + len, compressedLen); + fragments.unlock(); + clear(); + return false; + } + uint8_t* outBuf = new uint8_t[len + 1U]; ::memset(outBuf, 0x00U, len + 1U); ::memcpy(outBuf, buffer, len); @@ -202,6 +260,7 @@ bool PacketBuffer::decode(const uint8_t* data, uint8_t** message, uint32_t* outL } fragments.unlock(); + clear(); } return false; @@ -230,19 +289,23 @@ bool PacketBuffer::encode(uint8_t* data, uint32_t length) } // create packet fragments - uint8_t blockCnt = (compressedLen / FRAG_BLOCK_SIZE) + (compressedLen % FRAG_BLOCK_SIZE ? 1U : 0U); - if (blockCnt > MAX_BLOCK_CNT) { + uint32_t blockCnt = (compressedLen + FRAG_BLOCK_SIZE - 1U) / FRAG_BLOCK_SIZE; + if (blockCnt == 0U || blockCnt > MAX_BLOCK_CNT) { LogError(LOG_NET, "%s, Outbound Packet Fragment, too many packet fragments, %u (max %u)", m_name, blockCnt, MAX_BLOCK_CNT); return false; } uint32_t offs = 0U; - for (uint8_t i = 0U; i < blockCnt; i++) { + for (uint32_t i = 0U; i < blockCnt; i++) { // build dataset uint16_t bufSize = FRAG_SIZE; Fragment* frag = new Fragment(); - frag->blockId = i; + frag->compressedSize = 0U; + frag->size = 0U; + frag->blockSize = 0U; + frag->blockId = (uint8_t)i; + frag->data = nullptr; frag->data = new uint8_t[bufSize]; ::memset(frag->data, 0x00U, bufSize); @@ -254,8 +317,8 @@ bool PacketBuffer::encode(uint8_t* data, uint32_t length) frag->compressedSize = compressedLen; } - frag->data[8U] = i; - frag->data[9U] = blockCnt - 1U; + frag->data[8U] = (uint8_t)i; + frag->data[9U] = (uint8_t)(blockCnt - 1U); uint32_t blockSize = FRAG_BLOCK_SIZE; if (offs + FRAG_BLOCK_SIZE > compressedLen) @@ -265,7 +328,7 @@ bool PacketBuffer::encode(uint8_t* data, uint32_t length) offs += FRAG_BLOCK_SIZE; - fragments.insert(i, frag); + fragments.insert((uint8_t)i, frag); LogInfoEx(LOG_NET, "%s, Outbound Packet Fragment, block %u of %u, txFragments = %u", m_name, i, blockCnt - 1U, fragments.size()); } diff --git a/tests/network/PacketBuffer_Tests.cpp b/tests/network/PacketBuffer_Tests.cpp index 417578e4..aa7c6071 100644 --- a/tests/network/PacketBuffer_Tests.cpp +++ b/tests/network/PacketBuffer_Tests.cpp @@ -96,6 +96,95 @@ TEST_CASE("PacketBuffer reassembles uncompressed fragments in order", "[network] REQUIRE(encoder.fragments.size() == 0U); } +TEST_CASE("PacketBuffer reassembles fragments received out of order", "[network][packetbuffer]") +{ + PacketBuffer encoder(false, "packetbuffer-out-of-order"); + std::vector payload(FRAG_BLOCK_SIZE * 2U + 73U); + for (size_t i = 0U; i < payload.size(); i++) + payload[i] = (uint8_t)((i * 17U) & 0xFFU); + + REQUIRE(encoder.encode(payload.data(), (uint32_t)payload.size())); + auto encodedFragments = fragmentsInOrder(encoder); + REQUIRE(encodedFragments.size() == 3U); + + PacketBuffer decoder(false, "packetbuffer-out-of-order"); + uint8_t* message = nullptr; + uint32_t messageLength = 0U; + + REQUIRE_FALSE(decoder.decode(encodedFragments[2U]->data, &message, &messageLength)); + REQUIRE(message == nullptr); + REQUIRE(decoder.fragments.size() == 1U); + + REQUIRE_FALSE(decoder.decode(encodedFragments[0U]->data, &message, &messageLength)); + REQUIRE(message == nullptr); + REQUIRE(decoder.fragments.size() == 2U); + + REQUIRE(decoder.decode(encodedFragments[1U]->data, &message, &messageLength)); + REQUIRE(message != nullptr); + REQUIRE(messageLength == payload.size()); + REQUIRE(std::memcmp(message, payload.data(), payload.size()) == 0); + delete[] message; + REQUIRE(decoder.fragments.empty()); +} + +TEST_CASE("PacketBuffer ignores duplicate fragments without corrupting assembly", "[network][packetbuffer]") +{ + PacketBuffer encoder(false, "packetbuffer-duplicate"); + std::vector payload(FRAG_BLOCK_SIZE * 2U + 19U, 0x6BU); + + REQUIRE(encoder.encode(payload.data(), (uint32_t)payload.size())); + auto encodedFragments = fragmentsInOrder(encoder); + REQUIRE(encodedFragments.size() == 3U); + + PacketBuffer decoder(false, "packetbuffer-duplicate"); + uint8_t* message = nullptr; + uint32_t messageLength = 0U; + + REQUIRE_FALSE(decoder.decode(encodedFragments[1U]->data, &message, &messageLength)); + REQUIRE(decoder.fragments.size() == 1U); + + REQUIRE_FALSE(decoder.decode(encodedFragments[1U]->data, &message, &messageLength)); + REQUIRE(message == nullptr); + REQUIRE(messageLength == 0U); + REQUIRE(decoder.fragments.size() == 1U); + + REQUIRE_FALSE(decoder.decode(encodedFragments[0U]->data, &message, &messageLength)); + REQUIRE(decoder.decode(encodedFragments[2U]->data, &message, &messageLength)); + REQUIRE(message != nullptr); + REQUIRE(messageLength == payload.size()); + REQUIRE(std::memcmp(message, payload.data(), payload.size()) == 0); + delete[] message; + REQUIRE(decoder.fragments.empty()); +} + +TEST_CASE("PacketBuffer rejects more fragments than block zero declares", "[network][packetbuffer]") +{ + PacketBuffer encoder(false, "packetbuffer-excess-source"); + std::vector payload(FRAG_BLOCK_SIZE * 2U + 11U, 0xA7U); + + REQUIRE(encoder.encode(payload.data(), (uint32_t)payload.size())); + auto encodedFragments = fragmentsInOrder(encoder); + REQUIRE(encodedFragments.size() == 3U); + + PacketBuffer decoder(false, "packetbuffer-excess"); + uint8_t* message = nullptr; + uint32_t messageLength = 0U; + + REQUIRE_FALSE(decoder.decode(encodedFragments[1U]->data, &message, &messageLength)); + REQUIRE_FALSE(decoder.decode(encodedFragments[2U]->data, &message, &messageLength)); + REQUIRE(decoder.fragments.size() == 2U); + + std::array blockZero = {}; + std::memcpy(blockZero.data(), encodedFragments[0U]->data, FRAG_SIZE); + setFragmentHeader(blockZero.data(), FRAG_BLOCK_SIZE * 2U, + FRAG_BLOCK_SIZE * 2U, 0U, 1U); + + REQUIRE_FALSE(decoder.decode(blockZero.data(), &message, &messageLength)); + REQUIRE(message == nullptr); + REQUIRE(messageLength == 0U); + REQUIRE(decoder.fragments.empty()); +} + TEST_CASE("PacketBuffer reassembles compressed fragments", "[network][packetbuffer]") { PacketBuffer encoder(true, "packetbuffer-compressed"); @@ -161,7 +250,7 @@ TEST_CASE("PacketBuffer rejects invalid ZLIB data", "[network][packetbuffer]") TEST_CASE("PacketBuffer enforces the maximum block count", "[network][packetbuffer]") { - constexpr uint32_t maxBlockCount = 254U; + constexpr uint32_t maxBlockCount = 256U; SECTION("accepts exactly the maximum number of blocks") { PacketBuffer buffer(false, "packetbuffer-max-blocks");