From 725e8ab30e22e532bc7cfaafbc4e55d6daef2d96 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 20 Nov 2024 08:06:39 +0100 Subject: [PATCH] Improve `OpenSSL` lifecycle handling (#5384) (#5405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [22024] Improve `OpenSSL` lifecycle handling (#5384) * Refs #22024: Add BB test Signed-off-by: Mario Dominguez * Refs #22024: Make OpenSSLInit Mayers singleton Signed-off-by: Mario Dominguez * Refs #22024: Fix: Do not register atexit in OPenSSL. Instead, Comply with OpenSSL initialization and destruction. Signed-off-by: Mario Dominguez * Refs #22024: Do not reference OpenSSLInit if security features are no present Signed-off-by: Mario Dominguez --------- Signed-off-by: Mario Dominguez (cherry picked from commit 44310c4f8efabfacc55d14604d25175d8f33070a) # Conflicts: # src/cpp/rtps/RTPSDomainImpl.hpp * Solve conflicts Signed-off-by: Mario Dominguez * Apply Miguels suggestion Signed-off-by: Mario Dominguez --------- Signed-off-by: Mario Dominguez Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com> Co-authored-by: Mario Dominguez --- src/cpp/rtps/RTPSDomainImpl.hpp | 8 +++- src/cpp/rtps/security/SecurityManager.cpp | 1 - src/cpp/security/OpenSSLInit.hpp | 44 +++++++++++-------- .../blackbox/common/BlackboxTestsSecurity.cpp | 27 ++++++++++++ 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/src/cpp/rtps/RTPSDomainImpl.hpp b/src/cpp/rtps/RTPSDomainImpl.hpp index 1f736cd66f2..1abe1490805 100644 --- a/src/cpp/rtps/RTPSDomainImpl.hpp +++ b/src/cpp/rtps/RTPSDomainImpl.hpp @@ -29,9 +29,12 @@ #include #include +#include #include -#include +#if HAVE_SECURITY +#include +#endif // HAVE_SECURITY namespace eprosima { namespace fastrtps { @@ -249,6 +252,9 @@ class RTPSDomainImpl std::shared_ptr boost_singleton_handler_ { eprosima::detail:: BoostAtExitRegistry:: get_instance() }; +#if HAVE_SECURITY + std::shared_ptr openssl_singleton_handler_{ security::OpenSSLInit::get_instance() }; +#endif // HAVE_SECURITY std::mutex m_mutex; diff --git a/src/cpp/rtps/security/SecurityManager.cpp b/src/cpp/rtps/security/SecurityManager.cpp index 2d4f7736eb7..fb63fd498f3 100644 --- a/src/cpp/rtps/security/SecurityManager.cpp +++ b/src/cpp/rtps/security/SecurityManager.cpp @@ -94,7 +94,6 @@ SecurityManager::SecurityManager( participant->getRTPSParticipantAttributes().allocation.data_limits}) { assert(participant != nullptr); - static OpenSSLInit openssl_init; } SecurityManager::~SecurityManager() diff --git a/src/cpp/security/OpenSSLInit.hpp b/src/cpp/security/OpenSSLInit.hpp index 91391ab74eb..5095951f550 100644 --- a/src/cpp/security/OpenSSLInit.hpp +++ b/src/cpp/security/OpenSSLInit.hpp @@ -1,7 +1,20 @@ -#include -#include -#include -#include +// Copyright 2024 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 + +#include namespace eprosima { namespace fastrtps { @@ -14,24 +27,19 @@ class OpenSSLInit OpenSSLInit() { -#if OPENSSL_VERSION_NUMBER < 0x10100000L - OpenSSL_add_all_algorithms(); -#endif // if OPENSSL_VERSION_NUMBER < 0x10100000L + uint64_t opts = OPENSSL_INIT_NO_ATEXIT; + OPENSSL_init_crypto(opts, NULL); } ~OpenSSLInit() { -#if OPENSSL_VERSION_NUMBER < 0x10000000L - ERR_remove_state(0); - ENGINE_cleanup(); -#elif OPENSSL_VERSION_NUMBER < 0x10100000L - ERR_remove_thread_state(NULL); - ENGINE_cleanup(); -#endif // if OPENSSL_VERSION_NUMBER < 0x10000000L - RAND_cleanup(); - CRYPTO_cleanup_all_ex_data(); - ERR_free_strings(); - EVP_cleanup(); + OPENSSL_cleanup(); + } + + static std::shared_ptr get_instance() + { + static auto instance = std::make_shared(); + return instance; } }; diff --git a/test/blackbox/common/BlackboxTestsSecurity.cpp b/test/blackbox/common/BlackboxTestsSecurity.cpp index 3e2211330a2..c416eb3193d 100644 --- a/test/blackbox/common/BlackboxTestsSecurity.cpp +++ b/test/blackbox/common/BlackboxTestsSecurity.cpp @@ -5191,6 +5191,33 @@ TEST(Security, participant_stateless_secure_writer_pool_change_is_removed_upon_p EXPECT_EQ(0u, n_logs); } +// Regression test for Redmine issue #22024 +// OpenSSL assertion is not thrown when the library is abruptly finished. +TEST(Security, openssl_correctly_finishes) +{ + // Create + PubSubWriter writer("HelloWorldTopic_openssl_is_correctly_finished"); + PubSubReader reader("HelloWorldTopic_openssl_is_correctly_finished"); + + const std::string governance_file("governance_helloworld_all_enable.smime"); + const std::string permissions_file("permissions_helloworld.smime"); + + CommonPermissionsConfigure(reader, writer, governance_file, permissions_file); + + reader.init(); + writer.init(); + + ASSERT_TRUE(reader.isInitialized()); + ASSERT_TRUE(writer.isInitialized()); + + std::this_thread::sleep_for(std::chrono::seconds(2)); + + // Here we force the atexit function from openssl to be abruptly called + // i.e in a disordered way + // If OpenSSL is not correctly finished, a SIGSEGV will be thrown + std::exit(0); +} + void blackbox_security_init() { certs_path = std::getenv("CERTS_PATH");