From 7eb900bd897582f1f0277d70d5d4821e22845c5e Mon Sep 17 00:00:00 2001 From: Tomasz Swierczek Date: Tue, 1 Oct 2019 08:45:27 +0200 Subject: [PATCH] Fix compiler warnings & refactor code after security review Change-Id: I9ab0dccd7ed167a01bbceb5ac5195aca21f5414b --- CMakeLists.txt | 4 ++ src/dummy-backend/dcm-backend-api-dummy.cpp | 13 +++-- src/dummy-backend/dummycryptobackendcontext.cpp | 21 +++++-- src/kse-backend/dcm-backend-api-kse.cpp | 13 +++-- src/kse-backend/ksebackendcontext.cpp | 24 ++++++-- src/kse-backend/soresolver.cpp | 2 + src/kse-backend/tools/konaise_tool.cpp | 77 +++++++++++++------------ 7 files changed, 94 insertions(+), 60 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6944372..552c499 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -39,6 +39,10 @@ ENDIF() INCLUDE(cmake/CheckFrameworks.cmake) INCLUDE(cmake/CStandard.cmake) +ADD_DEFINITIONS("-Werror") +ADD_DEFINITIONS("-Wall") +ADD_DEFINITIONS("-Wextra") + SET(CMAKE_POSITION_INDEPENDENT_CODE "True") SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -pie") diff --git a/src/dummy-backend/dcm-backend-api-dummy.cpp b/src/dummy-backend/dcm-backend-api-dummy.cpp index 744e86e..28de132 100644 --- a/src/dummy-backend/dcm-backend-api-dummy.cpp +++ b/src/dummy-backend/dcm-backend-api-dummy.cpp @@ -28,26 +28,27 @@ void dcm_backend_create_key_context(dcm_backend_context& ctx, void dcm_backend_free_key_context(dcm_backend_context& ctx) { delete static_cast(ctx.backend); + ctx.backend = nullptr; } int dcm_backend_request_certificate_chain(dcm_backend_context& ctx, std::string& mutable_chain) { - return static_cast(ctx.backend) - ->request_certificate_chain(mutable_chain); + return ctx.backend ? static_cast(ctx.backend) + ->request_certificate_chain(mutable_chain) : -1; } int dcm_backend_sign_crypto_data(dcm_backend_context& ctx, MessageDigestType digestType, const std::string& dataToSign, std::string& digestResult) { - return static_cast(ctx.backend) - ->sign_crypto_data(digestType, dataToSign, digestResult); + return ctx.backend ? static_cast(ctx.backend) + ->sign_crypto_data(digestType, dataToSign, digestResult) : -1; } CryptoKeyType dcm_backend_key_type(dcm_backend_context& ctx) { - return static_cast(ctx.backend)->key_type(); + return ctx.backend ? static_cast(ctx.backend)->key_type() : CryptoKeyType::CRYPTO_KEY_TYPE_INVALID; } unsigned int dcm_backend_key_length(dcm_backend_context& ctx) { - return static_cast(ctx.backend)->key_length(); + return ctx.backend ? static_cast(ctx.backend)->key_length() : 0; } diff --git a/src/dummy-backend/dummycryptobackendcontext.cpp b/src/dummy-backend/dummycryptobackendcontext.cpp index 2d35e88..5b8d9ed 100644 --- a/src/dummy-backend/dummycryptobackendcontext.cpp +++ b/src/dummy-backend/dummycryptobackendcontext.cpp @@ -110,15 +110,23 @@ int dummy_crypto_backend_context::sign_crypto_data(MessageDigestType digestType, } size_t sig_len = 0; - digestResult.resize(MBEDTLS_MPI_MAX_SIZE); - BOOST_LOG_SEV(dcm_logger::get(), log_severity::debug) << "Maximum digest size is " << digestResult.size(); + BOOST_LOG_SEV(dcm_logger::get(), log_severity::debug) << "Maximum digest size is " << MBEDTLS_MPI_MAX_SIZE; + + char* output = (char*)malloc(MBEDTLS_MPI_MAX_SIZE); + + if (!output) { + BOOST_LOG_SEV(dcm_logger::get(), log_severity::error) << "Can't allocate output buffer for signing"; + error = -1; + mbedtls_pk_free(&pk); + return error; + } error = mbedtls_pk_sign(&pk, static_cast(digestType), (const unsigned char *)dataToSign.c_str(), dataToSign.size(), - (unsigned char *)digestResult.c_str(), + (unsigned char *)output, &sig_len, &mbedtls_ctr_drbg_random, &fCtrDrbg); @@ -127,10 +135,11 @@ int dummy_crypto_backend_context::sign_crypto_data(MessageDigestType digestType, BOOST_LOG_SEV(dcm_logger::get(), log_severity::error) << "Signature generation failed"; } else { BOOST_LOG_SEV(dcm_logger::get(), log_severity::debug) << "Signature size is " << sig_len; - digestResult.resize(sig_len); + digestResult = std::string(output, sig_len); } mbedtls_pk_free(&pk); + free(output); return error; } @@ -143,7 +152,7 @@ CryptoKeyType dummy_crypto_backend_context::dummy_crypto_backend_context::key_ty unsigned int dummy_crypto_backend_context::key_length() { BOOST_LOG_FUNCTION(); - unsigned int keyLength = 0; + size_t keyLength = 0; mbedtls_pk_context pk; mbedtls_pk_init(&pk); @@ -171,5 +180,7 @@ unsigned int dummy_crypto_backend_context::key_length() keyLength = mbedtls_pk_get_bitlen(&pk); mbedtls_pk_free(&pk); + assert(UINT_MAX >= keyLength); + return keyLength; } diff --git a/src/kse-backend/dcm-backend-api-kse.cpp b/src/kse-backend/dcm-backend-api-kse.cpp index 5ececda..21d3891 100644 --- a/src/kse-backend/dcm-backend-api-kse.cpp +++ b/src/kse-backend/dcm-backend-api-kse.cpp @@ -30,26 +30,27 @@ void dcm_backend_create_key_context(dcm_backend_context& ctx, void dcm_backend_free_key_context(dcm_backend_context& ctx) { delete static_cast(ctx.backend); + ctx.backend= nullptr; } int dcm_backend_request_certificate_chain(dcm_backend_context& ctx, std::string& mutable_chain) { - return static_cast(ctx.backend) - ->request_certificate_chain(mutable_chain); + return ctx.backend ? static_cast(ctx.backend) + ->request_certificate_chain(mutable_chain) : -1; } int dcm_backend_sign_crypto_data(dcm_backend_context& ctx, MessageDigestType digestType, const std::string& dataToSign, std::string& digestResult) { - return static_cast(ctx.backend) - ->sign_crypto_data(digestType, dataToSign, digestResult); + return ctx.backend ? static_cast(ctx.backend) + ->sign_crypto_data(digestType, dataToSign, digestResult) : -1; } CryptoKeyType dcm_backend_key_type(dcm_backend_context& ctx) { - return static_cast(ctx.backend)->key_type(); + return ctx.backend ? static_cast(ctx.backend)->key_type() : CryptoKeyType::CRYPTO_KEY_TYPE_INVALID; } unsigned int dcm_backend_key_length(dcm_backend_context& ctx) { - return static_cast(ctx.backend)->key_length(); + return ctx.backend ? static_cast(ctx.backend)->key_length() : 0; } diff --git a/src/kse-backend/ksebackendcontext.cpp b/src/kse-backend/ksebackendcontext.cpp index a7b2c36..dd9a890 100644 --- a/src/kse-backend/ksebackendcontext.cpp +++ b/src/kse-backend/ksebackendcontext.cpp @@ -72,8 +72,16 @@ int kse_backend_context::get_certificate(unsigned int index, std::string& outcer } auto& resolver(backend->get_so_resolver()); - int error = resolver.invoke(&kse_get_certificate_key, + int error = 0; + try { + error = resolver.invoke(&kse_get_certificate_key, method_name, index, &cert); + } catch(...) { + BOOST_LOG_SEV(dcm_logger::get(), log_severity::error) + << "KSE: Got exception when calling resolver.invoke"; + throw; + } + if(error != 0) { BOOST_LOG_SEV(dcm_logger::get(), log_severity::error) << "Failed to get certificate. error=" << error; @@ -111,17 +119,17 @@ int kse_backend_context::request_certificate_chain(std::string& mutable_chain) int error = 0; - if(error = get_certificate(LEAF_CERT_INDEX, leaf_cert)) + if((error = get_certificate(LEAF_CERT_INDEX, leaf_cert))) return error; - if(error = get_certificate(SUBCA_CERT_INDEX, subca_cert)) + if((error = get_certificate(SUBCA_CERT_INDEX, subca_cert))) return error; x509_crt_rewriter cert_writer; - if(error = cert_writer.parse(reinterpret_cast(leaf_cert.c_str()), + if((error = cert_writer.parse(reinterpret_cast(leaf_cert.c_str())), leaf_cert.length()+1)) { return error; } - if(error = cert_writer.parse(reinterpret_cast(subca_cert.c_str()), + if((error = cert_writer.parse(reinterpret_cast(subca_cert.c_str())), subca_cert.length()+1)) { return error; } @@ -145,7 +153,11 @@ hal_hash_type _get_hal_hash_type(MessageDigestType digestType) { case MD_SHA256: return HAL_HASH_SHA256; case MD_SHA384: return HAL_HASH_SHA384; case MD_SHA512: return HAL_HASH_SHA512; - defalut: return HAL_HASH_UNKNOWN; + case MD_NONE: + case MD_MD2: + case MD_MD4: + case MD_RIPEMD160: + defalut: return HAL_HASH_UNKNOWN; } } diff --git a/src/kse-backend/soresolver.cpp b/src/kse-backend/soresolver.cpp index b1208c1..e3481cb 100644 --- a/src/kse-backend/soresolver.cpp +++ b/src/kse-backend/soresolver.cpp @@ -64,6 +64,8 @@ void * so_resolver::resolve_function(const int * key_ptr, const char * name) noe fCache.emplace(key_ptr, sym); } } catch(...) { + BOOST_LOG_SEV(dcm_logger::get(), log_severity::error) << "Problem with updating symbol cache!!"; + sym = nullptr; } } return sym; diff --git a/src/kse-backend/tools/konaise_tool.cpp b/src/kse-backend/tools/konaise_tool.cpp index 58d39ea..8fde08a 100644 --- a/src/kse-backend/tools/konaise_tool.cpp +++ b/src/kse-backend/tools/konaise_tool.cpp @@ -28,7 +28,6 @@ static int kse_cert_get = 0x00; static int kse_cert_add = 0x01; -static int kse_cert_del = 0x02; static int kse_key_get = 0x10; static int kse_key_add = 0x11; static int kse_key_del = 0x12; @@ -271,45 +270,49 @@ void print_usage(char *prog_name) { int main(int argc, char ** argv) { - if(argc < 2) { - print_usage(argv[0]); - return -1; - } + try { + if(argc < 2) { + print_usage(argv[0]); + return -1; + } - SEKonai se(KSE_LIB_NAME); - if(!se.is_loaded()) { - std::cout << "LIBRARY WAS NOT LOADED!!" << std::endl; - return -1; - } - std::string cmd(argv[1]); + SEKonai se(KSE_LIB_NAME); + if(!se.is_loaded()) { + std::cout << "LIBRARY WAS NOT LOADED!!" << std::endl; + return -1; + } + std::string cmd(argv[1]); - if((cmd.compare("get_cert") == 0) && argc > 3) { - se.get_cert(std::stoi(argv[2]), std::string(argv[3])); - }else if((cmd.compare("add_cert") == 0) && argc > 3) { - se.add_cert(std::string(argv[2]), std::stoi(argv[3])); - }else if((cmd.compare("del_cert") == 0) && argc > 2) { - se.del_cert(std::stoi(argv[2])); - }else if((cmd.compare("get_key") == 0) && argc > 4) { - se.get_key(se.get_key_type(std::string(argv[2])), - std::stoi(argv[3]), std::string(argv[4])); - }else if((cmd.compare("add_key") == 0) && argc >= 5) { - char * priv_file = nullptr; - if(argc > 5) - priv_file = argv[5]; - se.add_key(std::string(argv[2]), se.get_key_type(std::string(argv[3])), - std::stoi(argv[4]), std::string(priv_file)); - }else if((cmd.compare("del_key") == 0) && argc > 3) { - se.del_key(se.get_key_type(std::string(argv[2])), std::stoi(argv[3])); - }else if((cmd.compare("ec_sec_p256r1_sign") == 0) && argc > 4) { - se.ec_sec_p256r1_sign(std::string(argv[2]), std::stoi(argv[3]), - std::string(argv[4])); - }else if((cmd.compare("ec_sec_p256r1_verify") == 0) && argc > 4) { - se.ec_sec_p256r1_verify(std::string(argv[2]), std::stoi(argv[3]), - std::string(argv[4])); - }else { - print_usage(argv[0]); + if((cmd.compare("get_cert") == 0) && argc > 3) { + se.get_cert(std::stoi(argv[2]), std::string(argv[3])); + }else if((cmd.compare("add_cert") == 0) && argc > 3) { + se.add_cert(std::string(argv[2]), std::stoi(argv[3])); + }else if((cmd.compare("del_cert") == 0) && argc > 2) { + se.del_cert(std::stoi(argv[2])); + }else if((cmd.compare("get_key") == 0) && argc > 4) { + se.get_key(se.get_key_type(std::string(argv[2])), + std::stoi(argv[3]), std::string(argv[4])); + }else if((cmd.compare("add_key") == 0) && argc >= 5) { + char * priv_file = nullptr; + if(argc > 5) + priv_file = argv[5]; + se.add_key(std::string(argv[2]), se.get_key_type(std::string(argv[3])), + std::stoi(argv[4]), std::string(priv_file)); + }else if((cmd.compare("del_key") == 0) && argc > 3) { + se.del_key(se.get_key_type(std::string(argv[2])), std::stoi(argv[3])); + }else if((cmd.compare("ec_sec_p256r1_sign") == 0) && argc > 4) { + se.ec_sec_p256r1_sign(std::string(argv[2]), std::stoi(argv[3]), + std::string(argv[4])); + }else if((cmd.compare("ec_sec_p256r1_verify") == 0) && argc > 4) { + se.ec_sec_p256r1_verify(std::string(argv[2]), std::stoi(argv[3]), + std::string(argv[4])); + }else { + print_usage(argv[0]); + return -1; + } + catch (...) { + std::cout << "EXCEPTION OCCURRED!!" << std::endl; return -1; } - return 0; } -- 2.7.4