Fix CVE-2024-30258 / CVE-2024-30259

Signed-off-by: Mario Dominguez <[email protected]>

include/fastdds/rtps/messages/CDRMessage.h

@@ -140,15 +140,18 @@ inline bool readBinaryProperty(
 
 inline bool readPropertySeq(
         CDRMessage_t* msg,
-        PropertySeq& properties);
+        PropertySeq& properties,
+        const uint32_t parameter_length);
 
 inline bool readBinaryPropertySeq(
         CDRMessage_t* msg,
-        BinaryPropertySeq& binary_properties);
+        BinaryPropertySeq& binary_properties,
+        const uint32_t parameter_length);
 
 inline bool readDataHolder(
         CDRMessage_t* msg,
-        DataHolder& data_holder);
+        DataHolder& data_holder,
+        const uint32_t parameter_length);
 
 inline bool readDataHolderSeq(
         CDRMessage_t* msg,

include/fastdds/rtps/messages/CDRMessage.hpp

@@ -860,7 +860,8 @@ inline bool CDRMessage::addPropertySeq(
 
 inline bool CDRMessage::readPropertySeq(
         CDRMessage_t* msg,
-        PropertySeq& properties)
+        PropertySeq& properties,
+        const uint32_t parameter_length)
 {
     assert(msg);
 
@@ -870,6 +871,13 @@ inline bool CDRMessage::readPropertySeq(
         return false;
     }
 
+    // Length should be at least 16 times the number of elements, since each property contains
+    // 2 empty strings, each with 4 bytes for its length + at least 4 bytes of data (single NUL character + padding)
+    if (16 * length > parameter_length)
+    {
+        return false;
+    }
+
     properties.resize(length);
     bool returnedValue = true;
     for (uint32_t i = 0; returnedValue && i < length; ++i)
@@ -962,7 +970,8 @@ inline bool CDRMessage::addBinaryPropertySeq(
 
 inline bool CDRMessage::readBinaryPropertySeq(
         CDRMessage_t* msg,
-        BinaryPropertySeq& binary_properties)
+        BinaryPropertySeq& binary_properties,
+        const uint32_t parameter_length)
 {
     assert(msg);
 
@@ -972,6 +981,14 @@ inline bool CDRMessage::readBinaryPropertySeq(
         return false;
     }
 
+    // Length should be at least 12 times the number of elements, since each each property contains at least
+    // 1 empty string with 4 bytes for its length + at least 4 bytes of data (single NUL character + padding) and
+    // 1 empty byte sequence with 4 bytes for its length
+    if (12 * length > parameter_length)
+    {
+        return false;
+    }
+
     binary_properties.resize(length);
     bool returnedValue = true;
     for (uint32_t i = 0; returnedValue && i < length; ++i)
@@ -1006,19 +1023,20 @@ inline bool CDRMessage::addDataHolder(
 
 inline bool CDRMessage::readDataHolder(
         CDRMessage_t* msg,
-        DataHolder& data_holder)
+        DataHolder& data_holder,
+        const uint32_t parameter_length)
 {
     assert(msg);
 
     if (!CDRMessage::readString(msg, &data_holder.class_id()))
     {
         return false;
     }
-    if (!CDRMessage::readPropertySeq(msg, data_holder.properties()))
+    if (!CDRMessage::readPropertySeq(msg, data_holder.properties(), parameter_length))
     {
         return false;
     }
-    if (!CDRMessage::readBinaryPropertySeq(msg, data_holder.binary_properties()))
+    if (!CDRMessage::readBinaryPropertySeq(msg, data_holder.binary_properties(), parameter_length))
     {
         return false;
     }
@@ -1063,11 +1081,21 @@ inline bool CDRMessage::readDataHolderSeq(
         return false;
     }
 
+    // Length should be at least 16 times the number of elements, since each DataHolder contains at least
+    // 1 empty string with 4 bytes for its length + at least 4 bytes of data (single NUL character + padding) and
+    // 2 empty property sequences, each with 4 bytes for its length
+    if (msg->pos + 16 * length > msg->length)
+    {
+        return false;
+    }
+
     data_holders.resize(length);
     bool returnedValue = true;
     for (uint32_t i = 0; returnedValue && i < length; ++i)
     {
-        returnedValue = CDRMessage::readDataHolder(msg, data_holders.at(i));
+        //! The parameter length should be the remaining length of the message
+        uint32_t remaining_length = msg->length - msg->pos;
+        returnedValue = CDRMessage::readDataHolder(msg, data_holders.at(i), remaining_length);
     }
 
     return returnedValue;

src/cpp/fastdds/core/policy/ParameterSerializer.hpp

@@ -1039,7 +1039,7 @@ inline bool ParameterSerializer<ParameterToken_t>::read_content_from_cdr_message
 
     parameter.length = parameter_length;
     uint32_t pos_ref = cdr_message->pos;
-    bool valid =  fastrtps::rtps::CDRMessage::readDataHolder(cdr_message, parameter.token);
+    bool valid =  fastrtps::rtps::CDRMessage::readDataHolder(cdr_message, parameter.token, parameter_length);
     uint32_t length_diff = cdr_message->pos - pos_ref;
     valid &= (parameter_length == length_diff);
     return valid;

test/blackbox/common/BlackboxTestsTransportUDP.cpp

@@ -554,6 +554,8 @@ TEST(TransportUDP, DatagramInjection)
 
     deliver_datagram_from_file(receivers, "datagrams/16784.bin");
     deliver_datagram_from_file(receivers, "datagrams/20140.bin");
+    deliver_datagram_from_file(receivers, "datagrams/20574.bin");
+    deliver_datagram_from_file(receivers, "datagrams/20660.bin");
 }
 
 TEST(TransportUDP, MaliciousManipulatedDataOctetsToNextHeaderIgnore)