From 5a364fb188e09e4fe15892a535b7e6c98f4edd9d Mon Sep 17 00:00:00 2001 From: AKalinich-Luxoft Date: Fri, 25 May 2018 13:18:50 +0300 Subject: Fix nonfunctional CertificatePath and KeyPath parameters in INI Currently, SDL Core ignores both the CertificatePath and KeyPath keywords that would allow the system integrator to specify certificates for their environment, instead SDL Core only processes the certificate provided via the policy table. This fix makes these keywords functional. Following changes were done: - Added getters for CertificatePath and KeyPath parameters in SecurityManagerSettings class to provide another components an access to these properties - Added methods for loading certificate and private key data from the files specified by CertificatePath and KeyPath keywords - CryptoManager component implementation was updated. Now this component also read certificate data from files (if they are present and accessible) on its own initialization --- .../security_manager/security_manager_settings.h | 2 + .../include/security_manager/crypto_manager_impl.h | 24 ++++++ .../crypto_manager_settings_impl.h | 8 ++ .../security_manager/src/crypto_manager_impl.cc | 95 +++++++++++++++++++++- 4 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/components/include/security_manager/security_manager_settings.h b/src/components/include/security_manager/security_manager_settings.h index f8eaadce3e..0bbe0f4f96 100644 --- a/src/components/include/security_manager/security_manager_settings.h +++ b/src/components/include/security_manager/security_manager_settings.h @@ -54,6 +54,8 @@ class CryptoManagerSettings { virtual const std::string& certificate_data() const = 0; virtual const std::string& ciphers_list() const = 0; virtual const std::string& ca_cert_path() const = 0; + virtual const std::string& module_cert_path() const = 0; + virtual const std::string& module_key_path() const = 0; virtual size_t update_before_hours() const = 0; virtual size_t maximum_payload_size() const = 0; virtual const std::vector& force_protected_service() const = 0; diff --git a/src/components/security_manager/include/security_manager/crypto_manager_impl.h b/src/components/security_manager/include/security_manager/crypto_manager_impl.h index be0d102a36..f4a13198da 100644 --- a/src/components/security_manager/include/security_manager/crypto_manager_impl.h +++ b/src/components/security_manager/include/security_manager/crypto_manager_impl.h @@ -153,6 +153,30 @@ class CryptoManagerImpl : public CryptoManager { private: bool AreForceProtectionSettingsCorrect() const; bool set_certificate(const std::string& cert_data); + + /** + * @brief Updates certificate and private key for the current SSL context + * @param certificate new certificate to update + * @param key new private key to update + * @return true if certificate and private key were updated successfully, + * otherwise returns false + */ + bool UpdateModuleCertificateData(X509* certificate, EVP_PKEY* key); + + /** + * @brief Loads X509 certificate from file specified in CryptoManagerSettings + * @return returns pointer to the loaded X509 certificate in case of success + * otherwise returns NULL + */ + X509* LoadModuleCertificateFromFile(); + + /** + * @brief Loads private key from file specified in CryptoManagerSettings + * @return returns pointer to the loaded private key in case of success + * otherwise returns NULL + */ + EVP_PKEY* LoadModulePrivateKeyFromFile(); + const utils::SharedPtr settings_; SSL_CTX* context_; static uint32_t instance_count_; diff --git a/src/components/security_manager/include/security_manager/crypto_manager_settings_impl.h b/src/components/security_manager/include/security_manager/crypto_manager_settings_impl.h index 295a76680f..f20d3e4034 100644 --- a/src/components/security_manager/include/security_manager/crypto_manager_settings_impl.h +++ b/src/components/security_manager/include/security_manager/crypto_manager_settings_impl.h @@ -60,6 +60,14 @@ class CryptoManagerSettingsImpl : public CryptoManagerSettings { return profile_.ca_cert_path(); } + const std::string& module_cert_path() const OVERRIDE { + return profile_.cert_path(); + } + + const std::string& module_key_path() const OVERRIDE { + return profile_.key_path(); + } + size_t update_before_hours() const OVERRIDE { return profile_.update_before_hours(); } diff --git a/src/components/security_manager/src/crypto_manager_impl.cc b/src/components/security_manager/src/crypto_manager_impl.cc index f5908f8043..cf92c2d40c 100644 --- a/src/components/security_manager/src/crypto_manager_impl.cc +++ b/src/components/security_manager/src/crypto_manager_impl.cc @@ -254,6 +254,14 @@ bool CryptoManagerImpl::Init() { << '"'); } + LOG4CXX_DEBUG(logger_, "Setting up module certificate and private key"); + X509* module_certificate = LoadModuleCertificateFromFile(); + EVP_PKEY* module_key = LoadModulePrivateKeyFromFile(); + + if (!UpdateModuleCertificateData(module_certificate, module_key)) { + LOG4CXX_WARN(logger_, "Failed to update module key and certificate"); + } + guard.Dismiss(); const int verify_mode = @@ -273,7 +281,15 @@ bool CryptoManagerImpl::OnCertificateUpdated(const std::string& data) { return false; } - return set_certificate(data); + if (!set_certificate(data)) { + LOG4CXX_ERROR(logger_, "Failed to save certificate data"); + return false; + } + + X509* module_certificate = LoadModuleCertificateFromFile(); + EVP_PKEY* module_key = LoadModulePrivateKeyFromFile(); + + return UpdateModuleCertificateData(module_certificate, module_key); } SSLContext* CryptoManagerImpl::CreateSSLContext() { @@ -394,4 +410,81 @@ bool CryptoManagerImpl::set_certificate(const std::string& cert_data) { return true; } +bool CryptoManagerImpl::UpdateModuleCertificateData(X509* certificate, + EVP_PKEY* key) { + LOG4CXX_AUTO_TRACE(logger_); + if (NULL != certificate) { + if (!SSL_CTX_use_certificate(context_, certificate)) { + LOG4CXX_WARN(logger_, "Could not use certificate: " << LastError()); + return false; + } + } + + if (NULL != key) { + if (!SSL_CTX_use_PrivateKey(context_, key)) { + LOG4CXX_ERROR(logger_, "Could not use key: " << LastError()); + return false; + } + + if (!SSL_CTX_check_private_key(context_)) { + LOG4CXX_ERROR(logger_, "Private key is invalid: " << LastError()); + return false; + } + } + + LOG4CXX_DEBUG(logger_, "Certificate and key are successfully updated"); + return true; +} + +X509* CryptoManagerImpl::LoadModuleCertificateFromFile() { + LOG4CXX_AUTO_TRACE(logger_); + + const std::string cert_path = get_settings().module_cert_path(); + BIO* bio_cert = BIO_new_file(cert_path.c_str(), "r"); + if (NULL == bio_cert) { + LOG4CXX_WARN(logger_, + "Failed to open " << cert_path << " file: " << LastError()); + return NULL; + } + + utils::ScopeGuard bio_guard = utils::MakeGuard(BIO_free, bio_cert); + UNUSED(bio_guard); + + X509* module_certificate = NULL; + if (0 == PEM_read_bio_X509(bio_cert, &module_certificate, NULL, NULL)) { + LOG4CXX_ERROR(logger_, + "Failed to read certificate data from file: " << LastError()); + return NULL; + } + LOG4CXX_DEBUG(logger_, + "Module certificate was loaded: " << module_certificate); + + return module_certificate; +} + +EVP_PKEY* CryptoManagerImpl::LoadModulePrivateKeyFromFile() { + LOG4CXX_AUTO_TRACE(logger_); + + const std::string key_path = get_settings().module_key_path(); + BIO* bio_key = BIO_new_file(key_path.c_str(), "r"); + if (NULL == bio_key) { + LOG4CXX_WARN(logger_, + "Failed to open " << key_path << " file: " << LastError()); + return NULL; + } + + utils::ScopeGuard bio_guard = utils::MakeGuard(BIO_free, bio_key); + UNUSED(bio_guard); + + EVP_PKEY* module_key = NULL; + if (0 == PEM_read_bio_PrivateKey(bio_key, &module_key, NULL, NULL)) { + LOG4CXX_ERROR(logger_, + "Failed to read private key data from file: " << LastError()); + return NULL; + } + LOG4CXX_DEBUG(logger_, "Module private key was loaded: " << module_key); + + return module_key; +} + } // namespace security_manager -- cgit v1.2.1 From fb10b983d2f831c948ce4290170d6fa155dfe5bc Mon Sep 17 00:00:00 2001 From: AKalinich-Luxoft Date: Fri, 25 May 2018 13:24:52 +0300 Subject: Fixed affected mocks and UT's Added new expectations for a security tests due to some changes in the security flow --- .../mock_security_manager_settings.h | 2 ++ .../test/crypto_manager_impl_test.cc | 7 +++++++ .../test/ssl_certificate_handshake_test.cc | 12 ++++++++++++ .../security_manager/test/ssl_context_test.cc | 20 ++++++++++++++++++++ 4 files changed, 41 insertions(+) diff --git a/src/components/include/test/security_manager/mock_security_manager_settings.h b/src/components/include/test/security_manager/mock_security_manager_settings.h index 961b5e2ff8..b1c869cd1b 100644 --- a/src/components/include/test/security_manager/mock_security_manager_settings.h +++ b/src/components/include/test/security_manager/mock_security_manager_settings.h @@ -50,6 +50,8 @@ class MockCryptoManagerSettings MOCK_CONST_METHOD0(certificate_data, const std::string&()); MOCK_CONST_METHOD0(ciphers_list, const std::string&()); MOCK_CONST_METHOD0(ca_cert_path, const std::string&()); + MOCK_CONST_METHOD0(module_cert_path, const std::string&()); + MOCK_CONST_METHOD0(module_key_path, const std::string&()); MOCK_CONST_METHOD0(update_before_hours, size_t()); MOCK_CONST_METHOD0(maximum_payload_size, size_t()); MOCK_CONST_METHOD0(force_protected_service, const std::vector&()); diff --git a/src/components/security_manager/test/crypto_manager_impl_test.cc b/src/components/security_manager/test/crypto_manager_impl_test.cc index 46429ea0e9..47e63dc194 100644 --- a/src/components/security_manager/test/crypto_manager_impl_test.cc +++ b/src/components/security_manager/test/crypto_manager_impl_test.cc @@ -54,6 +54,9 @@ const std::string kAllCiphers = "ALL"; const std::string kCaCertPath = ""; const uint32_t kServiceNumber = 2u; const size_t kMaxSizeVector = 1u; +const std::string kCertPath = "certificate.crt"; +const std::string kPrivateKeyPath = "private.key"; + #ifdef __QNXNTO__ const std::string kFordCipher = SSL3_TXT_RSA_DES_192_CBC3_SHA; #else @@ -118,6 +121,10 @@ class CryptoManagerTest : public testing::Test { .WillByDefault(ReturnRef(cipher)); ON_CALL(*mock_security_manager_settings_, ca_cert_path()) .WillByDefault(ReturnRef(kCaCertPath)); + ON_CALL(*mock_security_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(kCertPath)); + ON_CALL(*mock_security_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(kPrivateKeyPath)); ON_CALL(*mock_security_manager_settings_, verify_peer()) .WillByDefault(Return(false)); } diff --git a/src/components/security_manager/test/ssl_certificate_handshake_test.cc b/src/components/security_manager/test/ssl_certificate_handshake_test.cc index 2423ab00b7..d5bb1ecaee 100644 --- a/src/components/security_manager/test/ssl_certificate_handshake_test.cc +++ b/src/components/security_manager/test/ssl_certificate_handshake_test.cc @@ -56,6 +56,10 @@ namespace custom_str = utils::custom_string; namespace { const std::string server_ca_cert_filename = "server"; const std::string client_ca_cert_filename = "client"; +const std::string client_cert_filename = "client.crt"; +const std::string server_cert_filename = "server.crt"; +const std::string client_key_filename = "client_private.key"; +const std::string server_key_filename = "server_private.key"; const std::string client_certificate = "client/client_credential.pem"; const std::string server_certificate = "server/spt_credential.pem"; const std::string server_unsigned_cert_file = @@ -126,6 +130,10 @@ class SSLHandshakeTest : public testing::TestWithParam { .WillByDefault(ReturnRef(server_ciphers_list_)); ON_CALL(*mock_server_manager_settings_, ca_cert_path()) .WillByDefault(ReturnRef(server_ca_certificate_path_)); + ON_CALL(*mock_server_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(server_cert_filename)); + ON_CALL(*mock_server_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(server_key_filename)); ON_CALL(*mock_server_manager_settings_, verify_peer()) .WillByDefault(Return(verify_peer)); } @@ -152,6 +160,10 @@ class SSLHandshakeTest : public testing::TestWithParam { .WillByDefault(ReturnRef(client_ciphers_list_)); ON_CALL(*mock_client_manager_settings_, ca_cert_path()) .WillByDefault(ReturnRef(client_ca_certificate_path_)); + ON_CALL(*mock_client_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(client_cert_filename)); + ON_CALL(*mock_client_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(client_key_filename)); ON_CALL(*mock_client_manager_settings_, verify_peer()) .WillByDefault(Return(verify_peer)); } diff --git a/src/components/security_manager/test/ssl_context_test.cc b/src/components/security_manager/test/ssl_context_test.cc index 7a4a9c3a87..20e509ebc6 100644 --- a/src/components/security_manager/test/ssl_context_test.cc +++ b/src/components/security_manager/test/ssl_context_test.cc @@ -50,6 +50,10 @@ using ::testing::NiceMock; namespace { const std::string kCaPath = ""; +const std::string kClientCertPath = "client_certificate.crt"; +const std::string kClientPrivateKeyPath = "client_private.key"; +const std::string kServerCertPath = "server_certificate.crt"; +const std::string kServerPrivateKeyPath = "server_private.key"; const uint8_t* kServerBuf; const uint8_t* kClientBuf; const std::string kAllCiphers = "ALL"; @@ -131,6 +135,10 @@ class SSLTest : public testing::Test { .WillByDefault(ReturnRef(forced_unprotected_service_)); ON_CALL(*mock_crypto_manager_settings_, force_protected_service()) .WillByDefault(ReturnRef(forced_protected_service_)); + ON_CALL(*mock_crypto_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(kServerCertPath)); + ON_CALL(*mock_crypto_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(kServerPrivateKeyPath)); const bool crypto_manager_initialization = crypto_manager_->Init(); EXPECT_TRUE(crypto_manager_initialization); @@ -160,6 +168,10 @@ class SSLTest : public testing::Test { .WillByDefault(ReturnRef(forced_unprotected_service_)); ON_CALL(*mock_client_manager_settings_, force_protected_service()) .WillByDefault(ReturnRef(forced_protected_service_)); + ON_CALL(*mock_client_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(kClientCertPath)); + ON_CALL(*mock_client_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(kClientPrivateKeyPath)); const bool client_manager_initialization = client_manager_->Init(); EXPECT_TRUE(client_manager_initialization); @@ -301,6 +313,10 @@ class SSLTestParam : public testing::TestWithParam { .WillByDefault(ReturnRef(kCaPath)); ON_CALL(*mock_crypto_manager_settings_, verify_peer()) .WillByDefault(Return(false)); + ON_CALL(*mock_crypto_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(kServerCertPath)); + ON_CALL(*mock_crypto_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(kServerPrivateKeyPath)); } void SetClientInitialValues(security_manager::Protocol protocol, @@ -321,6 +337,10 @@ class SSLTestParam : public testing::TestWithParam { .WillByDefault(ReturnRef(kCaPath)); ON_CALL(*mock_client_manager_settings_, verify_peer()) .WillByDefault(Return(false)); + ON_CALL(*mock_client_manager_settings_, module_cert_path()) + .WillByDefault(ReturnRef(kClientCertPath)); + ON_CALL(*mock_client_manager_settings_, module_key_path()) + .WillByDefault(ReturnRef(kClientPrivateKeyPath)); } utils::SharedPtr > -- cgit v1.2.1 From 8a84d14576256b4d6c66200f5c9ca0b379c2a4c7 Mon Sep 17 00:00:00 2001 From: Andrii Kalinich Date: Fri, 25 May 2018 23:56:10 +0300 Subject: Fixed leaked objects --- .../security_manager/src/crypto_manager_impl.cc | 26 +++++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/components/security_manager/src/crypto_manager_impl.cc b/src/components/security_manager/src/crypto_manager_impl.cc index cf92c2d40c..34727fedf9 100644 --- a/src/components/security_manager/src/crypto_manager_impl.cc +++ b/src/components/security_manager/src/crypto_manager_impl.cc @@ -255,8 +255,15 @@ bool CryptoManagerImpl::Init() { } LOG4CXX_DEBUG(logger_, "Setting up module certificate and private key"); + X509* module_certificate = LoadModuleCertificateFromFile(); + utils::ScopeGuard certificate_guard = + utils::MakeGuard(X509_free, module_certificate); + UNUSED(certificate_guard); + EVP_PKEY* module_key = LoadModulePrivateKeyFromFile(); + utils::ScopeGuard key_guard = utils::MakeGuard(EVP_PKEY_free, module_key); + UNUSED(key_guard); if (!UpdateModuleCertificateData(module_certificate, module_key)) { LOG4CXX_WARN(logger_, "Failed to update module key and certificate"); @@ -289,6 +296,13 @@ bool CryptoManagerImpl::OnCertificateUpdated(const std::string& data) { X509* module_certificate = LoadModuleCertificateFromFile(); EVP_PKEY* module_key = LoadModulePrivateKeyFromFile(); + utils::ScopeGuard certificate_guard = + utils::MakeGuard(X509_free, module_certificate); + UNUSED(certificate_guard); + + utils::ScopeGuard key_guard = utils::MakeGuard(EVP_PKEY_free, module_key); + UNUSED(key_guard); + return UpdateModuleCertificateData(module_certificate, module_key); } @@ -413,14 +427,14 @@ bool CryptoManagerImpl::set_certificate(const std::string& cert_data) { bool CryptoManagerImpl::UpdateModuleCertificateData(X509* certificate, EVP_PKEY* key) { LOG4CXX_AUTO_TRACE(logger_); - if (NULL != certificate) { + if (certificate) { if (!SSL_CTX_use_certificate(context_, certificate)) { LOG4CXX_WARN(logger_, "Could not use certificate: " << LastError()); return false; } } - if (NULL != key) { + if (key) { if (!SSL_CTX_use_PrivateKey(context_, key)) { LOG4CXX_ERROR(logger_, "Could not use key: " << LastError()); return false; @@ -441,7 +455,7 @@ X509* CryptoManagerImpl::LoadModuleCertificateFromFile() { const std::string cert_path = get_settings().module_cert_path(); BIO* bio_cert = BIO_new_file(cert_path.c_str(), "r"); - if (NULL == bio_cert) { + if (!bio_cert) { LOG4CXX_WARN(logger_, "Failed to open " << cert_path << " file: " << LastError()); return NULL; @@ -451,7 +465,7 @@ X509* CryptoManagerImpl::LoadModuleCertificateFromFile() { UNUSED(bio_guard); X509* module_certificate = NULL; - if (0 == PEM_read_bio_X509(bio_cert, &module_certificate, NULL, NULL)) { + if (!PEM_read_bio_X509(bio_cert, &module_certificate, NULL, NULL)) { LOG4CXX_ERROR(logger_, "Failed to read certificate data from file: " << LastError()); return NULL; @@ -467,7 +481,7 @@ EVP_PKEY* CryptoManagerImpl::LoadModulePrivateKeyFromFile() { const std::string key_path = get_settings().module_key_path(); BIO* bio_key = BIO_new_file(key_path.c_str(), "r"); - if (NULL == bio_key) { + if (!bio_key) { LOG4CXX_WARN(logger_, "Failed to open " << key_path << " file: " << LastError()); return NULL; @@ -477,7 +491,7 @@ EVP_PKEY* CryptoManagerImpl::LoadModulePrivateKeyFromFile() { UNUSED(bio_guard); EVP_PKEY* module_key = NULL; - if (0 == PEM_read_bio_PrivateKey(bio_key, &module_key, NULL, NULL)) { + if (!PEM_read_bio_PrivateKey(bio_key, &module_key, NULL, NULL)) { LOG4CXX_ERROR(logger_, "Failed to read private key data from file: " << LastError()); return NULL; -- cgit v1.2.1