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 <noreply@anthropic.com>

dCommon/AMFDeserialize.cpp

@@ -11,6 +11,13 @@
  */
 
 std::unique_ptr<AMFBaseValue> 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<AMFDoubleValue> AMFDeserialize::ReadAmfDouble(RakNet::BitStream&
 }
 
 std::unique_ptr<AMFArrayValue> 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<AMFArrayValue>();
 
 	// 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;
 }
 

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<std::string> 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;
 };

dCommon/Amf3.h

@@ -5,7 +5,7 @@
 #include "Logger.h"
 #include "Game.h"
 #include <type_traits>
-#include <unordered_map>
+#include <map>
 #include <vector>
 
 enum class eAmf : uint8_t {
@@ -115,8 +115,9 @@ using AMFDoubleValue = AMFValue<double>;
  * and are not to be deleted by a caller.
  */
 class AMFArrayValue : public AMFBaseValue {
-	using AMFAssociative =
-		std::unordered_map<std::string, std::unique_ptr<AMFBaseValue>, 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::string, std::unique_ptr<AMFBaseValue>, std::less<>>;
 
 	using AMFDense = std::vector<std::unique_ptr<AMFBaseValue>>;
 
@@ -222,9 +223,9 @@ public:
 	 */
 	template<typename AmfType>
 	AmfType& Insert(const std::string_view key, std::unique_ptr<AmfType> 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));

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<uint8_t>(static_cast<uint8_t>((str.size() << 1) | 1));
+		for (const auto e : str) bitStream.Write<char>(e);
+	}
+
+	// Writes a U29 integer the way AMF3 encodes it.
+	void WriteU29(RakNet::BitStream& bitStream, uint32_t value) {
+		if (value < 0x80) {
+			bitStream.Write<uint8_t>(value);
+		} else if (value < 0x4000) {
+			bitStream.Write<uint8_t>(((value >> 7) & 0x7F) | 0x80);
+			bitStream.Write<uint8_t>(value & 0x7F);
+		} else if (value < 0x200000) {
+			bitStream.Write<uint8_t>(((value >> 14) & 0x7F) | 0x80);
+			bitStream.Write<uint8_t>(((value >> 7) & 0x7F) | 0x80);
+			bitStream.Write<uint8_t>(value & 0x7F);
+		} else {
+			bitStream.Write<uint8_t>(((value >> 22) & 0x7F) | 0x80);
+			bitStream.Write<uint8_t>(((value >> 15) & 0x7F) | 0x80);
+			bitStream.Write<uint8_t>(((value >> 8) & 0x7F) | 0x80);
+			bitStream.Write<uint8_t>(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<uint8_t>(0x09);
+			bitStream.Write<uint8_t>(0x01);
+			if (i + 1 < depth) WriteShortAmfString(bitStream, "a");
+		}
+		for (uint32_t i = 0; i < depth; i++) bitStream.Write<uint8_t>(0x01);
+	};
+
+	{
+		CBITSTREAM;
+		writeNested(bitStream, AMFDeserialize::MaxDepth);
+		std::unique_ptr<AMFBaseValue> 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<uint8_t>(0x09);
+		bitStream.Write<uint8_t>(0x01);
+		for (uint32_t i = 0; i < entries; i++) {
+			WriteShortAmfString(bitStream, std::to_string(i));
+			bitStream.Write<uint8_t>(0x03); // true
+		}
+		bitStream.Write<uint8_t>(0x01);
+	};
+
+	{
+		CBITSTREAM;
+		writeArray(bitStream, AMFDeserialize::MaxArraySize);
+		std::unique_ptr<AMFBaseValue> res;
+		ASSERT_NO_THROW(res = ReadFromBitStream(bitStream));
+		ASSERT_EQ(static_cast<AMFArrayValue*>(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<uint8_t>(0x09);
+	WriteU29(bitStream, ((AMFDeserialize::MaxArraySize + 1) << 1) | 1);
+	bitStream.Write<uint8_t>(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<uint8_t>(0x09);
+	WriteU29(bitStream, (20 << 1) | 1);
+	bitStream.Write<uint8_t>(0x01);
+	for (int i = 0; i < 20; i++) {
+		bitStream.Write<uint8_t>(0x09);
+		WriteU29(bitStream, (AMFDeserialize::MaxArraySize << 1) | 1);
+		bitStream.Write<uint8_t>(0x01);
+		for (uint32_t j = 0; j < AMFDeserialize::MaxArraySize; j++) bitStream.Write<uint8_t>(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<uint8_t>(0x09);
+	bitStream.Write<uint8_t>(0x01);
+	WriteShortAmfString(bitStream, "key");
+	bitStream.Write<uint8_t>(0x02); // false
+	WriteShortAmfString(bitStream, "key");
+	bitStream.Write<uint8_t>(0x03); // true
+	bitStream.Write<uint8_t>(0x01);
+	std::unique_ptr<AMFBaseValue> res{ ReadFromBitStream(bitStream) };
+	auto* const array = static_cast<AMFArrayValue*>(res.get());
+	ASSERT_EQ(array->GetAssociative().size(), 1);
+	ASSERT_TRUE(array->Get<bool>("key")->GetValue());
+}