GH-998: Fix Reporter::conn_weird() to handle expired connections

This introduces a new sampling state-map for expired connections to fix
segfaults that previously occured when passing in a `connection` record
to `Reporter::conn_weird()` for which the internal `Connection` object
had already been expired and deleted.  This also introduces a new event
called `expired_conn_weird`, which is similar to `conn_weird`, except
the full `connection` record is no longer available, just the `conn_id`
and UID string.

scripts/base/frameworks/notice/weird.zeek

@@ -406,6 +406,17 @@ event conn_weird(name: string, c: connection, addl: string)
 	weird(i);
 	}
 
+event expired_conn_weird(name: string, id: conn_id, uid: string, addl: string)
+	{
+	local i = Info($ts=network_time(), $name=name, $uid=uid, $id=id,
+	               $identifier=id_string(id));
+
+	if ( addl != "" )
+		i$addl = addl;
+
+	weird(i);
+	}
+
 event flow_weird(name: string, src: addr, dst: addr, addl: string)
 	{
 	# We add the source and destination as port 0/unknown because that is

src/Reporter.cc

@@ -273,6 +273,21 @@ public:
 	IPPair endpoints;
 };
 
+class ConnTupleWeirdTimer final : public Timer {
+public:
+	using ConnTuple = Reporter::ConnTuple;
+
+	ConnTupleWeirdTimer(double t, ConnTuple id, double timeout)
+		: Timer(t + timeout, TIMER_CONN_TUPLE_WEIRD_EXPIRE),
+		  conn_id(std::move(id))
+		{}
+
+	void Dispatch(double t, bool is_expire) override
+		{ reporter->ResetExpiredConnWeird(conn_id); }
+
+	ConnTuple conn_id;
+};
+
 void Reporter::ResetNetWeird(const std::string& name)
 	{
 	net_weird_state.erase(name);
@@ -283,6 +298,11 @@ void Reporter::ResetFlowWeird(const IPAddr& orig, const IPAddr& resp)
 	flow_weird_state.erase(std::make_pair(orig, resp));
 	}
 
+void Reporter::ResetExpiredConnWeird(const ConnTuple& id)
+	{
+	expired_conn_weird_state.erase(id);
+	}
+
 bool Reporter::PermitNetWeird(const char* name)
 	{
 	auto& count = net_weird_state[name];
@@ -325,6 +345,35 @@ bool Reporter::PermitFlowWeird(const char* name,
 		return false;
 	}
 
+bool Reporter::PermitExpiredConnWeird(const char* name, const RecordVal& conn_id)
+	{
+	auto conn_tuple = std::make_tuple(conn_id.GetField("orig_h")->AsAddr(),
+	                                  conn_id.GetField("resp_h")->AsAddr(),
+	                                  conn_id.GetField("orig_p")->AsPortVal()->Port(),
+	                                  conn_id.GetField("resp_p")->AsPortVal()->Port(),
+	                                  conn_id.GetField("resp_p")->AsPortVal()->PortType());
+
+	auto& map = expired_conn_weird_state[conn_tuple];
+
+	if ( map.empty() )
+		timer_mgr->Add(new ConnTupleWeirdTimer(network_time,
+		                                       std::move(conn_tuple),
+		                                       weird_sampling_duration));
+
+	auto& count = map[name];
+	++count;
+
+	if ( count <= weird_sampling_threshold )
+		return true;
+
+	auto num_above_threshold = count - weird_sampling_threshold;
+
+	if ( weird_sampling_rate )
+		return num_above_threshold % weird_sampling_rate == 0;
+	else
+		return false;
+	}
+
 void Reporter::Weird(const char* name, const char* addl)
 	{
 	UpdateWeirdStats(name);
@@ -368,6 +417,22 @@ void Reporter::Weird(Connection* conn, const char* name, const char* addl)
 	            "%s", name);
 	}
 
+void Reporter::Weird(IntrusivePtr<RecordVal> conn_id, IntrusivePtr<StringVal> uid,
+                     const char* name, const char* addl)
+	{
+	UpdateWeirdStats(name);
+
+	if ( ! WeirdOnSamplingWhiteList(name) )
+		{
+		if ( ! PermitExpiredConnWeird(name, *conn_id) )
+			return;
+		}
+
+	WeirdHelper(expired_conn_weird,
+	            {conn_id.release(), uid.release(), new StringVal(addl)},
+	            "%s", name);
+	}
+
 void Reporter::Weird(const IPAddr& orig, const IPAddr& resp, const char* name, const char* addl)
 	{
 	UpdateWeirdStats(name);

src/Reporter.h

@@ -7,11 +7,13 @@
 #include <list>
 #include <utility>
 #include <string>
+#include <tuple>
 #include <map>
 #include <unordered_set>
 #include <unordered_map>
 
 #include "BroList.h"
+#include "net_util.h"
 
 namespace analyzer { class Analyzer; }
 namespace file_analysis { class File; }
@@ -19,6 +21,10 @@ class Connection;
 class Location;
 class Reporter;
 class EventHandlerPtr;
+class RecordVal;
+class StringVal;
+
+template <class T> class IntrusivePtr;
 
 // One cannot raise this exception directly, go through the
 // Reporter's methods instead.
@@ -44,8 +50,10 @@ ZEEK_FORWARD_DECLARE_NAMESPACED(Expr, zeek::detail);
 class Reporter {
 public:
 	using IPPair = std::pair<IPAddr, IPAddr>;
+	using ConnTuple = std::tuple<IPAddr, IPAddr, uint32_t, uint32_t, TransportProto>;
 	using WeirdCountMap = std::unordered_map<std::string, uint64_t>;
 	using WeirdFlowMap = std::map<IPPair, WeirdCountMap>;
+	using WeirdConnTupleMap = std::map<ConnTuple, WeirdCountMap>;
 	using WeirdSet = std::unordered_set<std::string>;
 
 	Reporter(bool abort_on_scripting_errors);
@@ -89,6 +97,8 @@ public:
 	void Weird(const char* name, const char* addl = "");	// Raises net_weird().
 	void Weird(file_analysis::File* f, const char* name, const char* addl = "");	// Raises file_weird().
 	void Weird(Connection* conn, const char* name, const char* addl = "");	// Raises conn_weird().
+	void Weird(IntrusivePtr<RecordVal> conn_id, IntrusivePtr<StringVal> uid,
+	           const char* name, const char* addl = "");	// Raises expired_conn_weird().
 	void Weird(const IPAddr& orig, const IPAddr& resp, const char* name, const char* addl = "");	// Raises flow_weird().
 
 	// Syslog a message. This methods does nothing if we're running
@@ -142,6 +152,11 @@ public:
 	 */
 	void ResetFlowWeird(const IPAddr& orig, const IPAddr& resp);
 
+	/**
+	 * Reset/cleanup state tracking for a "expired conn" weird.
+	 */
+	void ResetExpiredConnWeird(const ConnTuple& id);
+
 	/**
 	 * Return the total number of weirds generated (counts weirds before
 	 * any rate-limiting occurs).
@@ -255,6 +270,7 @@ private:
 		{ return weird_sampling_whitelist.find(name) != weird_sampling_whitelist.end(); }
 	bool PermitNetWeird(const char* name);
 	bool PermitFlowWeird(const char* name, const IPAddr& o, const IPAddr& r);
+	bool PermitExpiredConnWeird(const char* name, const RecordVal& conn_id);
 
 	bool EmitToStderr(bool flag)
 		{ return flag || ! after_zeek_init; }
@@ -274,6 +290,7 @@ private:
 	WeirdCountMap weird_count_by_type;
 	WeirdCountMap net_weird_state;
 	WeirdFlowMap flow_weird_state;
+	WeirdConnTupleMap expired_conn_weird_state;
 
 	WeirdSet weird_sampling_whitelist;
 	uint64_t weird_sampling_threshold;

src/Timer.cc

@@ -19,6 +19,7 @@ const char* TimerNames[] = {
 	"ConnectionExpireTimer",
 	"ConnectionInactivityTimer",
 	"ConnectionStatusUpdateTimer",
+	"ConnTupleWeirdTimer",
 	"DNSExpireTimer",
 	"FileAnalysisInactivityTimer",
 	"FlowWeirdTimer",

src/Timer.h

@@ -15,6 +15,7 @@ enum TimerType : uint8_t {
 	TIMER_CONN_EXPIRE,
 	TIMER_CONN_INACTIVITY,
 	TIMER_CONN_STATUS_UPDATE,
+	TIMER_CONN_TUPLE_WEIRD_EXPIRE,
 	TIMER_DNS_EXPIRE,
 	TIMER_FILE_ANALYSIS_INACTIVITY,
 	TIMER_FLOW_WEIRD_EXPIRE,

src/event.bif

@@ -493,7 +493,7 @@ event conn_stats%(c: connection, os: endpoint_stats, rs: endpoint_stats%);
 ##
 ## addl: Optional additional context further describing the situation.
 ##
-## .. zeek:see:: flow_weird net_weird file_weird
+## .. zeek:see:: flow_weird net_weird file_weird expired_conn_weird
 ##
 ## .. note:: "Weird" activity is much more common in real-world network traffic
 ##    than one would intuitively expect. While in principle, any protocol
@@ -501,6 +501,35 @@ event conn_stats%(c: connection, os: endpoint_stats, rs: endpoint_stats%);
 ##    endpoint's implementation interprets an RFC quite liberally.
 event conn_weird%(name: string, c: connection, addl: string%);
 
+## Generated for unexpected activity related to a specific connection whose
+## internal state has already been expired.  That is to say,
+## :zeek:see:`Reporter::conn_weird` may have been called from a script, but
+## the internal connection object/state was expired and so the full
+## :zeek:see:`connection` record is no longer available, just the UID
+## and :zeek:see:`conn_id`.
+## When Zeek's packet analysis encounters activity that does not conform to a
+## protocol's specification, it raises one of the ``*_weird`` events to report
+## that. This event is raised if the activity is tied directly to a specific
+## connection.
+##
+## name: A unique name for the specific type of "weird" situation. Zeek's default
+##       scripts use this name in filtering policies that specify which
+##       "weirds" are worth reporting.
+##
+## id: The tuple associated with a previously-expired connection.
+##
+## uid: The UID string associated with a previously-expired connection.
+##
+## addl: Optional additional context further describing the situation.
+##
+## .. zeek:see:: flow_weird net_weird file_weird conn_weird
+##
+## .. note:: "Weird" activity is much more common in real-world network traffic
+##    than one would intuitively expect. While in principle, any protocol
+##    violation could be an attack attempt, it's much more likely that an
+##    endpoint's implementation interprets an RFC quite liberally.
+event expired_conn_weird%(name: string, id: conn_id, uid: string, addl: string%);
+
 ## Generated for unexpected activity related to a pair of hosts, but independent
 ## of a specific connection.  When Zeek's packet analysis encounters activity
 ## that does not conform to a protocol's specification, it raises one of
@@ -518,7 +547,7 @@ event conn_weird%(name: string, c: connection, addl: string%);
 ##
 ## addl: Optional additional context further describing the situation.
 ##
-## .. zeek:see:: conn_weird net_weird file_weird
+## .. zeek:see:: conn_weird net_weird file_weird expired_conn_weird
 ##
 ## .. note:: "Weird" activity is much more common in real-world network traffic
 ##    than one would intuitively expect. While in principle, any protocol
@@ -538,7 +567,7 @@ event flow_weird%(name: string, src: addr, dst: addr, addl: string%);
 ##
 ## addl: Optional additional context further describing the situation.
 ##
-## .. zeek:see:: flow_weird file_weird
+## .. zeek:see:: flow_weird file_weird conn_weird expired_conn_weird
 ##
 ## .. note:: "Weird" activity is much more common in real-world network traffic
 ##    than one would intuitively expect. While in principle, any protocol
@@ -559,7 +588,7 @@ event net_weird%(name: string, addl: string%);
 ##
 ## addl: Additional information related to the weird.
 ##
-## .. zeek:see:: flow_weird net_weird conn_weird
+## .. zeek:see:: flow_weird net_weird conn_weird expired_conn_weird
 ##
 ## .. note:: "Weird" activity is much more common in real-world network traffic
 ##    than one would intuitively expect. While in principle, any protocol

src/reporter.bif

@@ -123,7 +123,17 @@ function Reporter::flow_weird%(name: string, orig: addr, resp: addr%): bool
 ## Returns: Always true.
 function Reporter::conn_weird%(name: string, c: connection, addl: string &default=""%): bool
 	%{
-	reporter->Weird(c, name->CheckString(), addl->CheckString());
+	if ( c )
+		reporter->Weird(c, name->CheckString(), addl->CheckString());
+	else
+		{
+		auto connection_record = @ARG@[1]->AsRecordVal();
+		auto conn_id_val = connection_record->GetField<RecordVal>("id");
+		auto uid_val = connection_record->GetField<StringVal>("uid");
+		reporter->Weird(conn_id_val, uid_val,
+		                name->CheckString(), addl->CheckString());
+		}
+
 	return val_mgr->True();
 	%}
 

testing/btest/Baseline/core.expired-conn-weird/out

@@ -0,0 +1,2 @@
+expired_conn_weird, test!, [orig_h=192.168.1.200, orig_p=49206/tcp, resp_h=192.168.1.150, resp_p=3389/tcp], CHhAvVGS1DHFjwGM9, test2
+expired_conn_weird, test!, [orig_h=192.168.1.200, orig_p=49206/tcp, resp_h=192.168.1.150, resp_p=3389/tcp], CHhAvVGS1DHFjwGM9, test2

testing/btest/core/expired-conn-weird.zeek

@@ -0,0 +1,22 @@
+# @TEST-EXEC: zeek -b -r $TRACES/rdp/rdp-to-ssl.pcap %INPUT >out
+# @TEST-EXEC: btest-diff out
+
+redef Weird::sampling_threshold = 2;
+
+event my_event(c: connection)
+	{
+	Reporter::conn_weird("test!", c, "test2");
+	Reporter::conn_weird("test!", c, "test2");
+	Reporter::conn_weird("test!", c, "test2");
+	Reporter::conn_weird("test!", c, "test2");
+	}
+
+event connection_state_remove(c: connection)
+	{
+	schedule 1sec { my_event(c) };
+	}
+
+event expired_conn_weird(name: string, id: conn_id, uid: string, addl: string)
+	{
+	print "expired_conn_weird", name, id, uid, addl;
+	}