From 7d9a73725ab0106abfb93dfb1ab0043d5703db49 Mon Sep 17 00:00:00 2001 From: "Hui Cao (huica)" Date: Wed, 30 Nov 2016 13:58:59 -0500 Subject: [PATCH] Merge pull request #729 in SNORT/snort3 from appid_x509_memleak to master Squashed commit of the following: commit 1d47856b2d2f0f69c3d53550e3a9cff236ffdbed Author: davis mcpherson Date: Mon Nov 28 15:37:18 2016 -0500 cleanup openssl resources when snort exits to eliminate memory leaks only call openssl cleanup functions required to clean crypto* api usage, move cleanup to plugin terminate api function, improve service ssl code style --- .../appid/appid_inspector.cc | 20 ++- .../appid/service_plugins/service_ssl.cc | 138 +++++++++--------- 2 files changed, 82 insertions(+), 76 deletions(-) diff --git a/src/network_inspectors/appid/appid_inspector.cc b/src/network_inspectors/appid/appid_inspector.cc index fe5631d87..5d0112812 100644 --- a/src/network_inspectors/appid/appid_inspector.cc +++ b/src/network_inspectors/appid/appid_inspector.cc @@ -25,6 +25,8 @@ #include "config.h" #endif +#include + #include "log/messages.h" #include "main/thread.h" #include "profiler/profiler.h" @@ -56,6 +58,13 @@ static void dump_appid_stats() AppIdServiceState::dump_stats(); } +// FIXIT-L - appid cleans up openssl now as it is the primary (only) user... eventually this +// should probably be done outside of appid +void openssl_cleanup() +{ + CRYPTO_cleanup_all_ex_data(); +} + AppIdInspector::AppIdInspector(const AppIdModuleConfig* pc) { assert(pc); @@ -168,11 +177,16 @@ static void mod_dtor(Module* m) delete m; } -static void appid_inspector_init() +static void appid_inspector_pinit() { AppIdSession::init(); } +static void appid_inspector_pterm() +{ + openssl_cleanup(); +} + static Inspector* appid_inspector_ctor(Module* m) { AppIdModule* mod = (AppIdModule*)m; @@ -202,8 +216,8 @@ const InspectApi appid_inspector_api = (uint16_t)PktType::ANY_IP, nullptr, // buffers nullptr, // service - appid_inspector_init, // pinit - nullptr, // pterm + appid_inspector_pinit, // pinit + appid_inspector_pterm, // pterm nullptr, // tinit nullptr, // tterm appid_inspector_ctor, diff --git a/src/network_inspectors/appid/service_plugins/service_ssl.cc b/src/network_inspectors/appid/service_plugins/service_ssl.cc index fea8b6073..b838f1098 100644 --- a/src/network_inspectors/appid/service_plugins/service_ssl.cc +++ b/src/network_inspectors/appid/service_plugins/service_ssl.cc @@ -470,16 +470,11 @@ static void parse_client_initiation(const uint8_t* data, uint16_t size, ServiceS } } -static int parse_certificates(ServiceSSLData* ss) +static bool parse_certificates(ServiceSSLData* ss) { - int success = 0; + bool success = false; if (ss->certs_data && ss->certs_len) { - char* common_name; - char* org_name; - char* common_name_ptr; - char* org_name_ptr; - /* Pull out certificates from block of data. */ uint8_t* data = ss->certs_data; int len = ss->certs_len; @@ -488,31 +483,25 @@ static int parse_certificates(ServiceSSLData* ss) int common_name_tot_len = 0; int org_name_tot_len = 0; int num_certs = 0; - success = 1; + success = true; + while (len > 0) { - X509* cert; - char* start; - char* end; - int length; - - /* Get each certificate. */ int cert_len = ntoh3(data); data += 3; len -= 3; if (len < cert_len) { - success = 0; + success = false; break; } crypto_lib_mutex.lock(); - cert = d2i_X509(nullptr, (const unsigned char**)&data, cert_len); + X509* cert = d2i_X509(nullptr, (const unsigned char**)&data, cert_len); crypto_lib_mutex.unlock(); - len -= cert_len; /* Above call increments data pointer already. */ if (!cert) { - success = 0; + success = false; break; } @@ -523,13 +512,14 @@ static int parse_certificates(ServiceSSLData* ss) certs_head = certs_curr; num_certs++; - /* Find "common name" value. */ - start = strstr(cert->name, COMMON_NAME_STR); + char* start = strstr(cert->name, COMMON_NAME_STR); if (start) { + int length; + start += strlen(COMMON_NAME_STR); certs_curr->common_name_ptr = (uint8_t*)start; - end = strstr(start, FIELD_SEPARATOR); + char* end = strstr(start, FIELD_SEPARATOR); if (end) length = end - start; else @@ -539,13 +529,14 @@ static int parse_certificates(ServiceSSLData* ss) common_name_tot_len += length; } - /* Find "org name" value. */ start = strstr(cert->name, ORG_NAME_STR); if (start) { + int length; + start += strlen(ORG_NAME_STR); certs_curr->org_name_ptr = (uint8_t*)start; - end = strstr(start, FIELD_SEPARATOR); + char* end = strstr(start, FIELD_SEPARATOR); if (end) length = end - start; else @@ -555,66 +546,66 @@ static int parse_certificates(ServiceSSLData* ss) org_name_tot_len += length; } } - if (!success) - goto parse_certificates_clean; - // Build up concatenated string of fields. - common_name = nullptr; - org_name = nullptr; - if (common_name_tot_len) + if ( success ) { - common_name_tot_len += num_certs; /* Space between each and terminator at end. */ - common_name = (char*)snort_calloc(common_name_tot_len); - } - - if (org_name_tot_len) - { - org_name_tot_len += num_certs; /* Space between each and terminator at end. */ - org_name = (char*)snort_calloc(org_name_tot_len); - } - - common_name_ptr = common_name; - org_name_ptr = org_name; - certs_curr = certs_head; - while (certs_curr) - { - /* Grab this common name. */ - if (common_name_ptr && certs_curr->common_name_ptr && certs_curr->common_name_len) + char* common_name = nullptr; + if (common_name_tot_len) { - memcpy(common_name_ptr, certs_curr->common_name_ptr, certs_curr->common_name_len); - common_name_ptr += certs_curr->common_name_len; - *common_name_ptr = ' '; - common_name_ptr += 1; + common_name_tot_len += num_certs; /* Space between each and terminator at end. */ + common_name = (char*)snort_calloc(common_name_tot_len); } - /* Grab this org name. */ - if (org_name_ptr && certs_curr->org_name_ptr && certs_curr->org_name_len) + char* org_name = nullptr; + if (org_name_tot_len) { - memcpy(org_name_ptr, certs_curr->org_name_ptr, certs_curr->org_name_len); - org_name_ptr += certs_curr->org_name_len; - *org_name_ptr = ' '; - org_name_ptr += 1; + org_name_tot_len += num_certs; /* Space between each and terminator at end. */ + org_name = (char*)snort_calloc(org_name_tot_len); } - certs_curr = certs_curr->next; - } + char* common_name_ptr = common_name; + char* org_name_ptr = org_name; + certs_curr = certs_head; + while (certs_curr) + { + /* Grab this common name. */ + if (common_name_ptr && certs_curr->common_name_ptr && certs_curr->common_name_len) + { + memcpy(common_name_ptr, certs_curr->common_name_ptr, certs_curr->common_name_len); + common_name_ptr += certs_curr->common_name_len; + *common_name_ptr = ' '; + common_name_ptr += 1; + } - if (common_name_tot_len) - { - common_name_ptr -= 1; - *common_name_ptr = '\0'; /* Put terminator at end rather than space. */ - } - if (org_name_tot_len) - { - org_name_ptr -= 1; - *org_name_ptr = '\0'; /* Put terminator at end rather than space. */ + /* Grab this org name. */ + if (org_name_ptr && certs_curr->org_name_ptr && certs_curr->org_name_len) + { + memcpy(org_name_ptr, certs_curr->org_name_ptr, certs_curr->org_name_len); + org_name_ptr += certs_curr->org_name_len; + *org_name_ptr = ' '; + org_name_ptr += 1; + } + + certs_curr = certs_curr->next; + } + + if (common_name_tot_len) + { + common_name_ptr -= 1; + *common_name_ptr = '\0'; + } + if (org_name_tot_len) + { + org_name_ptr -= 1; + *org_name_ptr = '\0'; + } + ss->common_name = common_name; + ss->common_name_strlen = common_name_tot_len - 1; /* Minus terminator. */ + ss->org_name = org_name; + ss->org_name_strlen = org_name_tot_len - 1; /* Minus terminator. */ } - ss->common_name = common_name; - ss->common_name_strlen = common_name_tot_len - 1; /* Minus terminator. */ - ss->org_name = org_name; - ss->org_name_strlen = org_name_tot_len - 1; /* Minus terminator. */ -parse_certificates_clean: + crypto_lib_mutex.lock(); while (certs_head) { certs_curr = certs_head; @@ -630,7 +621,8 @@ parse_certificates_clean: ss->certs_data = nullptr; ss->certs_len = 0; } - return success; /* 1 is OK; 0 is fail. */ + + return success; } static int ssl_validate(ServiceValidationArgs* args) -- 2.47.2