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

@@ -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) )
+	if ( HasChildAnalyzer(analyzer) )
-		{
+		return nullptr;
-		Analyzer* a = analyzer_mgr->InstantiateAnalyzer(analyzer, conn);
 
-		if ( a && AddChildAnalyzer(a) )
+	auto it = std::find(prevented.begin(), prevented.end(), analyzer);
-			return a;
+
+	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)
+	return RemoveChild(children, id) || RemoveChild(new_children, id);
-		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;
-			}
 	}
 
-void Analyzer::RemoveChildAnalyzer(ID id)
+bool Analyzer::Remove()
 	{
-	LOOP_OVER_CHILDREN(i)
+	assert(parent);
-		if ( (*i)->id == id && ! ((*i)->finished || (*i)->removing) )
+	parent->RemoveChildAnalyzer(this);
-			{
+	return removing;
-			DBG_LOG(DBG_ANALYZER, "%s  disabling child %s",
+	}
-					fmt_analyzer(this).c_str(), fmt_analyzer(*i).c_str());
+
-			// See comment above.
+void Analyzer::PreventChildren(Tag tag)
-			(*i)->removing = true;
+	{
-			return;
+	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
+	 * the same type already exists or is prevented, the one passed in is
-	 * discarded.
+	 * 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
+	 * the same type already exists or is prevented, the one passed in is
-	 * discarded.
+	 * 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
+	 * Returns true if the child analyzer is now scheduled to be
-	 * shouldn't be used otherwise anymore.
+	 * 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)
+	analyzer_list::iterator next;
-		(*i)->NextPacket(len, data, is_orig, rel_data_seq, ip, caplen);
+
+	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);

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

@@ -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();
+	if ( prevent )
-	return val_mgr->GetBool(1);
+		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;
+	}
+
+