From: Michael Altizer (mialtize) Date: Tue, 6 Dec 2016 03:28:18 +0000 (-0500) Subject: Merge pull request #738 in SNORT/snort3 from curse_uaf to master X-Git-Tag: 3.0.0-233~160 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ae39c2822c46d5e3d6c032fccfff63fe4ebc30f;p=thirdparty%2Fsnort3.git Merge pull request #738 in SNORT/snort3 from curse_uaf to master Squashed commit of the following: commit 11760bf923bbbe087b21330f6319d279908c8a6f Author: Michael Altizer 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 5fa7dc154..1ed924d24 100644 --- a/src/service_inspectors/wizard/curses.cc +++ b/src/service_inspectors/wizard/curses.cc @@ -19,10 +19,6 @@ #include "curses.h" -#include -#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 curse_map +static vector 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& 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 cbfe7264e..a7a6046df 100644 --- a/src/service_inspectors/wizard/curses.h +++ b/src/service_inspectors/wizard/curses.h @@ -21,9 +21,9 @@ #define CURSES_H #include -#include -#include "flow/flow.h" -#include "protocols/packet.h" + +#include +#include 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 curse_map; +class CurseBook +{ +public: + bool add_curse(const char* service); + const std::vector& get_curses(bool tcp) const; + +private: + std::vector tcp_curses; + std::vector non_tcp_curses; +}; #endif diff --git a/src/service_inspectors/wizard/wiz_module.cc b/src/service_inspectors/wizard/wiz_module.cc index 1cde6e921..7b2037a4d 100644 --- 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 8af4a7761..b2515249d 100644 --- 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 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 spells; - std::vector 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 5b6419979..171db2e2a 100644 --- 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&,Flow*, const uint8_t*, unsigned); + bool cursebind(vector&, Flow*, const uint8_t*, unsigned); public: MagicBook* c2s_hexes; @@ -119,7 +119,8 @@ public: MagicBook* c2s_spells; MagicBook* s2c_spells; - vector 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 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& 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& 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 //-------------------------------------------------------------------------