]> git.ipfire.org Git - thirdparty/snort3.git/commitdiff
Merge pull request #2168 in SNORT/snort3 from ~DAVMCPHE/snort3:reload_memory_leaks...
authorMichael Altizer (mialtize) <mialtize@cisco.com>
Mon, 8 Jun 2020 16:33:58 +0000 (16:33 +0000)
committerMichael Altizer (mialtize) <mialtize@cisco.com>
Mon, 8 Jun 2020 16:33:58 +0000 (16:33 +0000)
Squashed commit of the following:

commit 8b865427b64ced3d8fa7b49db9206e13201ece4c
Author: davis mcpherson <davmcphe@cisco.com>
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

12 files changed:
src/actions/act_react.cc
src/log/messages.cc
src/main/snort_config.cc
src/main/snort_config.h
src/network_inspectors/appid/appid_inspector.cc
src/network_inspectors/appid/service_state.cc
src/network_inspectors/port_scan/port_scan.cc
src/parser/parse_rule.cc
src/stream/base/stream_base.cc
src/stream/base/stream_ha.cc
src/target_based/host_attributes.cc
tools/snort2lua/rule_states/rule_react.cc

index 63985f12de8b1cb799cf510a27a69a95c9608a10..74910cc045e9356c838248871055c2324d4282ce 100644 (file)
@@ -46,6 +46,9 @@
 
 #include <sys/stat.h>
 
+#include <fstream>
+#include <string>
+
 #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 \
     "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.1//EN\"\r\n" \
@@ -84,118 +83,83 @@ static THREAD_LOCAL ProfileStats reactPerfStats;
     "</head>\r\n" \
     "<body>\r\n" \
     "<h1>Access Denied</h1>\r\n" \
-    "<p>%s</p>\r\n" \
+    "<p>You are attempting to access a forbidden site.<br />" \
+    "Consult your system administrator for details.</p>\r\n" \
     "</body>\r\n" \
     "</html>\r\n"
 
-#define DEFAULT_MSG \
-    "You are attempting to access a forbidden site.<br />" \
-    "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<char>(ifs)), (std::istreambuf_iterator<char>()));
     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 =
 {
index 42e91a2d1b632134ed0bf3ffe9cb3f3ca26f4dab..b95104d4c3ad3579c9bec144c333453db242d11a 100644 (file)
@@ -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
index 25924d1329780b6821d0a28a7bcd6a219e2190e3..fa6e393e3408b933f8bad17797e4f45529865d69 100644 (file)
@@ -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();
+    }
+}
+
index 846bb0cb6894465567a8fe1b25f725af71b1bcb6..b10bed934e02589cd9d0390c3287e84da500b3b6 100644 (file)
@@ -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();
 };
 }
 
index 9aad875a759fd54e852719124d42b4ae5870e35c..a3b68c3c4cf9f10ea112dc81b3151cb3b4ff079a 100644 (file)
@@ -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)
index e5b83405df93a3e7b0b4acc166e729a2827cdbc6..e5947ee596b3934c34e062345bbab62abee5cafd 100644 (file)
@@ -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,
index 938d7aaebfc7e8fd9b238c16fdbc55e8896f2527..53764eab8fd0675fa4acf547a27e701c4f700883 100644 (file)
@@ -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
index 3c261380cf41537c1e2af2e60898115f136d2f8a..fa212ff47f43568216674b744aa02a6b584442cf 100644 (file)
@@ -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);
index 0270a2900bcd91a5ee154eb504a37b05d12355a0..fe914be03c5fa4dc80ad1dcf1c75b913a1a48c86 100644 (file)
@@ -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;
index 987ffd5855b1c594d2f465179228e10e6eac79a4..cf13463106065e87c783ea831043a02570b34366 100644 (file)
@@ -282,5 +282,8 @@ void StreamHAManager::tinit()
 void StreamHAManager::tterm()
 {
     if ( ha_client )
+    {
         delete ha_client;
+        ha_client = nullptr;
+    }
 }
index de14a3145a6681c5f6c3d7c4a629966a931ed4b5..855bc8578a692a884e725923da5b15aeca1975bb 100644 (file)
@@ -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 )
index b88c6671e7f5fd86d799d9a4a989023b99b97427..4fa669428da1bbe366a175166b7530d048ef81a6 100644 (file)
@@ -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);
                 }