From a235e5797565feb8ee10333390e3e4d88be7b370 Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Tue, 10 Nov 2026 13:02:05 -0600 Subject: [PATCH] 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 --- dCommon/AMFDeserialize.cpp | 19 +++++--- dCommon/AMFDeserialize.h | 4 +- tests/dCommonTests/AMFDeserializeTests.cpp | 56 ++++++++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/dCommon/AMFDeserialize.cpp b/dCommon/AMFDeserialize.cpp index 003d1a338..2b79483e0 100644 --- a/dCommon/AMFDeserialize.cpp +++ b/dCommon/AMFDeserialize.cpp @@ -20,7 +20,9 @@ std::unique_ptr 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 AMFDeserialize::ReadAmfDouble(RakNet::BitStream& } std::unique_ptr 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(); @@ -153,7 +161,6 @@ std::unique_ptr AMFDeserialize::ReadAmfArray(RakNet::BitStream& i arrayValue->Insert(i, Read(inStream)); } - --m_Depth; return arrayValue; } diff --git a/dCommon/AMFDeserialize.h b/dCommon/AMFDeserialize.h index 5bd1d1717..7e9892845 100644 --- a/dCommon/AMFDeserialize.h +++ b/dCommon/AMFDeserialize.h @@ -79,7 +79,9 @@ private: */ std::vector 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. diff --git a/tests/dCommonTests/AMFDeserializeTests.cpp b/tests/dCommonTests/AMFDeserializeTests.cpp index 62e329b5c..b2d4d742a 100644 --- a/tests/dCommonTests/AMFDeserializeTests.cpp +++ b/tests/dCommonTests/AMFDeserializeTests.cpp @@ -583,3 +583,59 @@ TEST_F(AMFDeserializeLimitsTest, DuplicateKeyTest) { ASSERT_EQ(array->GetAssociative().size(), 1); ASSERT_TRUE(array->Get("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(0x09); // Array + bitStream.Write(0x03); // One dense value + bitStream.Write(0x01); // No associative values + } + bitStream.Write(0x09); // Array + bitStream.Write(0x01); // No dense values + bitStream.Write(0x01); // No associative values +} + +TEST_F(AMFDeserializeLimitsTest, NestedArraysTest) { + RakNet::BitStream bitStream; + WriteNestedArrays(bitStream, AMFDeserialize::MaxDepth); + AMFDeserialize deserializer; + std::unique_ptr 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(0x09); // Array + bitStream.Write(0x01); // No dense values + bitStream.Write(0x03); // A key of one character + bitStream.Write('a'); + EXPECT_THROW(ReadFromBitStream(bitStream), std::invalid_argument); + + RakNet::BitStream empty; + EXPECT_THROW(ReadFromBitStream(empty), std::invalid_argument); +}