Merge pull request #738 in SNORT/snort3 from curse_uaf to master

Squashed commit of the following:

commit 11760bf923bbbe087b21330f6319d279908c8a6f
Author: Michael Altizer <mialtize@cisco.com>
Date:   Mon Dec 5 19:24:51 2016 -0500

    wizard: Refactor curses to prevent use-after-free of service name

diff --git a/src/service_inspectors/wizard/curses.cc b/src/service_inspectors/wizard/curses.cc
index 5fa7dc1549c59f9e36872ade8e3ca40b148ffcaa..1ed924d2454a098525230cac8c73b743e0a49485 100644 (file)
--- a/src/service_inspectors/wizard/curses.cc
+++ b/src/service_inspectors/wizard/curses.cc
@@ -19,10 +19,6 @@
 
 #include "curses.h"
 
-#include <ctype.h>
-#include "flow/flow.h"
-#include "protocols/packet.h"
-
 using namespace std;
 
 enum DceRpcPduType
@@ -251,11 +247,34 @@ static bool dce_smb_curse(const uint8_t* data, unsigned len, CurseTracker* track
 }
 
 // map between service and curse details
-map<string, CurseDetails> curse_map
+static vector<CurseDetails> curse_map
 {
     // service_name    alg        is_tcp
-    { "dce_udp", { dce_udp_curse, false }},
-    { "dce_tcp", { dce_tcp_curse, true }},
-    { "dce_smb", { dce_smb_curse, true }},
+    { "dce_udp", dce_udp_curse, false },
+    { "dce_tcp", dce_tcp_curse, true },
+    { "dce_smb", dce_smb_curse, true },
 };
 
+bool CurseBook::add_curse(const char* key)
+{
+    for (const CurseDetails& curse : curse_map)
+    {
+        if (curse.service == key)
+        {
+            if (curse.is_tcp)
+                tcp_curses.push_back(&curse);
+            else
+                non_tcp_curses.push_back(&curse);
+            return true;
+        }
+    }
+    return false;
+}
+
+const vector<const CurseDetails*>& CurseBook::get_curses(bool tcp) const
+{
+    if (tcp)
+        return tcp_curses;
+    return non_tcp_curses;
+}
+

diff --git a/src/service_inspectors/wizard/curses.h b/src/service_inspectors/wizard/curses.h
index cbfe7264ee763e589e2c9d63784ef6df3c833129..a7a6046df0f25f8311c098d1fb17f084ac1df9ac 100644 (file)
--- a/src/service_inspectors/wizard/curses.h
+++ b/src/service_inspectors/wizard/curses.h
@@ -21,9 +21,9 @@
 #define CURSES_H
 
 #include <ctype.h>
-#include <map>
-#include "flow/flow.h"
-#include "protocols/packet.h"
+
+#include <string>
+#include <vector>
 
 enum DCE_States
 {
@@ -53,12 +53,21 @@ typedef bool (* curse_alg)(const uint8_t* data, unsigned len, CurseTracker*);
 
 struct CurseDetails
 {
+    std::string service;
     curse_alg alg;
     bool is_tcp;
 };
 
-// map between service and curse details
-extern std::map<std::string, CurseDetails > curse_map;
+class CurseBook
+{
+public:
+    bool add_curse(const char* service);
+    const std::vector<const CurseDetails*>& get_curses(bool tcp) const;
+
+private:
+    std::vector<const CurseDetails*> tcp_curses;
+    std::vector<const CurseDetails*> non_tcp_curses;
+};
 
 #endif
 

diff --git a/src/service_inspectors/wizard/wiz_module.cc b/src/service_inspectors/wizard/wiz_module.cc
index 1cde6e921f853e3b74cf689ad37a27fe789b0090..7b2037a4d1f843707e3f27a4693f1b30dabf508c 100644 (file)
--- a/src/service_inspectors/wizard/wiz_module.cc
+++ b/src/service_inspectors/wizard/wiz_module.cc
@@ -110,6 +110,7 @@ WizardModule::WizardModule() : Module(WIZ_NAME, WIZ_HELP, s_params)
     s2c_hexes = nullptr;
     c2s_spells = nullptr;
     s2c_spells = nullptr;
+    curses = nullptr;
 }
 
 WizardModule::~WizardModule()
@@ -119,6 +120,8 @@ WizardModule::~WizardModule()
 
     delete c2s_spells;
     delete s2c_spells;
+
+    delete curses;
 }
 
 ProfileStats* WizardModule::get_profile() const
@@ -143,7 +146,7 @@ bool WizardModule::set(const char*, Value& v, SnortConfig*)
         spells.push_back(v.get_string());
 
     else if ( v.is("curses") )
-        curses.push_back(v.get_string());
+        curses->add_curse(v.get_string());
 
     else
         return false;
@@ -160,7 +163,8 @@ bool WizardModule::begin(const char* fqn, int, SnortConfig*)
 
         c2s_spells = new SpellBook;
         s2c_spells = new SpellBook;
-        curses.clear();
+
+        curses = new CurseBook;
     }
     else if ( !strcmp(fqn, "wizard.hexes") )
         hex = true;
@@ -209,8 +213,8 @@ bool WizardModule::end(const char*, int idx, SnortConfig*)
             add_spells(s2c_spells, service);
     }
 
-    spells.clear();
     service.clear();
+    spells.clear();
 
     return true;
 }
@@ -247,6 +251,13 @@ MagicBook* WizardModule::get_book(bool c2s, bool hex)
     return b;
 }
 
+CurseBook* WizardModule::get_curse_book()
+{
+    CurseBook* b = curses;
+    curses = nullptr;
+    return b;
+}
+
 const PegInfo* WizardModule::get_pegs() const
 { return wiz_pegs; }
 

diff --git a/src/service_inspectors/wizard/wiz_module.h b/src/service_inspectors/wizard/wiz_module.h
index 8af4a7761fec65594bab72fe466a758cee951e21..b2515249de721f86842a6407f0ae3ee83ee673a1 100644 (file)
--- a/src/service_inspectors/wizard/wiz_module.h
+++ b/src/service_inspectors/wizard/wiz_module.h
@@ -37,6 +37,7 @@ extern THREAD_LOCAL struct WizStats tstats;
 extern THREAD_LOCAL ProfileStats wizPerfStats;
 
 class MagicBook;
+class CurseBook;
 
 class WizardModule : public Module
 {
@@ -53,10 +54,7 @@ public:
     ProfileStats* get_profile() const override;
 
     MagicBook* get_book(bool c2s, bool hex);
-    std::vector<std::string> get_curse_book()
-    {
-        return curses;
-    }
+    CurseBook* get_curse_book();
 
 private:
     void add_spells(MagicBook*, std::string&);
@@ -64,16 +62,16 @@ private:
 private:
     bool hex;
     bool c2s;
-
     std::string service;
     std::vector<std::string> spells;
-    std::vector<std::string> curses;
 
     MagicBook* c2s_hexes;
     MagicBook* s2c_hexes;
 
     MagicBook* c2s_spells;
     MagicBook* s2c_spells;
+
+    CurseBook* curses;
 };
 
 #endif

diff --git a/src/service_inspectors/wizard/wizard.cc b/src/service_inspectors/wizard/wizard.cc
index 5b641997911a6f4f183ec5d70d9b5202bf27900a..171db2e2a6bd2d7566e593c0a899fbc4dacd334a 100644 (file)
--- a/src/service_inspectors/wizard/wizard.cc
+++ b/src/service_inspectors/wizard/wizard.cc
@@ -66,7 +66,7 @@ THREAD_LOCAL WizStats tstats;
 
 struct CurseServiceTracker
 {
-    string service;
+    const CurseDetails* curse;
     CurseTracker* tracker;
 };
 
@@ -111,7 +111,7 @@ public:
     void reset(Wand&, bool tcp, bool c2s);
     bool cast_spell(Wand&, Flow*, const uint8_t*, unsigned);
     bool spellbind(const MagicPage*&, Flow*, const uint8_t*, unsigned);
-    bool cursebind(vector<CurseServiceTracker>&,Flow*, const uint8_t*, unsigned);
+    bool cursebind(vector<CurseServiceTracker>&, Flow*, const uint8_t*, unsigned);
 
 public:
     MagicBook* c2s_hexes;
@@ -119,7 +119,8 @@ public:
 
     MagicBook* c2s_spells;
     MagicBook* s2c_spells;
-    vector<string> curse_book;
+
+    CurseBook* curses;
 };
 
 //-------------------------------------------------------------------------
@@ -169,7 +170,8 @@ Wizard::Wizard(WizardModule* m)
 
     c2s_spells = m->get_book(true, false);
     s2c_spells = m->get_book(false, false);
-    curse_book = m->get_curse_book();
+
+    curses = m->get_curse_book();
 }
 
 Wizard::~Wizard()
@@ -179,6 +181,8 @@ Wizard::~Wizard()
 
     delete c2s_spells;
     delete s2c_spells;
+
+    delete curses;
 }
 
 void Wizard::reset(Wand& w, bool tcp, bool c2s)
@@ -196,15 +200,13 @@ void Wizard::reset(Wand& w, bool tcp, bool c2s)
 
     if (w.curse_tracker.empty())
     {
-        for ( auto service:curse_book )
+        vector<const CurseDetails*> pages = curses->get_curses(tcp);
+        for ( const CurseDetails* curse : pages )
         {
-            if (tcp == curse_map[service].is_tcp)
-            {
-                if (tcp)
-                    w.curse_tracker.push_back({ service, new CurseTracker });
-                else
-                    w.curse_tracker.push_back({ service, nullptr });
-            }
+            if (tcp)
+                w.curse_tracker.push_back({ curse, new CurseTracker });
+            else
+                w.curse_tracker.push_back({ curse, nullptr });
         }
     }
 }
@@ -247,6 +249,24 @@ bool Wizard::spellbind(
     return false;
 }
 
+bool Wizard::cursebind(vector<CurseServiceTracker>& curse_tracker, Flow* f,
+        const uint8_t* data, unsigned len)
+{
+    for (const CurseServiceTracker& cst : curse_tracker)
+    {
+        if (cst.curse->alg(data, len, cst.tracker))
+        {
+            f->service = cst.curse->service.c_str();
+            // FIXIT-H need to make sure Flow's ipproto and service
+            // correspond to HostApplicationEntry's ipproto and service
+            host_cache_add_service(f->server_ip, f->ip_proto, f->server_port, f->service);
+            return true;
+        }
+    }
+
+    return false;
+}
+
 bool Wizard::cast_spell(
     Wand& w, Flow* f, const uint8_t* data, unsigned len)
 {
@@ -262,31 +282,6 @@ bool Wizard::cast_spell(
     return false;
 }
 
-bool Wizard::cursebind(
-    vector<CurseServiceTracker>& curse_tracker, Flow* f, const uint8_t* data, unsigned len)
-{
-    bool match = false;
-
-    for (auto const& p : curse_tracker)
-    {
-        if (curse_map[p.service].alg(data,len, p.tracker))
-        {
-            match = true;
-            f->service = p.service.c_str();
-            break;
-        }
-    }
-
-    if (match)
-    {
-        // FIXIT-H need to make sure Flow's ipproto and service
-        // correspond to HostApplicationEntry's ipproto and service
-        host_cache_add_service(f->server_ip, f->ip_proto, f->server_port, f->service);
-    }
-
-    return match;
-}
-
 //-------------------------------------------------------------------------
 // api stuff
 //-------------------------------------------------------------------------