From 8a2ccb1ae7d96379f73463d9745c51506012208b Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Sat, 26 Sep 2026 18:41:58 -0500 Subject: [PATCH] fix: bound AMF3 decoding and stop hashing client keys Client-sent AMF (CONTROL_BEHAVIORS) was decoded without limits on the associative part of arrays, on nesting depth, or on the total number of values. Associative entries went into an unordered_map with the standard unseeded string hash, so a client could pick colliding keys and make insertion quadratic, and deeply nested arrays recursed until the stack overflowed, crashing the world server. - The associative map is now an ordered std::map (O(log n) whatever the keys, deterministic serialization order). - Each array allows at most 10,000 associative entries, the same as the existing dense limit, which is now checked before anything is read. - Arrays may nest at most 32 deep and one deserializer reads at most 100,000 values. Every limit throws, which the only caller already catches and drops the message. - Inserting a duplicate key keeps the last value and no longer returns a reference to a value that was destroyed when the key already held null. Verified with new unit tests for each limit (including a 100,000 deep nesting that previously overflowed the stack) and the existing live packet test, which still decodes. Fixes #2035 Co-Authored-By: Claude Opus 5.5 --- dCommon/AMFDeserialize.cpp | 31 ++++- dCommon/AMFDeserialize.h | 15 +++ dCommon/Amf3.h | 11 +- tests/dCommonTests/AMFDeserializeTests.cpp | 147 +++++++++++++++++++++ 4 files changed, 194 insertions(+), 10 deletions(-) diff --git a/dCommon/AMFDeserialize.cpp b/dCommon/AMFDeserialize.cpp index 9d46b1759..003d1a338 100644 --- a/dCommon/AMFDeserialize.cpp +++ b/dCommon/AMFDeserialize.cpp @@ -11,6 +11,13 @@ */ std::unique_ptr AMFDeserialize::Read(RakNet::BitStream& inStream) { + // Every value counts against the budget for this deserializer, so one message + // cannot make the server build an unbounded tree of values. + if (++m_ValuesRead > MaxValues) { + LOG("AMF value budget of %u exceeded, possible spoof, aborting deserialize.", MaxValues); + throw std::invalid_argument("AMF value budget exceeded"); + } + // Read in the value type from the bitStream eAmf marker; inStream.Read(marker); @@ -112,27 +119,41 @@ std::unique_ptr AMFDeserialize::ReadAmfDouble(RakNet::BitStream& } std::unique_ptr AMFDeserialize::ReadAmfArray(RakNet::BitStream& inStream) { + // Arrays are the only values that nest, so bound the recursion here. + if (m_Depth >= MaxDepth) { + LOG("AMF arrays nested deeper than %u, possible spoof, aborting deserialize.", MaxDepth); + throw std::invalid_argument("AMF arrays nested too deeply"); + } + ++m_Depth; + auto arrayValue = std::make_unique(); // Read size of dense array const auto sizeOfDenseArray = (ReadU29(inStream) >> 1); + if (sizeOfDenseArray > MaxArraySize) { + LOG("Someone sent %u dense array entries, probably a bad packet.", sizeOfDenseArray); + throw std::invalid_argument("Too many dense AMF array entries"); + } + // Then read associative portion + uint32_t associativeEntries = 0; while (true) { const auto key = ReadString(inStream); // No more associative values when we encounter an empty string key if (key.size() == 0) break; + if (++associativeEntries > MaxArraySize) { + LOG("Someone sent more than %u associative array entries, probably a bad packet.", MaxArraySize); + throw std::invalid_argument("Too many associative AMF array entries"); + } arrayValue->Insert(key, Read(inStream)); } - constexpr int32_t maxArraySize = 10'000; - if (sizeOfDenseArray > maxArraySize) { - LOG("Someone sent 10,000 dense array entries, probably a bad packet."); - throw std::invalid_argument("Someone sent 10,000 dense array entries, probably a bad packet."); - } // Finally read dense portion for (uint32_t i = 0; i < sizeOfDenseArray; i++) { arrayValue->Insert(i, Read(inStream)); } + + --m_Depth; return arrayValue; } diff --git a/dCommon/AMFDeserialize.h b/dCommon/AMFDeserialize.h index dc8a21a8e..5bd1d1717 100644 --- a/dCommon/AMFDeserialize.h +++ b/dCommon/AMFDeserialize.h @@ -9,6 +9,15 @@ class AMFDeserialize { public: + // Most entries one array may hold, in each of its dense and associative parts. + static constexpr uint32_t MaxArraySize = 10'000; + + // Deepest nesting of arrays allowed. Client UI messages nest a handful of levels. + static constexpr uint32_t MaxDepth = 32; + + // Most values one deserializer will read over its lifetime. + static constexpr uint32_t MaxValues = 100'000; + /** * Read an AMF3 value from a bitstream. * @@ -69,4 +78,10 @@ private: * List of strings read so far saved to be read by reference. */ std::vector accessedElements; + + // How deeply nested the array being read is. + uint32_t m_Depth = 0; + + // How many values have been read so far. + uint32_t m_ValuesRead = 0; }; diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index dce06973e..f8f0eddf8 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -5,7 +5,7 @@ #include "Logger.h" #include "Game.h" #include -#include +#include #include enum class eAmf : uint8_t { @@ -115,8 +115,9 @@ using AMFDoubleValue = AMFValue; * and are not to be deleted by a caller. */ class AMFArrayValue : public AMFBaseValue { - using AMFAssociative = - std::unordered_map, GeneralUtils::transparent_string_hash, std::equal_to<>>; + // An ordered map keeps lookups and inserts O(log n) no matter which keys a client sends. + // A hash map here let a client pick colliding keys and make decoding quadratic (HashDoS). + using AMFAssociative = std::map, std::less<>>; using AMFDense = std::vector>; @@ -222,9 +223,9 @@ public: */ template AmfType& Insert(const std::string_view key, std::unique_ptr value) { - const auto element = m_Associative.find(key); auto& toReturn = *value; - if (element != m_Associative.cend() && element->second) { + const auto element = m_Associative.find(key); + if (element != m_Associative.cend()) { element->second = std::move(value); } else { m_Associative.emplace(key, std::move(value)); diff --git a/tests/dCommonTests/AMFDeserializeTests.cpp b/tests/dCommonTests/AMFDeserializeTests.cpp index 759e6fc44..b04e1a01a 100644 --- a/tests/dCommonTests/AMFDeserializeTests.cpp +++ b/tests/dCommonTests/AMFDeserializeTests.cpp @@ -7,6 +7,7 @@ #include "Game.h" #include "Logger.h" +#include "dCommonDependencies.h" /** * Helper method that all tests use to get their respective AMF. @@ -436,3 +437,149 @@ args: amf3! ], } */ + +namespace { + // Writes an inline AMF string of at most 63 characters (single byte U29 header). + void WriteShortAmfString(RakNet::BitStream& bitStream, const std::string& str) { + bitStream.Write(static_cast((str.size() << 1) | 1)); + for (const auto e : str) bitStream.Write(e); + } + + // Writes a U29 integer the way AMF3 encodes it. + void WriteU29(RakNet::BitStream& bitStream, uint32_t value) { + if (value < 0x80) { + bitStream.Write(value); + } else if (value < 0x4000) { + bitStream.Write(((value >> 7) & 0x7F) | 0x80); + bitStream.Write(value & 0x7F); + } else if (value < 0x200000) { + bitStream.Write(((value >> 14) & 0x7F) | 0x80); + bitStream.Write(((value >> 7) & 0x7F) | 0x80); + bitStream.Write(value & 0x7F); + } else { + bitStream.Write(((value >> 22) & 0x7F) | 0x80); + bitStream.Write(((value >> 15) & 0x7F) | 0x80); + bitStream.Write(((value >> 8) & 0x7F) | 0x80); + bitStream.Write(value & 0xFF); + } + } +} + +// The limit checks log before throwing, so these tests need a logger. +class AMFDeserializeLimitsTest : public dCommonDependenciesTest { +protected: + void SetUp() override { SetUpDependencies(); } + void TearDown() override { + TearDownDependencies(); + Game::logger = nullptr; + } +}; + +/** + * @brief Arrays nested past the depth limit must be rejected instead of recursing until the stack runs out. + */ +TEST_F(AMFDeserializeLimitsTest, NestingLimitTest) { + const auto writeNested = [](RakNet::BitStream& bitStream, uint32_t depth) { + // Each level is an array with no dense part whose only associative value is the next level. + for (uint32_t i = 0; i < depth; i++) { + bitStream.Write(0x09); + bitStream.Write(0x01); + if (i + 1 < depth) WriteShortAmfString(bitStream, "a"); + } + for (uint32_t i = 0; i < depth; i++) bitStream.Write(0x01); + }; + + { + CBITSTREAM; + writeNested(bitStream, AMFDeserialize::MaxDepth); + std::unique_ptr res; + ASSERT_NO_THROW(res = ReadFromBitStream(bitStream)); + ASSERT_EQ(res->GetValueType(), eAmf::Array); + } + { + CBITSTREAM; + writeNested(bitStream, AMFDeserialize::MaxDepth + 1); + ASSERT_THROW(ReadFromBitStream(bitStream), std::invalid_argument); + } + { + // Far past the limit, what a malicious client would send to overflow the stack. + CBITSTREAM; + writeNested(bitStream, 100'000); + ASSERT_THROW(ReadFromBitStream(bitStream), std::invalid_argument); + } +} + +/** + * @brief The associative part of an array is bounded the same way the dense part is. + */ +TEST_F(AMFDeserializeLimitsTest, AssociativeLimitTest) { + const auto writeArray = [](RakNet::BitStream& bitStream, uint32_t entries) { + bitStream.Write(0x09); + bitStream.Write(0x01); + for (uint32_t i = 0; i < entries; i++) { + WriteShortAmfString(bitStream, std::to_string(i)); + bitStream.Write(0x03); // true + } + bitStream.Write(0x01); + }; + + { + CBITSTREAM; + writeArray(bitStream, AMFDeserialize::MaxArraySize); + std::unique_ptr res; + ASSERT_NO_THROW(res = ReadFromBitStream(bitStream)); + ASSERT_EQ(static_cast(res.get())->GetAssociative().size(), AMFDeserialize::MaxArraySize); + } + { + CBITSTREAM; + writeArray(bitStream, AMFDeserialize::MaxArraySize + 1); + ASSERT_THROW(ReadFromBitStream(bitStream), std::invalid_argument); + } +} + +/** + * @brief The dense size is checked before any of the associative part is read. + */ +TEST_F(AMFDeserializeLimitsTest, DenseLimitTest) { + CBITSTREAM; + bitStream.Write(0x09); + WriteU29(bitStream, ((AMFDeserialize::MaxArraySize + 1) << 1) | 1); + bitStream.Write(0x01); + ASSERT_THROW(ReadFromBitStream(bitStream), std::invalid_argument); +} + +/** + * @brief Many small arrays that are each within limits still count toward one total budget. + */ +TEST_F(AMFDeserializeLimitsTest, TotalValueLimitTest) { + CBITSTREAM; + // An outer array of 20 arrays with 10,000 values each is 200,000 values. + bitStream.Write(0x09); + WriteU29(bitStream, (20 << 1) | 1); + bitStream.Write(0x01); + for (int i = 0; i < 20; i++) { + bitStream.Write(0x09); + WriteU29(bitStream, (AMFDeserialize::MaxArraySize << 1) | 1); + bitStream.Write(0x01); + for (uint32_t j = 0; j < AMFDeserialize::MaxArraySize; j++) bitStream.Write(0x03); + } + ASSERT_THROW(ReadFromBitStream(bitStream), std::invalid_argument); +} + +/** + * @brief Sending a key twice keeps the last value, and the returned reference is to a live value. + */ +TEST_F(AMFDeserializeLimitsTest, DuplicateKeyTest) { + CBITSTREAM; + bitStream.Write(0x09); + bitStream.Write(0x01); + WriteShortAmfString(bitStream, "key"); + bitStream.Write(0x02); // false + WriteShortAmfString(bitStream, "key"); + bitStream.Write(0x03); // true + bitStream.Write(0x01); + std::unique_ptr res{ ReadFromBitStream(bitStream) }; + auto* const array = static_cast(res.get()); + ASSERT_EQ(array->GetAssociative().size(), 1); + ASSERT_TRUE(array->Get("key")->GetValue()); +}