Fix memory problem when ciphering payload (#2485)

* Refs 13364. Regression test. Signed-off-by: Miguel Company <[email protected]> * Refs 13364. fix build when TLS_FOUND Signed-off-by: JLBuenoLopez-eProsima <[email protected]> * Refs 13364. Test with different lengths. Signed-off-by: Miguel Company <[email protected]> * Refs 13364. Apply review suggestions. Signed-off-by: Miguel Company <[email protected]> * Refs 13364. Fix #2379. Signed-off-by: Miguel Company <[email protected]> * Include what you use in DDSBlackboxTestsSecurity.cpp Signed-off-by: Miguel Company <[email protected]> * Refs #13364. Fix warning. Signed-off-by: Miguel Company <[email protected]> --------- Signed-off-by: Miguel Company <[email protected]> Signed-off-by: JLBuenoLopez-eProsima <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]>
eProsima · Dec 16, 2023 · 25dee05 · 25dee05
1 parent 7c75b6d
commit 25dee05
Show file tree

Hide file tree

Showing 10 changed files with 221 additions and 22 deletions.
diff --git a/src/cpp/rtps/messages/RTPSMessageGroup.cpp b/src/cpp/rtps/messages/RTPSMessageGroup.cpp
@@ -74,18 +74,29 @@ static bool data_exceeds_limitation(
 }
 
 static bool append_message(
+        RTPSParticipantImpl* participant,
         CDRMessage_t* full_msg,
         CDRMessage_t* submsg)
 {
-#ifndef FASTDDS_STATISTICS
-    return CDRMessage::appendMsg(full_msg, submsg);
-#else
+    static_cast<void>(participant);
+
+    uint32_t extra_size = 0;
+
+#if HAVE_SECURITY
+    // Avoid full message growing over estimated extra size for RTPS encryption
+    extra_size += participant->calculate_extra_size_for_rtps_message();
+#endif  // HAVE_SECURITY
+
+#ifdef FASTDDS_STATISTICS
     // Keep room for the statistics submessage by reducing max_size while appending submessage
-    full_msg->max_size -= eprosima::fastdds::statistics::rtps::statistics_submessage_length;
+    extra_size += eprosima::fastdds::statistics::rtps::statistics_submessage_length;
+#endif  // FASTDDS_STATISTICS
+
+    full_msg->max_size -= extra_size;
     bool ret_val = CDRMessage::appendMsg(full_msg, submsg);
-    full_msg->max_size += eprosima::fastdds::statistics::rtps::statistics_submessage_length;
+    full_msg->max_size += extra_size;
+
     return ret_val;
-#endif // FASTDDS_STATISTICS
 }
 
 bool sort_changes_group (
@@ -336,13 +347,13 @@ bool RTPSMessageGroup::insert_submessage(
         const GuidPrefix_t& destination_guid_prefix,
         bool is_big_submessage)
 {
-    if (!append_message(full_msg_, submessage_msg_))
+    if (!append_message(participant_, full_msg_, submessage_msg_))
     {
         // Retry
         flush_and_reset();
         add_info_dst_in_buffer(full_msg_, destination_guid_prefix);
 
-        if (!append_message(full_msg_, submessage_msg_))
+        if (!append_message(participant_, full_msg_, submessage_msg_))
         {
             EPROSIMA_LOG_ERROR(RTPS_WRITER, "Cannot add RTPS submesage to the CDRMessage. Buffer too small");
             return false;

diff --git a/src/cpp/rtps/participant/RTPSParticipantImpl.h b/src/cpp/rtps/participant/RTPSParticipantImpl.h
@@ -380,6 +380,17 @@ class RTPSParticipantImpl
             uint32_t length);
 
 #if HAVE_SECURITY
+    uint32_t calculate_extra_size_for_rtps_message()
+    {
+        uint32_t ret_val = 0u;
+        if (security_attributes_.is_rtps_protected)
+        {
+            ret_val = m_security_manager.calculate_extra_size_for_rtps_message();
+        }
+
+        return ret_val;
+    }
+
     security::SecurityManager& security_manager()
     {
         return m_security_manager;

diff --git a/test/blackbox/common/BlackboxTests.hpp b/test/blackbox/common/BlackboxTests.hpp
@@ -46,6 +46,7 @@
 #include <functional>
 
 #if HAVE_SECURITY
+extern const char* certs_path;
 extern void blackbox_security_init();
 #endif // if HAVE_SECURITY
 #if TLS_FOUND

diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp
@@ -37,7 +37,7 @@ using namespace eprosima::fastrtps::rtps;
 using test_UDPv4Transport = eprosima::fastdds::rtps::test_UDPv4Transport;
 using test_UDPv4TransportDescriptor = eprosima::fastdds::rtps::test_UDPv4TransportDescriptor;
 
-static const char* certs_path = nullptr;
+const char* certs_path = nullptr;
 
 enum communication_type
 {

diff --git a/test/blackbox/common/BlackboxTestsTransportTCP.cpp b/test/blackbox/common/BlackboxTestsTransportTCP.cpp
@@ -30,10 +30,6 @@
 using namespace eprosima::fastrtps;
 using namespace eprosima::fastrtps::rtps;
 
-#if TLS_FOUND
-static const char* certs_path = nullptr;
-#endif // if TLS_FOUND
-
 enum communication_type
 {
     TRANSPORT

diff --git a/test/blackbox/common/DDSBlackboxTestsSecurity.cpp b/test/blackbox/common/DDSBlackboxTestsSecurity.cpp
@@ -0,0 +1,180 @@
+// Copyright 2022 Proyectos y Sistemas de Mantenimiento SL (eProsima).
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "BlackboxTests.hpp"
+
+#if HAVE_SECURITY
+
+#include <gtest/gtest.h>
+
+#include <thread>
+
+#include <fastdds/dds/domain/DomainParticipant.hpp>
+#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
+#include <fastdds/dds/publisher/DataWriter.hpp>
+#include <fastdds/dds/publisher/DataWriterListener.hpp>
+#include <fastdds/dds/publisher/Publisher.hpp>
+#include <fastdds/dds/subscriber/DataReader.hpp>
+#include <fastdds/dds/subscriber/Subscriber.hpp>
+#include <fastrtps/transport/UDPv4TransportDescriptor.h>
+#include <fastrtps/types/DynamicDataFactory.h>
+#include <fastrtps/types/DynamicTypeBuilder.h>
+#include <fastrtps/types/DynamicTypeBuilderFactory.h>
+#include <fastrtps/types/DynamicTypeBuilderPtr.h>
+
+namespace fastdds = ::eprosima::fastdds::dds;
+using namespace eprosima::fastrtps;
+using namespace eprosima::fastrtps::rtps;
+using namespace eprosima::fastrtps::types;
+
+void set_authentication_config(
+        rtps::PropertySeq& props)
+{
+    static constexpr auto kAuthPlugin{ "dds.sec.auth.plugin" };
+    static constexpr auto kAuthPluginNameValue{ "builtin.PKI-DH" };
+    static constexpr auto kIdentityCA{ "dds.sec.auth.builtin.PKI-DH.identity_ca" };
+    static constexpr auto kIdentityCert{ "dds.sec.auth.builtin.PKI-DH.identity_certificate" };
+    static constexpr auto kIdentityPrivateKey{ "dds.sec.auth.builtin.PKI-DH.private_key" };
+
+    props.emplace_back(kAuthPlugin, kAuthPluginNameValue);
+    props.emplace_back(kIdentityCA, "file://" + std::string(certs_path) + "/maincacert.pem");
+    props.emplace_back(kIdentityCert, "file://" + std::string(certs_path) + "/mainsubcert.pem");
+    props.emplace_back(kIdentityPrivateKey, "file://" + std::string(certs_path) + "/mainsubkey.pem");
+}
+
+void set_participant_crypto_config(
+        rtps::PropertySeq& props)
+{
+    static constexpr auto kCryptoPlugin{ "dds.sec.crypto.plugin" };
+    static constexpr auto kCryptoPluginNameValue{ "builtin.AES-GCM-GMAC" };
+    static constexpr auto kRtpsProtectionKind{ "rtps.participant.rtps_protection_kind" };
+    static constexpr auto kProtectionKindValue{ "ENCRYPT" };
+
+    props.emplace_back(kCryptoPlugin, kCryptoPluginNameValue);
+    props.emplace_back(kRtpsProtectionKind, kProtectionKindValue);
+}
+
+void test_big_message_corner_case(
+        const std::string& name,
+        uint32_t array_length)
+{
+    std::cout << "==== Checking with length = " << array_length << " ====" << std::endl;
+
+    struct WriterListener : public fastdds::DataWriterListener
+    {
+        WriterListener(
+                std::atomic_bool& cond_ref)
+            : cond_(cond_ref)
+        {
+            cond_ = false;
+        }
+
+        void on_publication_matched(
+                fastdds::DataWriter* writer,
+                const fastdds::PublicationMatchedStatus& info)
+        {
+            static_cast<void>(writer);
+            static_cast<void>(info);
+
+            cond_ = true;
+        }
+
+    private:
+
+        std::atomic_bool& cond_;
+    };
+
+    auto qos{ fastdds::PARTICIPANT_QOS_DEFAULT };
+    set_authentication_config(qos.properties().properties());
+    set_participant_crypto_config(qos.properties().properties());
+    auto transport = std::make_shared<rtps::UDPv4TransportDescriptor>();
+    transport->interfaceWhiteList.push_back("127.0.0.1");
+    qos.transport().use_builtin_transports = false;
+    qos.transport().user_transports.push_back(transport);
+    auto participant = fastdds::DomainParticipantFactory::get_instance()->create_participant(0, qos);
+    ASSERT_NE(nullptr, participant);
+
+    std::vector<uint32_t> lengths = { array_length };
+    DynamicType_ptr base_type = DynamicTypeBuilderFactory::get_instance()->create_char8_type();
+    DynamicTypeBuilder_ptr builder =
+            DynamicTypeBuilderFactory::get_instance()->create_array_builder(base_type, lengths);
+    builder->set_name(name);
+    DynamicType_ptr array_type = builder->build();
+
+    fastdds::TypeSupport type_support(new eprosima::fastrtps::types::DynamicPubSubType(array_type));
+    type_support.get()->auto_fill_type_information(false);
+    type_support.get()->auto_fill_type_object(false);
+    EXPECT_EQ(ReturnCode_t::RETCODE_OK, participant->register_type(type_support));
+
+    auto topic = participant->create_topic(name, name, fastdds::TOPIC_QOS_DEFAULT);
+    ASSERT_NE(nullptr, topic);
+    auto subscriber = participant->create_subscriber(fastdds::SUBSCRIBER_QOS_DEFAULT);
+    ASSERT_NE(nullptr, subscriber);
+    auto reader = subscriber->create_datareader(topic, fastdds::DATAREADER_QOS_DEFAULT);
+    ASSERT_NE(nullptr, reader);
+    auto publisher = participant->create_publisher(fastdds::PUBLISHER_QOS_DEFAULT);
+    ASSERT_NE(nullptr, publisher);
+
+    std::atomic_bool writer_matched{ false };
+    WriterListener wlistener(writer_matched);
+    auto writer = publisher->create_datawriter(topic, fastdds::DATAWRITER_QOS_DEFAULT, &wlistener,
+                    fastdds::StatusMask::publication_matched());
+    ASSERT_NE(nullptr, writer);
+
+    // Wait for discovery to occur
+    while (!writer_matched)
+    {
+        std::this_thread::sleep_for(std::chrono::milliseconds(100));
+    }
+
+    auto data = eprosima::fastrtps::types::DynamicDataFactory::get_instance()->create_data(array_type);
+    EXPECT_EQ(ReturnCode_t::RETCODE_OK, writer->write(data, fastdds::HANDLE_NIL));
+
+    fastdds::SampleInfo info{};
+    bool taken = false;
+    for (size_t n_tries = 0; n_tries < 6u; ++n_tries)
+    {
+        if (ReturnCode_t::RETCODE_OK == reader->take_next_sample(data, &info))
+        {
+            taken = true;
+            break;
+        }
+        std::this_thread::sleep_for(std::chrono::milliseconds(500));
+    }
+
+    EXPECT_TRUE(taken);
+    EXPECT_EQ(ReturnCode_t::RETCODE_OK, participant->delete_contained_entities());
+    fastdds::DomainParticipantFactory::get_instance()->delete_participant(participant);
+}
+
+TEST(DDSSecurity, big_message_corner_case)
+{
+    const uint32_t array_lengths[] =
+    {
+        // Working case
+        65200,
+        // Failing cases
+        65240, 65252, 65260, 65300,
+        // Working case
+        65400
+    };
+
+    std::string topic_name = TEST_TOPIC_NAME;
+    for (uint32_t len : array_lengths)
+    {
+        test_big_message_corner_case(topic_name, len);
+    }
+}
+
+#endif  // HAVE_SECURITY
diff --git a/test/blackbox/common/TCPReqRepHelloWorldReplier.cpp b/test/blackbox/common/TCPReqRepHelloWorldReplier.cpp
@@ -65,7 +65,7 @@ void TCPReqRepHelloWorldReplier::init(
         int domainId,
         uint16_t listeningPort,
         uint32_t maxInitialPeer,
-        const char* certs_path,
+        const char* certs_folder,
         bool use_busy_listener)
 {
     ParticipantAttributes pattr;
@@ -98,14 +98,14 @@ void TCPReqRepHelloWorldReplier::init(
     }
     descriptor->add_listener_port(listeningPort);
 
-    if (certs_path != nullptr)
+    if (certs_folder != nullptr)
     {
         using TLSOptions = TCPTransportDescriptor::TLSConfig::TLSOptions;
         using TLSVerifyMode = TCPTransportDescriptor::TLSConfig::TLSVerifyMode;
         descriptor->apply_security = true;
         descriptor->tls_config.password = "testkey";
-        descriptor->tls_config.cert_chain_file = std::string(certs_path) + "/mainsubcert.pem";
-        descriptor->tls_config.private_key_file = std::string(certs_path) + "/mainsubkey.pem";
+        descriptor->tls_config.cert_chain_file = std::string(certs_folder) + "/mainsubcert.pem";
+        descriptor->tls_config.private_key_file = std::string(certs_folder) + "/mainsubkey.pem";
         descriptor->tls_config.verify_mode = TLSVerifyMode::VERIFY_PEER;
         descriptor->tls_config.add_option(TLSOptions::DEFAULT_WORKAROUNDS);
         descriptor->tls_config.add_option(TLSOptions::SINGLE_DH_USE);

diff --git a/test/blackbox/common/TCPReqRepHelloWorldReplier.hpp b/test/blackbox/common/TCPReqRepHelloWorldReplier.hpp
@@ -138,7 +138,7 @@ class TCPReqRepHelloWorldReplier
             int domainId,
             uint16_t listeningPort,
             uint32_t maxInitialPeer = 0,
-            const char* certs_path = nullptr,
+            const char* certs_folder = nullptr,
             bool use_busy_listener = false);
     bool isInitialized() const
     {

diff --git a/test/blackbox/common/TCPReqRepHelloWorldRequester.cpp b/test/blackbox/common/TCPReqRepHelloWorldRequester.cpp
@@ -68,7 +68,7 @@ void TCPReqRepHelloWorldRequester::init(
         int domainId,
         uint16_t listeningPort,
         uint32_t maxInitialPeer,
-        const char* certs_path,
+        const char* certs_folder,
         bool force_localhost)
 {
     ParticipantAttributes pattr;
@@ -133,13 +133,13 @@ void TCPReqRepHelloWorldRequester::init(
         descriptor->maxInitialPeersRange = maxInitialPeer;
     }
 
-    if (certs_path != nullptr)
+    if (certs_folder != nullptr)
     {
         using TLSOptions = TCPTransportDescriptor::TLSConfig::TLSOptions;
         using TLSVerifyMode = TCPTransportDescriptor::TLSConfig::TLSVerifyMode;
         descriptor->apply_security = true;
         //descriptor->tls_config.password = "testkey";
-        descriptor->tls_config.verify_file = std::string(certs_path) + "/maincacert.pem";
+        descriptor->tls_config.verify_file = std::string(certs_folder) + "/maincacert.pem";
         descriptor->tls_config.verify_mode = TLSVerifyMode::VERIFY_PEER;
         descriptor->tls_config.add_option(TLSOptions::DEFAULT_WORKAROUNDS);
         descriptor->tls_config.add_option(TLSOptions::SINGLE_DH_USE);

diff --git a/test/blackbox/common/TCPReqRepHelloWorldRequester.hpp b/test/blackbox/common/TCPReqRepHelloWorldRequester.hpp
@@ -126,7 +126,7 @@ class TCPReqRepHelloWorldRequester
             int domainId,
             uint16_t listeningPort,
             uint32_t maxInitialPeer = 0,
-            const char* certs_path = nullptr,
+            const char* certs_folder = nullptr,
             bool force_localhost = false);
     bool isInitialized() const
     {