fix: limit how deep AMF3 arrays nest and stop at the end of the data

The AMF3 reader read an array inside an array to any depth and kept
reading values after the data had ended. It now refuses arrays nested
more than 64 deep and throws when the data ends before a value does.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

dCommon/AMFDeserialize.cpp

@@ -20,7 +20,9 @@ std::unique_ptr<AMFBaseValue> AMFDeserialize::Read(RakNet::BitStream& inStream)
 
 	// Read in the value type from the bitStream
 	eAmf marker;
-	inStream.Read(marker);
+	if (!inStream.Read(marker)) {
+		throw std::invalid_argument("The AMF3 data ended before its value did");
+	}
 	// Based on the typing, create the value associated with that and return the base value class
 	switch (marker) {
 	case eAmf::Undefined:
@@ -119,12 +121,18 @@ 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.
+	// An array inside an array inside an array and so on only goes so deep
 	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");
+		LOG("AMF arrays nested deeper than %u, aborting deserialize.", MaxDepth);
+		throw std::invalid_argument("AMF3 arrays nested too deep");
 	}
-	++m_Depth;
+
+	// Counts this array for as long as it is being read, also when a value in it throws
+	struct DepthGuard {
+		uint32_t& depth;
+		explicit DepthGuard(uint32_t& depth) : depth(depth) { ++depth; }
+		~DepthGuard() { --depth; }
+	} depthGuard(m_Depth);
 
 	auto arrayValue = std::make_unique<AMFArrayValue>();
 
@@ -153,7 +161,6 @@ std::unique_ptr<AMFArrayValue> AMFDeserialize::ReadAmfArray(RakNet::BitStream& i
 		arrayValue->Insert(i, Read(inStream));
 	}
 
-	--m_Depth;
 	return arrayValue;
 }
 

dCommon/AMFDeserialize.h

@@ -79,7 +79,9 @@ private:
 	 */
 	std::vector<std::string> accessedElements;
 
-	// How deeply nested the array being read is.
+	/**
+	 * How many arrays are being read at the moment, each one inside the one before it.
+	 */
 	uint32_t m_Depth = 0;
 
 	// How many values have been read so far.

tests/dCommonTests/AMFDeserializeTests.cpp

@@ -583,3 +583,59 @@ TEST_F(AMFDeserializeLimitsTest, DuplicateKeyTest) {
 	ASSERT_EQ(array->GetAssociative().size(), 1);
 	ASSERT_TRUE(array->Get<bool>("key")->GetValue());
 }
+
+/**
+ * @brief Writes arrays inside one another, each as the only dense value of the one before it
+ */
+static void WriteNestedArrays(RakNet::BitStream& bitStream, const uint32_t depth) {
+	for (uint32_t i = 0; i < depth - 1; i++) {
+		bitStream.Write<uint8_t>(0x09); // Array
+		bitStream.Write<uint8_t>(0x03); // One dense value
+		bitStream.Write<uint8_t>(0x01); // No associative values
+	}
+	bitStream.Write<uint8_t>(0x09); // Array
+	bitStream.Write<uint8_t>(0x01); // No dense values
+	bitStream.Write<uint8_t>(0x01); // No associative values
+}
+
+TEST_F(AMFDeserializeLimitsTest, NestedArraysTest) {
+	RakNet::BitStream bitStream;
+	WriteNestedArrays(bitStream, AMFDeserialize::MaxDepth);
+	AMFDeserialize deserializer;
+	std::unique_ptr<AMFBaseValue> res{ deserializer.Read(bitStream) };
+	ASSERT_EQ(res->GetValueType(), eAmf::Array);
+
+	// The same reader reads another value afterwards
+	RakNet::BitStream second;
+	WriteNestedArrays(second, AMFDeserialize::MaxDepth);
+	ASSERT_EQ(deserializer.Read(second)->GetValueType(), eAmf::Array);
+}
+
+TEST_F(AMFDeserializeLimitsTest, ArraysNestedTooDeepTest) {
+	RakNet::BitStream bitStream;
+	WriteNestedArrays(bitStream, AMFDeserialize::MaxDepth + 1);
+	AMFDeserialize deserializer;
+	EXPECT_THROW(deserializer.Read(bitStream), std::invalid_argument);
+
+	// The count of open arrays is back at none after the failed read
+	RakNet::BitStream second;
+	WriteNestedArrays(second, AMFDeserialize::MaxDepth);
+	ASSERT_EQ(deserializer.Read(second)->GetValueType(), eAmf::Array);
+
+	RakNet::BitStream deep;
+	WriteNestedArrays(deep, 100000);
+	EXPECT_THROW(ReadFromBitStream(deep), std::invalid_argument);
+}
+
+TEST_F(AMFDeserializeLimitsTest, DataEndsEarlyTest) {
+	// A string key that is never followed by its value
+	RakNet::BitStream bitStream;
+	bitStream.Write<uint8_t>(0x09); // Array
+	bitStream.Write<uint8_t>(0x01); // No dense values
+	bitStream.Write<uint8_t>(0x03); // A key of one character
+	bitStream.Write<uint8_t>('a');
+	EXPECT_THROW(ReadFromBitStream(bitStream), std::invalid_argument);
+
+	RakNet::BitStream empty;
+	EXPECT_THROW(ReadFromBitStream(empty), std::invalid_argument);
+}