diff --git a/include/fastdds/rtps/messages/CDRMessage.h b/include/fastdds/rtps/messages/CDRMessage.h index 738c65b96b7..cb6e73b3db3 100644 --- a/include/fastdds/rtps/messages/CDRMessage.h +++ b/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, diff --git a/include/fastdds/rtps/messages/CDRMessage.hpp b/include/fastdds/rtps/messages/CDRMessage.hpp index d74f28bf8d0..4618dc361cb 100644 --- a/include/fastdds/rtps/messages/CDRMessage.hpp +++ b/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,7 +1023,8 @@ inline bool CDRMessage::addDataHolder( inline bool CDRMessage::readDataHolder( CDRMessage_t* msg, - DataHolder& data_holder) + DataHolder& data_holder, + const uint32_t parameter_length) { assert(msg); @@ -1014,11 +1032,11 @@ inline bool CDRMessage::readDataHolder( { 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; diff --git a/src/cpp/fastdds/core/policy/ParameterSerializer.hpp b/src/cpp/fastdds/core/policy/ParameterSerializer.hpp index 8c2ddacde70..e169c5364e2 100644 --- a/src/cpp/fastdds/core/policy/ParameterSerializer.hpp +++ b/src/cpp/fastdds/core/policy/ParameterSerializer.hpp @@ -983,7 +983,7 @@ inline bool ParameterSerializer::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; diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index 0efcf75dfdd..a8b0334a40f 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -550,6 +550,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) diff --git a/test/blackbox/datagrams/20574.bin b/test/blackbox/datagrams/20574.bin new file mode 100644 index 00000000000..1aba115252a Binary files /dev/null and b/test/blackbox/datagrams/20574.bin differ diff --git a/test/blackbox/datagrams/20660.bin b/test/blackbox/datagrams/20660.bin new file mode 100644 index 00000000000..ff9122757f7 Binary files /dev/null and b/test/blackbox/datagrams/20660.bin differ