Merge pull request from GHSA-9m2j-qw67-ph4w

* Refs #20549: Add BB test Signed-off-by: Mario Dominguez <[email protected]> * Fix:Fixed integer overflow causing heap overflow When a sub node receives a manipulated DATA sub-message, an Integer Overflow occurs in uint32_t payload_size. This causes a heap buffer overflow error. A comparison statement was inserted before the line that calculates the variable, which fixes the error. Signed-off-by: Desglaneurs <[email protected]> * Refs #20549: Fix review Signed-off-by: Mario Dominguez <[email protected]> * Refs #201549: Reset the change data fields before exiting Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: Desglaneurs <[email protected]> Co-authored-by: Mario Dominguez <[email protected]>
eProsima · Mar 20, 2024 · d4f57b0 · d4f57b0
1 parent ecca94c
commit d4f57b0
Show file tree

Hide file tree

Showing 13 changed files with 935 additions and 33 deletions.
diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp
@@ -805,8 +805,20 @@ bool MessageReceiver::proc_Submsg_Data(
     if (dataFlag || keyFlag)
     {
         uint32_t payload_size;
-        payload_size = smh->submessageLength -
-                (RTPSMESSAGE_DATA_EXTRA_INLINEQOS_SIZE + octetsToInlineQos + inlineQosSize);
+        const uint32_t submsg_no_payload_size =
+                RTPSMESSAGE_DATA_EXTRA_INLINEQOS_SIZE + octetsToInlineQos + inlineQosSize;
+
+        // Prevent integer overflow of variable payload_size
+        if (smh->submessageLength < submsg_no_payload_size)
+        {
+            logWarning(RTPS_MSG_IN, IDSTRING "Serialized Payload avoided overflow "
+                    "(" << smh->submessageLength << "/" << submsg_no_payload_size << ")");
+            ch.serializedPayload.data = nullptr;
+            ch.inline_qos.data = nullptr;
+            return false;
+        }
+
+        payload_size = smh->submessageLength - submsg_no_payload_size;
 
         if (dataFlag)
         {

diff --git a/test/blackbox/CMakeLists.txt b/test/blackbox/CMakeLists.txt
@@ -263,6 +263,8 @@ set(RTPS_BLACKBOXTESTS_SOURCE ${RTPS_BLACKBOXTESTS_TEST_SOURCE}
     types/KeyedData1mbPubSubTypes.cxx
     types/FixedSized.cxx
     types/FixedSizedPubSubTypes.cxx
+    types/UnboundedHelloWorld.cxx
+    types/UnboundedHelloWorldPubSubTypes.cxx
 
     utils/data_generators.cpp
     utils/lambda_functions.cpp
@@ -303,6 +305,8 @@ set(BLACKBOXTESTS_SOURCE ${BLACKBOXTESTS_TEST_SOURCE}
     types/TestRegression3361.cxx
     types/TestRegression3361PubSubTypes.cxx
     types/TestRegression3361TypeObject.cxx
+    types/UnboundedHelloWorld.cxx
+    types/UnboundedHelloWorldPubSubTypes.cxx
 
     utils/data_generators.cpp
     utils/lambda_functions.cpp

diff --git a/test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp b/test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp
@@ -331,7 +331,8 @@ class PubSubReader
 
             if (subscriber_ != nullptr)
             {
-                std::cout << "Created subscriber " << subscriber_->getGuid() << " for topic " <<
+                subscriber_guid_ = subscriber_->getGuid();
+                std::cout << "Created subscriber " << subscriber_guid_ << " for topic " <<
                     subscriber_attr_.topic.topicName << std::endl;
 
                 initialized_ = true;
@@ -1342,6 +1343,11 @@ class PubSubReader
         return participant_guid_;
     }
 
+    const eprosima::fastrtps::rtps::GUID_t& datareader_guid() const
+    {
+        return subscriber_guid_;
+    }
+
 private:
 
     void receive_one(
@@ -1433,6 +1439,7 @@ class PubSubReader
     eprosima::fastrtps::SubscriberAttributes subscriber_attr_;
     std::string topic_name_;
     eprosima::fastrtps::rtps::GUID_t participant_guid_;
+    eprosima::fastrtps::rtps::GUID_t subscriber_guid_;
     bool initialized_;
     std::list<type> total_msgs_;
     std::mutex mutex_;

diff --git a/test/blackbox/common/BlackboxTests.hpp b/test/blackbox/common/BlackboxTests.hpp
@@ -28,13 +28,14 @@
 #include <unistd.h>
 #endif // if defined(_WIN32)
 
-#include "../types/HelloWorldPubSubTypes.h"
+#include "../types/Data1mbPubSubTypes.h"
+#include "../types/Data64kbPubSubTypes.h"
 #include "../types/FixedSizedPubSubTypes.h"
+#include "../types/HelloWorldPubSubTypes.h"
+#include "../types/KeyedData1mbPubSubTypes.h"
 #include "../types/KeyedHelloWorldPubSubTypes.h"
 #include "../types/StringTestPubSubTypes.h"
-#include "../types/Data64kbPubSubTypes.h"
-#include "../types/Data1mbPubSubTypes.h"
-#include "../types/KeyedData1mbPubSubTypes.h"
+#include "../types/UnboundedHelloWorldPubSubTypes.h"
 
 #include <algorithm>
 #include <cstddef>
@@ -163,6 +164,9 @@ std::list<Data1mb> default_data96kb_data300kb_data_generator(
 std::list<KeyedData1mb> default_keyeddata300kb_data_generator(
         size_t max = 0);
 
+std::list<UnboundedHelloWorld> default_unbounded_helloworld_data_generator(
+        size_t max = 0);
+
 /****** Auxiliary lambda functions  ******/
 extern const std::function<void(const HelloWorld&)>  default_helloworld_print;
 

diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp
@@ -27,6 +27,7 @@
 #include "PubSubWriter.hpp"
 #include "PubSubWriterReader.hpp"
 #include "PubSubParticipant.hpp"
+#include "UDPMessageSender.hpp"
 
 #include <fastdds/dds/log/Log.hpp>
 #include <fastdds/rtps/common/EntityId_t.hpp>
@@ -89,32 +90,6 @@ class Security : public testing::TestWithParam<communication_type>
 
 };
 
-struct UDPMessageSender
-{
-    asio::io_service service;
-    asio::ip::udp::socket socket;
-
-    UDPMessageSender()
-        : service()
-        , socket(service)
-    {
-        socket.open(asio::ip::udp::v4());
-    }
-
-    void send(
-            const CDRMessage_t& msg,
-            const Locator_t& destination)
-    {
-        std::string addr = IPLocator::toIPv4string(destination);
-        unsigned short port = static_cast<unsigned short>(destination.port);
-        auto remote = asio::ip::udp::endpoint(asio::ip::address::from_string(addr), port);
-        asio::error_code ec;
-
-        socket.send_to(asio::buffer(msg.buffer, msg.length), remote, 0, ec);
-    }
-
-};
-
 class SecurityPkcs : public ::testing::Test
 {
 public:

diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp
@@ -29,6 +29,7 @@
 #include "DatagramInjectionTransport.hpp"
 #include "PubSubReader.hpp"
 #include "PubSubWriter.hpp"
+#include "UDPMessageSender.hpp"
 
 using namespace eprosima::fastrtps;
 using namespace eprosima::fastrtps::rtps;
@@ -551,6 +552,104 @@ TEST(TransportUDP, DatagramInjection)
     deliver_datagram_from_file(receivers, "datagrams/20140.bin");
 }
 
+TEST(TransportUDP, MaliciousManipulatedDataOctetsToNextHeaderIgnore)
+{
+    // Force using UDP transport
+    auto udp_transport = std::make_shared<UDPv4TransportDescriptor>();
+
+    PubSubWriter<UnboundedHelloWorldPubSubType> writer(TEST_TOPIC_NAME);
+    PubSubReader<UnboundedHelloWorldPubSubType> reader(TEST_TOPIC_NAME);
+
+    struct MaliciousManipulatedDataOctetsToNextHeader
+    {
+        std::array<char, 4> rtps_id{ {'R', 'T', 'P', 'S'} };
+        std::array<uint8_t, 2> protocol_version{ {2, 3} };
+        std::array<uint8_t, 2> vendor_id{ {0x01, 0x0F} };
+        GuidPrefix_t sender_prefix{};
+
+        struct DataSubMsg
+        {
+            struct Header
+            {
+                uint8_t submessage_id = 0x15;
+#if FASTDDS_IS_BIG_ENDIAN_TARGET
+                uint8_t flags = 0x04;
+#else
+                uint8_t flags = 0x05;
+#endif  // FASTDDS_IS_BIG_ENDIAN_TARGET
+                uint16_t octets_to_next_header = 0x30;
+                uint16_t extra_flags = 0;
+                uint16_t octets_to_inline_qos = 0x2d;
+                EntityId_t reader_id{};
+                EntityId_t writer_id{};
+                SequenceNumber_t sn{100};
+            };
+
+            struct SerializedData
+            {
+                uint16_t encapsulation;
+                uint16_t encapsulation_opts;
+                octet data[24];
+            };
+
+            Header header;
+            SerializedData payload;
+        }
+        data;
+
+        uint8_t additional_bytes[8] {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
+
+    };
+
+    UDPMessageSender fake_msg_sender;
+
+    // Set common QoS
+    reader.disable_builtin_transport().add_user_transport_to_pparams(udp_transport)
+            .history_depth(10).reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS);
+    writer.history_depth(10).reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS);
+
+    // Set custom reader locator so we can send malicious data to a known location
+    Locator_t reader_locator;
+    ASSERT_TRUE(IPLocator::setIPv4(reader_locator, "127.0.0.1"));
+    reader_locator.port = 7000;
+    reader.add_to_unicast_locator_list("127.0.0.1", 7000);
+
+    // Initialize and wait for discovery
+    reader.init();
+    ASSERT_TRUE(reader.isInitialized());
+    writer.init();
+    ASSERT_TRUE(writer.isInitialized());
+
+    reader.wait_discovery();
+    writer.wait_discovery();
+
+    auto data = default_unbounded_helloworld_data_generator();
+    reader.startReception(data);
+    writer.send(data);
+    ASSERT_TRUE(data.empty());
+
+    // Send malicious data
+    {
+        auto writer_guid = writer.datawriter_guid();
+
+        MaliciousManipulatedDataOctetsToNextHeader malicious_packet{};
+        malicious_packet.sender_prefix = writer_guid.guidPrefix;
+        malicious_packet.data.header.writer_id = writer_guid.entityId;
+        malicious_packet.data.header.reader_id = reader.datareader_guid().entityId;
+        malicious_packet.data.payload.encapsulation = CDR_LE;
+
+        CDRMessage_t msg(0);
+        uint32_t msg_len = static_cast<uint32_t>(sizeof(malicious_packet));
+        msg.init(reinterpret_cast<octet*>(&malicious_packet), msg_len);
+        msg.length = msg_len;
+        msg.pos = msg_len;
+        fake_msg_sender.send(msg, reader_locator);
+    }
+
+    // Block reader until reception finished or timeout.
+    reader.block_for_all();
+}
+
 // Test for ==operator UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method
 // Test for copy UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method
 

diff --git a/test/blackbox/common/UDPMessageSender.hpp b/test/blackbox/common/UDPMessageSender.hpp
@@ -0,0 +1,34 @@
+#include <asio/io_service.hpp>
+#include <asio/ip/udp.hpp>
+
+#include <fastdds/rtps/common/CDRMessage_t.h>
+#include <fastrtps/utils/IPLocator.h>
+
+using namespace eprosima::fastrtps;
+using namespace eprosima::fastrtps::rtps;
+
+struct UDPMessageSender
+{
+    asio::io_service service;
+    asio::ip::udp::socket socket;
+
+    UDPMessageSender()
+        : service()
+        , socket(service)
+    {
+        socket.open(asio::ip::udp::v4());
+    }
+
+    void send(
+            const CDRMessage_t& msg,
+            const Locator_t& destination)
+    {
+        std::string addr = IPLocator::toIPv4string(destination);
+        unsigned short port = static_cast<unsigned short>(destination.port);
+        auto remote = asio::ip::udp::endpoint(asio::ip::address::from_string(addr), port);
+        asio::error_code ec;
+
+        socket.send_to(asio::buffer(msg.buffer, msg.length), remote, 0, ec);
+    }
+
+};