From 65236f93e9c4ea3ff9a49fba4dfd9e43eb94037b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Dom=C3=ADnguez=20L=C3=B3pez?= <116071334+Mario-DL@users.noreply.github.com> Date: Tue, 2 Apr 2024 11:00:21 +0200 Subject: [PATCH] Merge pull request from GHSA-53xw-465j-rxfh Signed-off-by: Mario Dominguez --- include/fastdds/rtps/messages/CDRMessage.h | 9 ++-- include/fastdds/rtps/messages/CDRMessage.hpp | 40 +++++++++++++++--- .../core/policy/ParameterSerializer.hpp | 2 +- .../common/BlackboxTestsTransportUDP.cpp | 2 + test/blackbox/datagrams/20574.bin | Bin 0 -> 480 bytes test/blackbox/datagrams/20660.bin | Bin 0 -> 480 bytes 6 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 test/blackbox/datagrams/20574.bin create mode 100644 test/blackbox/datagrams/20660.bin 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 d53a25edc5e..a63c1af43af 100644 --- a/src/cpp/fastdds/core/policy/ParameterSerializer.hpp +++ b/src/cpp/fastdds/core/policy/ParameterSerializer.hpp @@ -1039,7 +1039,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 e907d1afab1..fd9c8e66355 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -533,6 +533,8 @@ TEST(TransportUDP, DatagramInjection) DatagramInjectionTransport::deliver_datagram_from_file(receivers, "datagrams/16784.bin"); DatagramInjectionTransport::deliver_datagram_from_file(receivers, "datagrams/20140.bin"); + DatagramInjectionTransport::deliver_datagram_from_file(receivers, "datagrams/20574.bin"); + DatagramInjectionTransport::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 0000000000000000000000000000000000000000..1aba115252a8d27f9ae56a74c1468c6ccd03bf70 GIT binary patch literal 480 zcmWFv2?%ClX5?q&x2*W%^6~=%5O6YbFfgoJlX~-fM1Ux310w^200RRf!*L)!1d;~| zFfcGPFo-g+FfajWF(A#z&%h7>REKO1P~|}*1_`kGZ!#cF2+(wTlA0k@?mHGaCIJox z5vcMAp!xPZ{0vDz5ui&L0uqZ#GLth467x#p3rdq1A{lM~RWL9Jg5(^7LOh*4104NA z;zJ?>TtP;#197mYZ-5VwCkSg2?moPBs1LZSK zbklQmOLDVKEe%X`jr9^!^a_eg^-@zxt&9weOw0@oOpJ^z3@r^!3=NG8@R?a!oLa=d z01Qorl-wdSQxhWwIh^(v6y+zU78e8MLGCp+F*P;;nhgr;21W}o{RJ(Y>Os=Lcv|&N PE{hYW4g{uvEJFeSS)X1( literal 0 HcmV?d00001 diff --git a/test/blackbox/datagrams/20660.bin b/test/blackbox/datagrams/20660.bin new file mode 100644 index 0000000000000000000000000000000000000000..ff9122757f724ab0400ddac3664e87d7f6b13fe6 GIT binary patch literal 480 zcmWFv2?%ClX5?q&x2*W%^6~=%5O6YbFfgoJlX~-fM1Ux310w^200RRf!*L)!1d;~| zFfcGPFo-g+FfajWF(A#z&%h7>REKO1P~|}*1_`kGZ!#cF2+(wTlA0k@?mHGaCZLrf zP~{Oo^X+-~8Iph^Kw*Y}#G;bSxi+-y@z0~1|iy~Gr~g5pxW)Ra;yBLgE7GeZLtBV!9gOG6VwLn8xxW|kJG7BMgY zLz5vTx5&)Y#E3x-r~L&*`N^rp#XxzGdyP#@jZJ`NgTlIj(E?0=K?|pPkTfu!Y@Nyi P!Epjq`vArOvOxd