Merge remote-tracking branch 'origin/topic/jsiwek/gh-532-improve-disable-analyzer'

Includes fix for potential iterator invalidation during iteration.

* origin/topic/jsiwek/gh-532-improve-disable-analyzer:
  GH-532: improve disable_analyzer BIF

CHANGES

@@ -1,4 +1,18 @@
 
+3.1.0-dev.58 | 2019-08-16 18:45:39 +0000
+
+  * GH-532: Improve disable_analyzer BIF. (Jon Siwek, Corelight)
+
+    - Add an extra "prevent" parameter (default value of false), which
+      helps prevent the same analyzer type from being attached in the
+      future.
+
+    - Fixes disable_analyzer() to work when called even earlier, like
+      within the protocol_confirmation event.
+
+    - Fixes disable_analyzer() when called on an analyzer added to the
+      tree via TCP_Analyzer::AddChildPacketAnalyzer.
+
 3.1.0-dev.55 | 2019-08-14 16:18:44 -0700
 
   * Fix misc. Coverity warnings (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-3.1.0-dev.55
+3.1.0-dev.58

doc

@@ -1 +1 @@
-Subproject commit 9305032f1ae14ec8a988ecd4e6217ef16d7c6415
+Subproject commit fb4d8cda00c30d7ae37b864521b14447fff5586b

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)

src/analyzer/Analyzer.h

@@ -4,6 +4,7 @@
 #define ANALYZER_ANALYZER_H
 
 #include <list>
+#include <vector>
 
 #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<Tag> prevented;
 
 	bool protocol_confirmed;
 

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:

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(); /* nop */ )
+		{
+		auto child = *i;
+
+		if ( child->IsFinished() || child->Removing() )
+			{
+			if ( child->Removing() )
+				child->Done();
+
+			DBG_LOG(DBG_ANALYZER, "%s deleted child %s",
+			        fmt_analyzer(this).c_str(), fmt_analyzer(child).c_str());
+			i = packet_children.erase(i);
+			delete child;
+			}
+		else
+			{
+			child->NextPacket(len, data, is_orig, rel_data_seq, ip, caplen);
+			++i;
+			}
+		}
 
 	if ( ! reassembling )
 		ForwardPacket(len, data, is_orig, rel_data_seq, ip, caplen);

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; }

src/zeek.bif

@@ -4223,11 +4223,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 the same analyzer from being
+##          automatically reattached 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 )
@@ -4244,8 +4251,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

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
+}

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

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
+}

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;
+	}
+
+

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);
+	}

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;
+	}
+
+