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); +}