From ffe3b0c1ff98d77ca836d3bc228522335f4027af Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 22 Dec 2023 12:02:02 +0100 Subject: [PATCH] Fix memory problem when ciphering payload (#2485) (#4153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix memory problem when ciphering payload (#2485) * Refs 13364. Regression test. Signed-off-by: Miguel Company * Refs 13364. fix build when TLS_FOUND Signed-off-by: JLBuenoLopez-eProsima * Refs 13364. Test with different lengths. Signed-off-by: Miguel Company * Refs 13364. Apply review suggestions. Signed-off-by: Miguel Company * Refs 13364. Fix #2379. Signed-off-by: Miguel Company * Include what you use in DDSBlackboxTestsSecurity.cpp Signed-off-by: Miguel Company * Refs #13364. Fix warning. Signed-off-by: Miguel Company --------- Signed-off-by: Miguel Company Signed-off-by: JLBuenoLopez-eProsima Signed-off-by: Miguel Company Co-authored-by: Miguel Company * Fix build with TLS, but not security (#4156) Signed-off-by: Miguel Company --------- Signed-off-by: Miguel Company Signed-off-by: JLBuenoLopez-eProsima Signed-off-by: Miguel Company Co-authored-by: José Luis Bueno López <69244257+JLBuenoLopez-eProsima@users.noreply.github.com> Co-authored-by: Miguel Company --- src/cpp/rtps/messages/RTPSMessageGroup.cpp | 27 ++- .../rtps/participant/RTPSParticipantImpl.h | 11 ++ test/blackbox/common/BlackboxTests.cpp | 1 + test/blackbox/common/BlackboxTests.hpp | 1 + .../blackbox/common/BlackboxTestsSecurity.cpp | 2 - .../common/BlackboxTestsTransportTCP.cpp | 4 - .../common/DDSBlackboxTestsSecurity.cpp | 180 ++++++++++++++++++ .../common/TCPReqRepHelloWorldReplier.cpp | 8 +- .../common/TCPReqRepHelloWorldReplier.hpp | 2 +- .../common/TCPReqRepHelloWorldRequester.cpp | 6 +- .../common/TCPReqRepHelloWorldRequester.hpp | 2 +- 11 files changed, 221 insertions(+), 23 deletions(-) create mode 100644 test/blackbox/common/DDSBlackboxTestsSecurity.cpp diff --git a/src/cpp/rtps/messages/RTPSMessageGroup.cpp b/src/cpp/rtps/messages/RTPSMessageGroup.cpp index 7ad2554b47e..6fef11e32b3 100644 --- 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(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 ( @@ -337,13 +348,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_)) { logError(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 index 575f9f02923..24a341e4af6 100644 --- a/src/cpp/rtps/participant/RTPSParticipantImpl.h +++ b/src/cpp/rtps/participant/RTPSParticipantImpl.h @@ -361,6 +361,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.cpp b/test/blackbox/common/BlackboxTests.cpp index c852a08be98..e5fd17e82c0 100644 --- a/test/blackbox/common/BlackboxTests.cpp +++ b/test/blackbox/common/BlackboxTests.cpp @@ -30,6 +30,7 @@ using namespace eprosima::fastrtps::rtps; //#define cout "Use Log instead!" +const char* certs_path = nullptr; uint16_t global_port = 0; bool enable_datasharing; bool use_pull_mode; diff --git a/test/blackbox/common/BlackboxTests.hpp b/test/blackbox/common/BlackboxTests.hpp index 20ac338d00c..d56fd742302 100644 --- a/test/blackbox/common/BlackboxTests.hpp +++ b/test/blackbox/common/BlackboxTests.hpp @@ -49,6 +49,7 @@ extern void blackbox_security_init(); extern void tls_init(); #endif // if TLS_FOUND +extern const char* certs_path; extern uint16_t global_port; extern bool enable_datasharing; extern bool use_pull_mode; diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index fc2f49bb650..8032e353e39 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -37,8 +37,6 @@ 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; - enum communication_type { TRANSPORT, diff --git a/test/blackbox/common/BlackboxTestsTransportTCP.cpp b/test/blackbox/common/BlackboxTestsTransportTCP.cpp index efb19cae3fd..8ffb8734eef 100644 --- a/test/blackbox/common/BlackboxTestsTransportTCP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportTCP.cpp @@ -26,10 +26,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 new file mode 100644 index 00000000000..32d325c0421 --- /dev/null +++ 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 + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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(writer); + static_cast(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(); + 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 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 index 0b01cc257d5..e8a5e08db4d 100644 --- 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) { ParticipantAttributes pattr; pattr.domainId = domainId; @@ -97,14 +97,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 index c08100b0faf..4dfceeea3c5 100644 --- a/test/blackbox/common/TCPReqRepHelloWorldReplier.hpp +++ b/test/blackbox/common/TCPReqRepHelloWorldReplier.hpp @@ -125,7 +125,7 @@ class TCPReqRepHelloWorldReplier int domainId, uint16_t listeningPort, uint32_t maxInitialPeer = 0, - const char* certs_path = nullptr); + const char* certs_folder = nullptr); bool isInitialized() const { return initialized_; diff --git a/test/blackbox/common/TCPReqRepHelloWorldRequester.cpp b/test/blackbox/common/TCPReqRepHelloWorldRequester.cpp index d90afa497d4..5359dcfd6e5 100644 --- 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 index 72991278591..1b69650629e 100644 --- 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 {