From a1c5eddb95afe9a746bb938e55c204eeccbf73e1 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 9 Aug 2019 19:51:08 -0700 Subject: [PATCH] GH-532: improve disable_analyzer BIF - Add an extra "prevent" parameter (default value of false), which helps prevent the same analyzer type from being attached in the future. It's useful in situations where you want to disable early on, but a DPD signature may still trigger later and re-attach the same analyzer. E.g. when not using this flag, but calling disable_analyzer() inside an http_request event, will remove the HTTP analyzer that was attached due to well-known-port, but a later DPD signature match from upon seeing the HTTP reply will end up attaching another HTTP analyzer. More surprising is that upon re-attaching that analyzer, you'll get the same http_request as before since the DPD buffer will get replayed into the new analyzer. - Fixes disable_analyzer() to work when called even earlier, like within the protocol_confirmation event. At that time, the Analyzer tree may have not properly added the new analyzer into Analyzer::children yet, but rather the temporary waiting list, Analyzer::new_children. Analyzer::RemoveChildAnalyzer previously did not inspect the later list. - Fixes disable_analyzer() when called on an analyzer added to the tree via TCP_Analyzer::AddChildPacketAnalyzer. TCP_Analyzer keeps track of such children in its own list, TCP_Analyzer::packet_children, which the previous Analyzer::RemoveChildAnalyzer implementation didn't inspect. --- src/analyzer/Analyzer.cc | 95 +++++++++++-------- src/analyzer/Analyzer.h | 55 ++++++++--- src/analyzer/protocol/pia/PIA.cc | 5 +- src/analyzer/protocol/tcp/TCP.cc | 33 ++++++- src/analyzer/protocol/tcp/TCP.h | 1 + src/zeek.bif | 18 +++- .../Baseline/bifs.disable_analyzer-early/out | 6 ++ .../out | 2 + .../btest/Baseline/bifs.disable_analyzer/out | 6 ++ .../btest/bifs/disable_analyzer-early.zeek | 34 +++++++ .../disable_analyzer-tcp-packet-children.zeek | 16 ++++ testing/btest/bifs/disable_analyzer.zeek | 34 +++++++ 12 files changed, 247 insertions(+), 58 deletions(-) create mode 100644 testing/btest/Baseline/bifs.disable_analyzer-early/out create mode 100644 testing/btest/Baseline/bifs.disable_analyzer-tcp-packet-children/out create mode 100644 testing/btest/Baseline/bifs.disable_analyzer/out create mode 100644 testing/btest/bifs/disable_analyzer-early.zeek create mode 100644 testing/btest/bifs/disable_analyzer-tcp-packet-children.zeek create mode 100644 testing/btest/bifs/disable_analyzer.zeek diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc index d0d3731463..0d4e532456 100644 --- a/src/analyzer/Analyzer.cc +++ b/src/analyzer/Analyzer.cc @@ -91,12 +91,6 @@ bool Analyzer::IsAnalyzer(const char* name) return strcmp(analyzer_mgr->GetComponentName(tag).c_str(), name) == 0; } -// Used in debugging output. -static string fmt_analyzer(Analyzer* a) - { - return string(a->GetAnalyzerName()) + fmt("[%d]", a->GetID()); - } - Analyzer::Analyzer(const char* name, Connection* conn) { Tag tag = analyzer_mgr->GetComponentTag(name); @@ -381,7 +375,11 @@ void Analyzer::ForwardEndOfData(bool orig) bool Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init) { - if ( HasChildAnalyzer(analyzer->GetAnalyzerTag()) ) + auto t = analyzer->GetAnalyzerTag(); + auto it = std::find(prevented.begin(), prevented.end(), t); + auto prevent = it != prevented.end(); + + if ( HasChildAnalyzer(t) || prevent ) { analyzer->Done(); delete analyzer; @@ -407,46 +405,67 @@ bool Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init) Analyzer* Analyzer::AddChildAnalyzer(Tag analyzer) { - if ( ! HasChildAnalyzer(analyzer) ) - { - Analyzer* a = analyzer_mgr->InstantiateAnalyzer(analyzer, conn); + if ( HasChildAnalyzer(analyzer) ) + return nullptr; - if ( a && AddChildAnalyzer(a) ) - return a; + auto it = std::find(prevented.begin(), prevented.end(), analyzer); + + if ( it != prevented.end() ) + return nullptr; + + Analyzer* a = analyzer_mgr->InstantiateAnalyzer(analyzer, conn); + + if ( a && AddChildAnalyzer(a) ) + return a; + + return nullptr; + } + +bool Analyzer::RemoveChild(const analyzer_list& children, ID id) + { + for ( const auto& i : children ) + { + if ( i->id != id ) + continue; + + if ( i->finished || i->removing ) + return false; + + DBG_LOG(DBG_ANALYZER, "%s disabling child %s", + fmt_analyzer(this).c_str(), fmt_analyzer(i).c_str()); + // We just flag it as being removed here but postpone + // actually doing that to later. Otherwise, we'd need + // to call Done() here, which then in turn might + // cause further code to be executed that may assume + // something not true because of a violation that + // triggered the removal in the first place. + i->removing = true; + return true; } - return 0; + return false; } -void Analyzer::RemoveChildAnalyzer(Analyzer* analyzer) +bool Analyzer::RemoveChildAnalyzer(ID id) { - LOOP_OVER_CHILDREN(i) - if ( *i == analyzer && ! (analyzer->finished || analyzer->removing) ) - { - DBG_LOG(DBG_ANALYZER, "%s disabling child %s", - fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str()); - // We just flag it as being removed here but postpone - // actually doing that to later. Otherwise, we'd need - // to call Done() here, which then in turn might - // cause further code to be executed that may assume - // something not true because of a violation that - // triggered the removal in the first place. - (*i)->removing = true; - return; - } + return RemoveChild(children, id) || RemoveChild(new_children, id); } -void Analyzer::RemoveChildAnalyzer(ID id) +bool Analyzer::Remove() { - LOOP_OVER_CHILDREN(i) - if ( (*i)->id == id && ! ((*i)->finished || (*i)->removing) ) - { - DBG_LOG(DBG_ANALYZER, "%s disabling child %s", - fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str()); - // See comment above. - (*i)->removing = true; - return; - } + assert(parent); + parent->RemoveChildAnalyzer(this); + return removing; + } + +void Analyzer::PreventChildren(Tag tag) + { + auto it = std::find(prevented.begin(), prevented.end(), tag); + + if ( it != prevented.end() ) + return; + + prevented.emplace_back(tag); } bool Analyzer::HasChildAnalyzer(Tag tag) diff --git a/src/analyzer/Analyzer.h b/src/analyzer/Analyzer.h index 141d420a82..bd959eb370 100644 --- a/src/analyzer/Analyzer.h +++ b/src/analyzer/Analyzer.h @@ -4,6 +4,7 @@ #define ANALYZER_ANALYZER_H #include +#include #include "Tag.h" @@ -318,6 +319,12 @@ public: */ bool IsFinished() const { return finished; } + /** + * Returns true if the analyzer has been flagged for removal and + * shouldn't be used anymore. + */ + bool Removing() const { return removing; } + /** * Returns the tag associated with the analyzer's type. */ @@ -349,19 +356,19 @@ public: /** * Adds a new child analyzer to the analyzer tree. If an analyzer of - * the same type already exists, the one passes in is silenty - * discarded. + * the same type already exists or is prevented, the one passed in is + * silently discarded. * * @param analyzer The ananlyzer to add. Takes ownership. - * @return false if analyzer type was already a child, else true. + * @return false if analyzer type was already a child or prevented, else true. */ bool AddChildAnalyzer(Analyzer* analyzer) { return AddChildAnalyzer(analyzer, true); } /** * Adds a new child analyzer to the analyzer tree. If an analyzer of - * the same type already exists, the one passes in is silenty - * discarded. + * the same type already exists or is prevented, the one passed in is + * silently discarded. * * @param tag The type of analyzer to add. * @return the new analyzer instance that was added. @@ -373,16 +380,30 @@ public: * child, in which case the method does nothing. * * @param analyzer The analyzer to remove. + * + * @return whether the child analyzer is scheduled for removal + * (and was not before). */ - void RemoveChildAnalyzer(Analyzer* analyzer); + bool RemoveChildAnalyzer(Analyzer* analyzer) + { return RemoveChildAnalyzer(analyzer->GetID()); } /** * Removes a child analyzer. It's ok for the analyzer to not to be a * child, in which case the method does nothing. * - * @param tag The type of analyzer to remove. + * @param id The type of analyzer to remove. + * + * @return whether the child analyzer is scheduled for removal + * (and was not before). */ - void RemoveChildAnalyzer(ID id); + virtual bool RemoveChildAnalyzer(ID id); + + /** + * Prevents an analyzer type from ever being added as a child. + * + * @param tag The type of analyzer to prevent. + */ + void PreventChildren(Tag tag); /** * Returns true if analyzer has a direct child of a given type. @@ -450,8 +471,10 @@ public: /** * Remove the analyzer form its parent. The analyzer must have a * parent associated with it. + * + * @return whether the analyzer is being removed */ - void Remove() { assert(parent); parent->RemoveChildAnalyzer(this); } + bool Remove(); /** * Appends a support analyzer to the current list. @@ -570,6 +593,13 @@ protected: friend class ::Connection; friend class tcp::TCP_ApplicationAnalyzer; + /** + * Return a string represantation of an analyzer, containing its name + * and ID. + */ + static string fmt_analyzer(const Analyzer* a) + { return string(a->GetAnalyzerName()) + fmt("[%d]", a->GetID()); } + /** * Associates a connection with this analyzer. Must be called if * using the default ctor. @@ -644,10 +674,10 @@ protected: void AppendNewChildren(); /** - * Returns true if the analyzer has been flagged for removal and - * shouldn't be used otherwise anymore. + * Returns true if the child analyzer is now scheduled to be + * removed (and was not before) */ - bool Removing() const { return removing; } + bool RemoveChild(const analyzer_list& children, ID id); private: // Internal method to eventually delete a child analyzer that's @@ -670,6 +700,7 @@ private: SupportAnalyzer* resp_supporters; analyzer_list new_children; + std::vector prevented; bool protocol_confirmed; diff --git a/src/analyzer/protocol/pia/PIA.cc b/src/analyzer/protocol/pia/PIA.cc index 8f5e23a1ce..1becfb2cb0 100644 --- a/src/analyzer/protocol/pia/PIA.cc +++ b/src/analyzer/protocol/pia/PIA.cc @@ -282,10 +282,11 @@ void PIA_TCP::ActivateAnalyzer(analyzer::Tag tag, const Rule* rule) return; } - if ( Parent()->HasChildAnalyzer(tag) ) + analyzer::Analyzer* a = Parent()->AddChildAnalyzer(tag); + + if ( ! a ) return; - analyzer::Analyzer* a = Parent()->AddChildAnalyzer(tag); a->SetSignature(rule); // We have two cases here: diff --git a/src/analyzer/protocol/tcp/TCP.cc b/src/analyzer/protocol/tcp/TCP.cc index 5be893fd8e..df4c149e89 100644 --- a/src/analyzer/protocol/tcp/TCP.cc +++ b/src/analyzer/protocol/tcp/TCP.cc @@ -205,6 +205,15 @@ analyzer::Analyzer* TCP_Analyzer::FindChild(Tag arg_tag) return 0; } +bool TCP_Analyzer::RemoveChildAnalyzer(ID id) + { + auto rval = analyzer::TransportLayerAnalyzer::RemoveChildAnalyzer(id); + + if ( rval ) + return rval; + + return RemoveChild(packet_children, id); + } void TCP_Analyzer::EnableReassembly() { @@ -1209,8 +1218,28 @@ void TCP_Analyzer::DeliverPacket(int len, const u_char* data, bool is_orig, // Handle child_packet analyzers. Note: This happens *after* the // packet has been processed and the TCP state updated. - LOOP_OVER_GIVEN_CHILDREN(i, packet_children) - (*i)->NextPacket(len, data, is_orig, rel_data_seq, ip, caplen); + analyzer_list::iterator next; + + for ( auto i = packet_children.begin(); i != packet_children.end(); i = next ) + { + auto child = *i; + next = ++i; + + if ( child->IsFinished() || child->Removing() ) + { + --i; + + if ( child->Removing() ) + child->Done(); + + DBG_LOG(DBG_ANALYZER, "%s deleted child %s", + fmt_analyzer(this).c_str(), fmt_analyzer(child).c_str()); + packet_children.erase(i); + delete child; + } + else + child->NextPacket(len, data, is_orig, rel_data_seq, ip, caplen); + } if ( ! reassembling ) ForwardPacket(len, data, is_orig, rel_data_seq, ip, caplen); diff --git a/src/analyzer/protocol/tcp/TCP.h b/src/analyzer/protocol/tcp/TCP.h index 95ef5c72d7..be0be4da82 100644 --- a/src/analyzer/protocol/tcp/TCP.h +++ b/src/analyzer/protocol/tcp/TCP.h @@ -36,6 +36,7 @@ public: Analyzer* FindChild(ID id) override; Analyzer* FindChild(Tag tag) override; + bool RemoveChildAnalyzer(ID id) override; // True if the connection has closed in some sense, false otherwise. int IsClosed() const { return orig->did_close || resp->did_close; } diff --git a/src/zeek.bif b/src/zeek.bif index 71c49f1c9e..48e18b5fec 100644 --- a/src/zeek.bif +++ b/src/zeek.bif @@ -4228,11 +4228,18 @@ function file_mode%(mode: count%): string ## ## aid: The analyzer ID. ## +## err_if_no_conn: Emit an error message if the connection does not exit. +## +## prevent: Prevent the same analyzer type from being attached in the future. +## This is useful for preventing a the same analyzer from being +## automatically attached in the future, e.g. as a result of a +## DPD signature suddenly matching. +## ## Returns: True if the connection identified by *cid* exists and has analyzer -## *aid*. +## *aid* and it is scheduled for removal. ## ## .. zeek:see:: Analyzer::schedule_analyzer Analyzer::name -function disable_analyzer%(cid: conn_id, aid: count, err_if_no_conn: bool &default=T%) : bool +function disable_analyzer%(cid: conn_id, aid: count, err_if_no_conn: bool &default=T, prevent: bool &default=F%) : bool %{ Connection* c = sessions->FindConnection(cid); if ( ! c ) @@ -4249,8 +4256,11 @@ function disable_analyzer%(cid: conn_id, aid: count, err_if_no_conn: bool &defau return val_mgr->GetBool(0); } - a->Remove(); - return val_mgr->GetBool(1); + if ( prevent ) + a->Parent()->PreventChildren(a->GetAnalyzerTag()); + + auto rval = a->Remove(); + return val_mgr->GetBool(rval); %} ## Informs Zeek that it should skip any further processing of the contents of diff --git a/testing/btest/Baseline/bifs.disable_analyzer-early/out b/testing/btest/Baseline/bifs.disable_analyzer-early/out new file mode 100644 index 0000000000..6a5ffcae8d --- /dev/null +++ b/testing/btest/Baseline/bifs.disable_analyzer-early/out @@ -0,0 +1,6 @@ +proto confirm, Analyzer::ANALYZER_HTTP +T +http_request, GET, /style/enhanced.css +total http messages, { +[[orig_h=192.168.1.104, orig_p=1673/tcp, resp_h=63.245.209.11, resp_p=80/tcp]] = 1 +} diff --git a/testing/btest/Baseline/bifs.disable_analyzer-tcp-packet-children/out b/testing/btest/Baseline/bifs.disable_analyzer-tcp-packet-children/out new file mode 100644 index 0000000000..70cd4de8e9 --- /dev/null +++ b/testing/btest/Baseline/bifs.disable_analyzer-tcp-packet-children/out @@ -0,0 +1,2 @@ +triggered packets, [orig_h=192.168.1.104, orig_p=1673/tcp, resp_h=63.245.209.11, resp_p=80/tcp], 1, T +T diff --git a/testing/btest/Baseline/bifs.disable_analyzer/out b/testing/btest/Baseline/bifs.disable_analyzer/out new file mode 100644 index 0000000000..1d57ac49fd --- /dev/null +++ b/testing/btest/Baseline/bifs.disable_analyzer/out @@ -0,0 +1,6 @@ +proto confirm, Analyzer::ANALYZER_HTTP +http_request, GET, /style/enhanced.css +T +total http messages, { +[[orig_h=192.168.1.104, orig_p=1673/tcp, resp_h=63.245.209.11, resp_p=80/tcp]] = 1 +} diff --git a/testing/btest/bifs/disable_analyzer-early.zeek b/testing/btest/bifs/disable_analyzer-early.zeek new file mode 100644 index 0000000000..bab832a489 --- /dev/null +++ b/testing/btest/bifs/disable_analyzer-early.zeek @@ -0,0 +1,34 @@ +# @TEST-EXEC: zeek -b -r $TRACES/http/pipelined-requests.trace %INPUT >out +# @TEST-EXEC: btest-diff out + +@load base/protocols/http + +global msg_count: table[conn_id] of count &default=0; + +event protocol_confirmation(c: connection, atype: Analyzer::Tag, aid: count) &priority=10 + { + if ( atype != Analyzer::ANALYZER_HTTP ) + return; + + print "proto confirm", atype; + print disable_analyzer(c$id, aid, T, T); + } + +event http_request(c: connection, method: string, original_URI: string, unescaped_URI: string, version: string) + { + ++msg_count[c$id]; + print "http_request", method, original_URI; + } + +event http_reply(c: connection, version: string, code: count, reason: string) + { + ++msg_count[c$id]; + print "http_reply", code; + } + +event zeek_done() + { + print "total http messages", msg_count; + } + + diff --git a/testing/btest/bifs/disable_analyzer-tcp-packet-children.zeek b/testing/btest/bifs/disable_analyzer-tcp-packet-children.zeek new file mode 100644 index 0000000000..b719eb010f --- /dev/null +++ b/testing/btest/bifs/disable_analyzer-tcp-packet-children.zeek @@ -0,0 +1,16 @@ +# @TEST-EXEC: zeek -b -r $TRACES/http/pipelined-requests.trace %INPUT >out +# @TEST-EXEC: btest-diff out + +@load base/protocols/http + +event connection_established(c: connection) + { + set_current_conn_packets_threshold(c$id, 1, T); + } + +event conn_packets_threshold_crossed(c: connection, threshold: count, is_orig: bool) + { + print "triggered packets", c$id, threshold, is_orig; + set_current_conn_packets_threshold(c$id, threshold + 1, T); + print disable_analyzer(c$id, current_analyzer(), T); + } diff --git a/testing/btest/bifs/disable_analyzer.zeek b/testing/btest/bifs/disable_analyzer.zeek new file mode 100644 index 0000000000..d3abbe893e --- /dev/null +++ b/testing/btest/bifs/disable_analyzer.zeek @@ -0,0 +1,34 @@ +# @TEST-EXEC: zeek -b -r $TRACES/http/pipelined-requests.trace %INPUT >out +# @TEST-EXEC: btest-diff out + +@load base/protocols/http + +global msg_count: table[conn_id] of count &default=0; + +event protocol_confirmation(c: connection, atype: Analyzer::Tag, aid: count) &priority=10 + { + if ( atype != Analyzer::ANALYZER_HTTP ) + return; + + print "proto confirm", atype; + } + +event http_request(c: connection, method: string, original_URI: string, unescaped_URI: string, version: string) + { + ++msg_count[c$id]; + print "http_request", method, original_URI; + print disable_analyzer(c$id, current_analyzer(), T, T); + } + +event http_reply(c: connection, version: string, code: count, reason: string) + { + ++msg_count[c$id]; + print "http_reply", code; + } + +event zeek_done() + { + print "total http messages", msg_count; + } + +