From: Michael Altizer (mialtize) Date: Mon, 8 Jun 2020 16:33:58 +0000 (+0000) Subject: Merge pull request #2168 in SNORT/snort3 from ~DAVMCPHE/snort3:reload_memory_leaks... X-Git-Tag: 3.0.1-5~20 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3aa1c15d550f67f481522b09a9cf31db8307a152;p=thirdparty%2Fsnort3.git Merge pull request #2168 in SNORT/snort3 from ~DAVMCPHE/snort3:reload_memory_leaks to master Squashed commit of the following: commit 8b865427b64ced3d8fa7b49db9206e13201ece4c Author: davis mcpherson Date: Thu Apr 16 13:14:56 2020 -0400 port_scan: cleanup port scan memory allocations in module tterm parser: free memory allocated for RTN when SO rule load fails stream: add final check to free allocated memory when module tterm is called actions: on a reload_config() free the memory allocated for react page on previous configuration loading shell: if initial load of snort configuration fails release memory allocated for modules and plugins appid: free memory allocated when appid is configured initially and then not configured on a subsequent reload snort_config: only perform FatalError cleanup from main thread actions: refactor to store react page response in std::string snort2lua: deprecate react::msg option, display of rule message in react page not currently supported --- diff --git a/src/actions/act_react.cc b/src/actions/act_react.cc index 63985f12d..74910cc04 100644 --- a/src/actions/act_react.cc +++ b/src/actions/act_react.cc @@ -46,6 +46,9 @@ #include +#include +#include + #include "framework/ips_action.h" #include "framework/module.h" #include "log/messages.h" @@ -64,15 +67,11 @@ using namespace snort; static THREAD_LOCAL ProfileStats reactPerfStats; -#define MSG_KEY "<>" -#define MSG_PERCENT "%" - #define DEFAULT_HTTP \ "HTTP/1.1 403 Forbidden\r\n" \ "Connection: close\r\n" \ "Content-Type: text/html; charset=utf-8\r\n" \ - "Content-Length: %d\r\n" \ - "\r\n" + "Content-Length: " #define DEFAULT_HTML \ "\r\n" \ "\r\n" \ "

Access Denied

\r\n" \ - "

\r\n" \ + "

You are attempting to access a forbidden site.
" \ + "Consult your system administrator for details.

\r\n" \ "\r\n" \ "\r\n" -#define DEFAULT_MSG \ - "You are attempting to access a forbidden site.
" \ - "Consult your system administrator for details." - - class ReactData { public: - ReactData(bool rmsg, const char* page); - ~ReactData(); - - ssize_t get_buf_len() const { return buf_len; } - const char* get_resp_buf() const { return resp_buf; } - -private: - // FIXIT-M: make it do what it says, or delete it. - // int rule_msg; // 1=>use rule msg; 0=>use DEFAULT_MSG - ssize_t buf_len; // length of response - char* resp_buf; // response to send - const char* resp_page; -}; - + ReactData(const std::string& page) + { + if ( page.empty()) + { + resp_buf = DEFAULT_HTTP + std::to_string(sizeof(DEFAULT_HTML)); + resp_buf.append("\r\n\r\n"); + resp_buf.append(DEFAULT_HTML); + } + else + { + resp_buf = DEFAULT_HTTP + std::to_string(page.size()); + resp_buf.append("\r\n\r\n"); + resp_buf.append(page); + } + } -class ReactAction : public snort::IpsAction -{ -public: - ReactAction(ReactData* c); - ~ReactAction() override; + ~ReactData() = default; - void exec(snort::Packet*) override; + size_t get_buf_len() const + { return resp_buf.size(); } -private: - void send(snort::Packet*); + const char* get_resp_buf() const + { return resp_buf.c_str(); } private: - ReactData* config; + std::string resp_buf; // response to send }; -ReactData::ReactData(bool rmsg, const char* page) - : buf_len(0), resp_buf(nullptr), resp_page(page) -{ - int body_len, head_len, total_len; - char dummy; - - const char* head = DEFAULT_HTTP; - const char* body = resp_page ? resp_page : DEFAULT_HTML; - const char* msg = DEFAULT_MSG; - UNUSED(rmsg); - - body_len = snprintf(&dummy, 1, body, msg); - head_len = snprintf(&dummy, 1, head, body_len); - total_len = head_len + body_len + 1; - - resp_buf = (char*)snort_calloc(total_len); - SnortSnprintf((char*)resp_buf, head_len+1, head, body_len); - SnortSnprintf((char*)resp_buf+head_len, body_len+1, body, msg); - - // set actual length - resp_buf[total_len-1] = '\0'; - buf_len = strlen(resp_buf); -} - -ReactData::~ReactData() +class ReactAction : public snort::IpsAction { - if ( resp_buf ) - snort_free(resp_buf); -} +public: + ReactAction(ReactData* c) + : IpsAction(s_name, ActionType::ACT_PROXY), config(c) + { } -//------------------------------------------------------------------------- -// class methods -//------------------------------------------------------------------------- + ~ReactAction() override + { delete config; } -ReactAction::ReactAction(ReactData* c) : IpsAction(s_name, ActionType::ACT_PROXY), config(c) {} + void exec(snort::Packet* p) override + { + Profile profile(reactPerfStats); -ReactAction::~ReactAction() -{ - delete config; -} + if ( p->active->is_reset_candidate(p) ) + send(p); + } -void ReactAction::exec(Packet* p) -{ - Profile profile(reactPerfStats); +private: + void send(snort::Packet* p) + { + EncodeFlags df = (p->is_from_server()) ? ENC_FLAG_FWD : 0; + EncodeFlags sent = 0; - if ( p->active->is_reset_candidate(p) ) - send(p); -} + Active* act = p->active; -void ReactAction::send(Packet* p) -{ - EncodeFlags df = (p->is_from_server()) ? ENC_FLAG_FWD : 0; - EncodeFlags sent = 0; + if ( p->packet_flags & PKT_STREAM_EST ) + sent = act->send_data(p, df, (const uint8_t*)config->get_resp_buf(), config->get_buf_len()); - Active* act = p->active; + EncodeFlags rf = ENC_FLAG_SEQ | (ENC_FLAG_VAL & sent); + act->send_reset(p, rf); - if ( p->packet_flags & PKT_STREAM_EST ) - { - sent = act->send_data(p, df, (const uint8_t*)config->get_resp_buf(), config->get_buf_len()); + // block the flow in case the RST is lost. + act->block_session(p); } - EncodeFlags rf = ENC_FLAG_SEQ | (ENC_FLAG_VAL & sent); - act->send_reset(p, rf); - - // Blacklist the flow so that no additional data goes through in case - // the RST is lost. - act->block_session(p); -} +private: + ReactData* config; +}; //------------------------------------------------------------------------- // module @@ -203,9 +167,6 @@ void ReactAction::send(Packet* p) static const Parameter s_params[] = { - { "msg", Parameter::PT_BOOL, nullptr, "false", - " use rule msg in response page instead of default message" }, - { "page", Parameter::PT_STRING, nullptr, nullptr, "file containing HTTP response (headers and body)" }, @@ -215,11 +176,8 @@ static const Parameter s_params[] = class ReactModule : public Module { public: - ReactModule() : Module(s_name, s_help, s_params), msg(false), page(nullptr) {} - ~ReactModule() override { - if (page) - snort_free(page); - } + ReactModule() : Module(s_name, s_help, s_params) + { } bool begin(const char*, int, SnortConfig*) override; bool set(const char*, Value&, SnortConfig*) override; @@ -231,81 +189,35 @@ public: { return DETECT; } public: - bool msg; - char* page; + std::string page; + private: bool getpage(const char* file); }; bool ReactModule::getpage(const char* file) { - char* msg; - char* percent_s; - struct stat fs; - FILE* fd; - size_t n; - - if ( stat(file, &fs) ) - { - ParseError("can't stat react page file '%s'.", file); - return false; - } - - page = (char*)snort_calloc(fs.st_size+1); - fd = fopen(file, "r"); - - if ( !fd ) + std::ifstream ifs(file); + if ( !ifs.good() ) { - ParseError("can't open react page file '%s'.", file); + ParseError("Failed to open custom react page file: %s.", file); return false; } - n = fread(page, 1, fs.st_size, fd); - fclose(fd); - - if ( n != (size_t)fs.st_size ) - { - ParseError("can't load react page file '%s'.", file); - return false; - } - - page[n] = '\0'; - msg = strstr(page, MSG_KEY); - if ( msg ) - strncpy(msg, "%s", 3); - - // search for % - percent_s = strstr(page, MSG_PERCENT); - if (percent_s) - { - percent_s += strlen(MSG_PERCENT); // move past current - // search for % again - percent_s = strstr(percent_s, MSG_PERCENT); - if (percent_s) - { - ParseError("can't specify more than one %%s or other " - "printf style formatting characters in react page '%s'.", - file); - return false; - } - } + page.assign((std::istreambuf_iterator(ifs)), (std::istreambuf_iterator())); return true; } bool ReactModule::begin(const char*, int, SnortConfig*) { - msg = false; + page.clear(); return true; } bool ReactModule::set(const char*, Value& v, SnortConfig*) { - if ( v.is("msg") ) - msg = v.get_bool(); - - else if ( v.is("page") ) + if ( v.is("page") ) return getpage(v.get_string()); - else return false; @@ -317,28 +229,22 @@ bool ReactModule::set(const char*, Value& v, SnortConfig*) //------------------------------------------------------------------------- static Module* mod_ctor() -{ - return new ReactModule; -} +{ return new ReactModule; } static void mod_dtor(Module* m) -{ - delete m; -} +{ delete m; } static IpsAction* react_ctor(Module* p) { ReactModule* m = (ReactModule*)p; - ReactData* rd = new ReactData(m->msg, m->page); + ReactData* rd = new ReactData(m->page); Active::set_enabled(); return new ReactAction(rd); } static void react_dtor(IpsAction* p) -{ - delete p; -} +{ delete p; } static const ActionApi react_api = { diff --git a/src/log/messages.cc b/src/log/messages.cc index 42e91a2d1..b95104d4c 100644 --- a/src/log/messages.cc +++ b/src/log/messages.cc @@ -336,6 +336,8 @@ void ErrorMessage(const char* format,...) fprintf(stderr,"Fatal Error, Quitting..\n"); } + SnortConfig::cleanup_fatal_error(); + #if 0 // FIXIT-M need to stop analyzers / workers // and they should handle the DAQ break / abort diff --git a/src/main/snort_config.cc b/src/main/snort_config.cc index 25924d132..fa6e393e3 100644 --- a/src/main/snort_config.cc +++ b/src/main/snort_config.cc @@ -981,3 +981,21 @@ void SnortConfig::set_conf(const SnortConfig* sc) set_policies(sc, sh); } } + +void SnortConfig::cleanup_fatal_error() +{ + // guard against FatalError calls from packet threads (which should not happen!) + // FIXIT-L should also guard against non-main non-packet threads calling FatalError + if ( is_packet_thread() ) + return; + + const SnortConfig* sc = SnortConfig::get_conf(); + if ( sc && !sc->dirty_pig ) + { + ModuleManager::term(); + EventManager::release_plugins(); + IpsManager::release_plugins(); + InspectorManager::release_plugins(); + } +} + diff --git a/src/main/snort_config.h b/src/main/snort_config.h index 846bb0cb6..b10bed934 100644 --- a/src/main/snort_config.h +++ b/src/main/snort_config.h @@ -703,11 +703,13 @@ public: // runtime access to mutable config - main thread only, and only special cases SO_PUBLIC static SnortConfig* get_main_conf(); + + static void set_conf(const SnortConfig*); SO_PUBLIC void register_reload_resource_tuner(ReloadResourceTuner& rrt) { reload_tuners.push_back(&rrt); } - static void set_conf(const SnortConfig*); + static void cleanup_fatal_error(); }; } diff --git a/src/network_inspectors/appid/appid_inspector.cc b/src/network_inspectors/appid/appid_inspector.cc index 9aad875a7..a3b68c3c4 100644 --- a/src/network_inspectors/appid/appid_inspector.cc +++ b/src/network_inspectors/appid/appid_inspector.cc @@ -140,10 +140,8 @@ void AppIdInspector::tinit() appid_mute = PacketTracer::get_mute(); AppIdStatistics::initialize_manager(*config); - appid_forecast_tinit(); LuaDetectorManager::initialize(*ctxt); AppIdServiceState::initialize(config->memcap); - appidDebug = new AppIdDebug(); assert(!tp_appid_thread_ctxt); tp_appid_thread_ctxt = ctxt->get_tp_appid_ctxt(); if (tp_appid_thread_ctxt) @@ -154,13 +152,8 @@ void AppIdInspector::tinit() void AppIdInspector::tterm() { - appid_forecast_tterm(); AppIdStatistics::cleanup(); - LuaDetectorManager::terminate(); AppIdDiscovery::tterm(); - AppIdServiceState::clean(); - delete appidDebug; - appidDebug = nullptr; ThirdPartyAppIdContext* tp_appid_ctxt = ctxt->get_tp_appid_ctxt(); if (tp_appid_ctxt) tp_appid_ctxt->tfini(); @@ -216,12 +209,18 @@ static void appid_inspector_pterm() static void appid_inspector_tinit() { AppIdPegCounts::init_pegs(); + appid_forecast_tinit(); + appidDebug = new AppIdDebug(); } static void appid_inspector_tterm() { TPLibHandler::tfini(); AppIdPegCounts::cleanup_pegs(); + LuaDetectorManager::terminate(); + AppIdServiceState::clean(); + appid_forecast_tterm(); + delete appidDebug; } static Inspector* appid_inspector_ctor(Module* m) diff --git a/src/network_inspectors/appid/service_state.cc b/src/network_inspectors/appid/service_state.cc index e5b83405d..e5947ee59 100644 --- a/src/network_inspectors/appid/service_state.cc +++ b/src/network_inspectors/appid/service_state.cc @@ -209,6 +209,7 @@ bool AppIdServiceState::initialize(size_t memcap) void AppIdServiceState::clean() { delete service_state_cache; + service_state_cache = nullptr; } ServiceDiscoveryState* AppIdServiceState::add(const SfIp* ip, IpProtocol proto, uint16_t port, diff --git a/src/network_inspectors/port_scan/port_scan.cc b/src/network_inspectors/port_scan/port_scan.cc index 938d7aaeb..53764eab8 100644 --- a/src/network_inspectors/port_scan/port_scan.cc +++ b/src/network_inspectors/port_scan/port_scan.cc @@ -411,9 +411,7 @@ static void portscan_config_show(const PortscanConfig* config) //------------------------------------------------------------------------- PortScan::PortScan(PortScanModule* mod) -{ - config = mod->get_data(); -} +{ config = mod->get_data(); } PortScan::~PortScan() { @@ -422,14 +420,10 @@ PortScan::~PortScan() } void PortScan::tinit() -{ - ps_init_hash(config->memcap); -} +{ ps_init_hash(config->memcap); } void PortScan::tterm() -{ - ps_cleanup(); -} +{ ps_cleanup(); } void PortScan::show(const SnortConfig*) const { @@ -467,6 +461,12 @@ void PortScan::eval(Packet* p) // api stuff //------------------------------------------------------------------------- +static void port_scan_tinit() +{ } + +static void port_scan_tterm() +{ ps_cleanup(); } + static Module* mod_ctor() { return new PortScanModule; } @@ -502,8 +502,8 @@ static const InspectApi sp_api = nullptr, // service nullptr, // pinit nullptr, // pterm - nullptr, // tinit - nullptr, // tterm + port_scan_tinit, // tinit + port_scan_tterm, // tterm sp_ctor, sp_dtor, nullptr, // ssn diff --git a/src/parser/parse_rule.cc b/src/parser/parse_rule.cc index 3c261380c..fa212ff47 100644 --- a/src/parser/parse_rule.cc +++ b/src/parser/parse_rule.cc @@ -1231,7 +1231,10 @@ void parse_rule_close(SnortConfig* sc, RuleTreeNode& rtn, OptTreeNode* otn) IpsManager::reset_options(); if ( !rule ) + { ParseError("SO rule %s not loaded.", otn->soid); + FreeRuleTreeNode(&rtn); + } else { SoRule so_rule(&rtn, otn); diff --git a/src/stream/base/stream_base.cc b/src/stream/base/stream_base.cc index 0270a2900..fe914be03 100644 --- a/src/stream/base/stream_base.cc +++ b/src/stream/base/stream_base.cc @@ -314,6 +314,9 @@ static void base_tinit() static void base_tterm() { + StreamHAManager::tterm(); + FlushBucket::clear(); + // this can't happen sooner because the counts haven't been harvested yet delete flow_con; flow_con = nullptr; diff --git a/src/stream/base/stream_ha.cc b/src/stream/base/stream_ha.cc index 987ffd585..cf1346310 100644 --- a/src/stream/base/stream_ha.cc +++ b/src/stream/base/stream_ha.cc @@ -282,5 +282,8 @@ void StreamHAManager::tinit() void StreamHAManager::tterm() { if ( ha_client ) + { delete ha_client; + ha_client = nullptr; + } } diff --git a/src/target_based/host_attributes.cc b/src/target_based/host_attributes.cc index de14a3145..855bc8578 100644 --- a/src/target_based/host_attributes.cc +++ b/src/target_based/host_attributes.cc @@ -142,6 +142,7 @@ void HostAttributeEntry::update_service ApplicationEntry* app = new ApplicationEntry(port, protocol, snort_protocol_id); host->add_service(app); } + SnortProtocolId HostAttributeEntry::get_snort_protocol_id(int ipprotocol, uint16_t port) const { for ( auto app : services ) diff --git a/tools/snort2lua/rule_states/rule_react.cc b/tools/snort2lua/rule_states/rule_react.cc index b88c6671e..4fa669428 100644 --- a/tools/snort2lua/rule_states/rule_react.cc +++ b/tools/snort2lua/rule_states/rule_react.cc @@ -74,20 +74,17 @@ bool React::convert(std::istringstream& data_stream) do { if (tmp == "warn") - table_api.add_deleted_comment("warn"); + rule_api.add_comment("react: warn - deprecated"); else if (tmp == "block") - table_api.add_deleted_comment("block"); + rule_api.add_comment("react: block - deprecated"); else if (!tmp.compare(0, 5, "proxy")) - table_api.add_deleted_comment(tmp); + rule_api.add_comment("react: proxy - deprecated"); else if (tmp == "msg") - { - table_api.add_diff_option_comment( - "msg", "react.msg = true"); - table_api.add_option("msg", true); - } + rule_api.add_comment("react: msg - deprecated"); + else rule_api.bad_rule(data_stream, "resp: " + tmp); }