Fix several issues on security code [13374]

src/cpp/rtps/messages/RTPSMessageGroup.cpp

@@ -173,6 +173,9 @@ RTPSMessageGroup::RTPSMessageGroup(
     {
         encrypt_msg_ = &(send_buffer_->rtpsmsg_encrypt_);
         CDRMessage::initCDRMsg(encrypt_msg_);
+
+        // Avoid full message growing over estimated extra size for RTPS encryption
+        full_msg_->max_size -= participant->calculate_extra_size_for_rtps_message();
     }
 #endif // if HAVE_SECURITY
 }
@@ -196,6 +199,14 @@ RTPSMessageGroup::~RTPSMessageGroup() noexcept(false)
 {
     try
     {
+#if HAVE_SECURITY
+        if (participant_->is_secure())
+        {
+            // Restore RTPS encryption overhead, so it can be reused on future calls
+            full_msg_->max_size += participant_->calculate_extra_size_for_rtps_message();
+        }
+#endif // if HAVE_SECURITY
+
         send();
     }
     catch (...)

src/cpp/rtps/participant/RTPSParticipantImpl.h

@@ -357,6 +357,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;

src/cpp/rtps/security/SecurityManager.cpp

@@ -550,6 +550,12 @@ void SecurityManager::remove_discovered_participant_info(
 
         authentication_plugin_->return_identity_handle(auth_ptr->identity_handle_, exception);
         auth_ptr->identity_handle_ = nullptr;
+
+        if (auth_ptr->change_sequence_number_ != SequenceNumber_t::unknown())
+        {
+            participant_stateless_message_writer_history_->remove_change(auth_ptr->change_sequence_number_);
+            auth_ptr->change_sequence_number_ = SequenceNumber_t::unknown();
+        }
     }
 }
 

src/cpp/security/artifact_providers/FileProvider.cpp

@@ -202,7 +202,11 @@ EVP_PKEY* FileProvider::load_private_key(
                             (void*)password.c_str());
 
             // Verify private key.
-            if (!X509_check_private_key(certificate, returnedValue))
+            if (nullptr == returnedValue)
+            {
+                exception = _SecurityException_(std::string("Error obtaining private key ") + pkey.substr(7));
+            }
+            else if (!X509_check_private_key(certificate, returnedValue))
             {
                 exception = _SecurityException_(std::string("Error verifying private key ") + pkey.substr(7));
                 EVP_PKEY_free(returnedValue);

src/cpp/security/authentication/PKIDH.cpp

@@ -601,11 +601,21 @@ static EVP_PKEY* generate_dh_key(
         params = EVP_PKEY_new();
         if (params != nullptr)
         {
-            if (1 != EVP_PKEY_set1_DH(params, DH_get_2048_256()))
+            DH* dh = DH_get_2048_256();
+            if (dh != nullptr)
             {
-                exception = _SecurityException_("Cannot set default parameters: ");
-                EVP_PKEY_free(params);
-                return nullptr;
+#if IS_OPENSSL_1_1_1d
+                int dh_type = DH_get0_q(dh) == NULL ? EVP_PKEY_DH : EVP_PKEY_DHX;
+                if (EVP_PKEY_assign(params, dh_type, dh) <= 0)
+#else
+                if (EVP_PKEY_assign_DH(params, dh) <= 0)
+#endif // if IS_OPENSSL_1_1_1d
+                {
+                    exception = _SecurityException_("Cannot set default parameters: ");
+                    DH_free(dh);
+                    EVP_PKEY_free(params);
+                    return nullptr;
+                }
             }
         }
         else
@@ -775,6 +785,7 @@ static EVP_PKEY* generate_dh_peer_key(
                     else
                     {
                         exception = _SecurityException_("OpenSSL library cannot set dh in pkey");
+                        DH_free(dh);
                     }
 
                     EVP_PKEY_free(key);

test/blackbox/api/fastrtps_deprecated/PubSubReader.hpp

@@ -459,6 +459,45 @@ class PubSubReader
         std::cout << "Reader discovery finished..." << std::endl;
     }
 
+    bool wait_participant_discovery(
+            unsigned int min_participants = 1,
+            std::chrono::seconds timeout = std::chrono::seconds::zero())
+    {
+        bool ret_value = true;
+        std::unique_lock<std::mutex> lock(mutexDiscovery_);
+
+        std::cout << "Reader is waiting discovery of at least " << min_participants << " participants..." << std::endl;
+
+        if (timeout == std::chrono::seconds::zero())
+        {
+            cvDiscovery_.wait(lock, [&]()
+                    {
+                        return participant_matched_ >= min_participants;
+                    });
+        }
+        else
+        {
+            if (!cvDiscovery_.wait_for(lock, timeout, [&]()
+                    {
+                        return participant_matched_ >= min_participants;
+                    }))
+            {
+                ret_value = false;
+            }
+        }
+
+        if (ret_value)
+        {
+            std::cout << "Reader participant discovery finished successfully..." << std::endl;
+        }
+        else
+        {
+            std::cout << "Reader participant discovery finished unsuccessfully..." << std::endl;
+        }
+
+        return ret_value;
+    }
+
     bool wait_participant_undiscovery(
             std::chrono::seconds timeout = std::chrono::seconds::zero())
     {

test/blackbox/common/BlackboxTests.hpp

@@ -43,6 +43,7 @@
 #include <functional>
 
 #if HAVE_SECURITY
+extern const char* certs_path;
 extern void blackbox_security_init();
 #endif // if HAVE_SECURITY
 #if TLS_FOUND

test/blackbox/common/BlackboxTestsSecurity.cpp

@@ -30,12 +30,14 @@
 #include <fastdds/rtps/transport/shared_mem/SharedMemTransportDescriptor.h>
 #include <rtps/transport/test_UDPv4Transport.h>
 
+#include <fastdds/dds/log/Log.hpp>
+
 using namespace eprosima::fastrtps;
 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
 {
@@ -438,6 +440,83 @@ TEST_P(Security, BuiltinAuthenticationPlugin_PKIDH_lossy_conditions)
     reader.wait_discovery();
 }
 
+// Regresion test for Refs #13295, github #2362
+TEST_P(Security, BuiltinAuthenticationPlugin_second_participant_creation_loop)
+{
+    constexpr size_t n_loops = 101;
+
+    using Log = eprosima::fastdds::dds::Log;
+    using LogConsumer = eprosima::fastdds::dds::LogConsumer;
+
+    // A LogConsumer that just counts the number of entries consumed
+    struct TestConsumer : public LogConsumer
+    {
+        TestConsumer(
+                std::atomic_size_t& n_logs_ref)
+            : n_logs_(n_logs_ref)
+        {
+        }
+
+        void Consume(
+                const Log::Entry&) override
+        {
+            ++n_logs_;
+        }
+
+    private:
+
+        std::atomic_size_t& n_logs_;
+    };
+
+    // Counter for log entries
+    std::atomic<size_t>n_logs{};
+
+    // Prepare Log module to check that no SECURITY errors are produced
+    Log::SetCategoryFilter(std::regex("SECURITY"));
+    Log::SetVerbosity(Log::Kind::Error);
+    Log::ClearConsumers();
+    Log::RegisterConsumer(std::unique_ptr<LogConsumer>(new TestConsumer(n_logs)));
+
+    // Prepare participant properties
+    PropertyPolicy property_policy;
+    property_policy.properties().emplace_back(Property("dds.sec.auth.plugin", "builtin.PKI-DH"));
+    property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.identity_ca",
+            "file://" + std::string(certs_path) + "/maincacert.pem"));
+    property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.identity_certificate",
+            "file://" + std::string(certs_path) + "/mainpubcert.pem"));
+    property_policy.properties().emplace_back(Property("dds.sec.auth.builtin.PKI-DH.private_key",
+            "file://" + std::string(certs_path) + "/mainpubkey.pem"));
+
+    // Create the participant being checked
+    PubSubReader<HelloWorldPubSubType> main_participant("HelloWorldTopic");
+    main_participant.property_policy(property_policy).init();
+    EXPECT_TRUE(main_participant.isInitialized());
+
+    // Perform a loop in which we create another participant, and destroy it just after it has been discovered.
+    // This is the best reproducer of the issue, as authentication messages should be sent when a remote participant
+    // is discovered.
+    for (size_t n = 1; n <= n_loops; ++n)
+    {
+        std::cout << "Iteration " << n << std::endl;
+
+        // Wait for undiscovery so we can wait for discovery below
+        EXPECT_TRUE(main_participant.wait_participant_undiscovery());
+
+        // Create another participant with authentication enabled
+        PubSubParticipant<HelloWorldPubSubType> other_participant(0, 0, 0, 0);
+        EXPECT_TRUE(other_participant.property_policy(property_policy).init_participant());
+
+        // Wait for the new participant to be discovered by the main one
+        EXPECT_TRUE(main_participant.wait_participant_discovery());
+
+        // The created participant gets out of scope here, and is destroyed
+    }
+
+    // No SECURITY error logs should have been produced
+    Log::Flush();
+    EXPECT_EQ(0u, n_logs);
+}
+
 TEST_P(Security, BuiltinAuthenticationAndCryptoPlugin_besteffort_rtps_ok)
 {
     PubSubReader<HelloWorldPubSubType> reader(TEST_TOPIC_NAME);