From 51e738a1c09721cd80964560b34b7e1ef35dd407 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 15 Jun 2020 12:53:46 -0700 Subject: [PATCH] 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 | 11 ++++ src/Reporter.cc | 65 +++++++++++++++++++ src/Reporter.h | 17 +++++ src/Timer.cc | 1 + src/Timer.h | 1 + src/event.bif | 37 +++++++++-- src/reporter.bif | 12 +++- .../Baseline/core.expired-conn-weird/out | 2 + testing/btest/core/expired-conn-weird.zeek | 22 +++++++ 9 files changed, 163 insertions(+), 5 deletions(-) create mode 100644 testing/btest/Baseline/core.expired-conn-weird/out create mode 100644 testing/btest/core/expired-conn-weird.zeek diff --git a/scripts/base/frameworks/notice/weird.zeek b/scripts/base/frameworks/notice/weird.zeek index ea876e6495..5d61cb3bfd 100644 --- a/scripts/base/frameworks/notice/weird.zeek +++ b/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 diff --git a/src/Reporter.cc b/src/Reporter.cc index e881706834..467ab7e0bf 100644 --- a/src/Reporter.cc +++ b/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 conn_id, IntrusivePtr 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); diff --git a/src/Reporter.h b/src/Reporter.h index 4c35009c82..0c09dfd722 100644 --- a/src/Reporter.h +++ b/src/Reporter.h @@ -7,11 +7,13 @@ #include #include #include +#include #include #include #include #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 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; + using ConnTuple = std::tuple; using WeirdCountMap = std::unordered_map; using WeirdFlowMap = std::map; + using WeirdConnTupleMap = std::map; using WeirdSet = std::unordered_set; 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 conn_id, IntrusivePtr 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; diff --git a/src/Timer.cc b/src/Timer.cc index 77d74fcdbb..97751a9b00 100644 --- a/src/Timer.cc +++ b/src/Timer.cc @@ -19,6 +19,7 @@ const char* TimerNames[] = { "ConnectionExpireTimer", "ConnectionInactivityTimer", "ConnectionStatusUpdateTimer", + "ConnTupleWeirdTimer", "DNSExpireTimer", "FileAnalysisInactivityTimer", "FlowWeirdTimer", diff --git a/src/Timer.h b/src/Timer.h index 970eef7064..8a82eb4627 100644 --- a/src/Timer.h +++ b/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, diff --git a/src/event.bif b/src/event.bif index 78bdefc0f9..c79f0c78a5 100644 --- a/src/event.bif +++ b/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 diff --git a/src/reporter.bif b/src/reporter.bif index addd56807c..2133bd7d00 100644 --- a/src/reporter.bif +++ b/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("id"); + auto uid_val = connection_record->GetField("uid"); + reporter->Weird(conn_id_val, uid_val, + name->CheckString(), addl->CheckString()); + } + return val_mgr->True(); %} diff --git a/testing/btest/Baseline/core.expired-conn-weird/out b/testing/btest/Baseline/core.expired-conn-weird/out new file mode 100644 index 0000000000..124a0fa17b --- /dev/null +++ b/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 diff --git a/testing/btest/core/expired-conn-weird.zeek b/testing/btest/core/expired-conn-weird.zeek new file mode 100644 index 0000000000..6fa6dc394e --- /dev/null +++ b/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; + }