Merge remote-tracking branch 'origin/topic/awelzel/3028-connection-flipped'

* origin/topic/awelzel/3028-connection-flipped:
  Update dump-events baseline, not running with OpenSSL 3
  Conn: In-place val flip and connection_flipped()
  Conn: Remove is_version_sep()
  Remove icmp_conn leftovers

CHANGES

@@ -1,3 +1,29 @@
+6.1.0-dev.170 | 2023-07-05 13:32:21 -0700
+
+  * Update dump-events baseline, not running with OpenSSL 3 (Arne Welzel, Corelight)
+
+  * Conn: In-place val flip and connection_flipped() (Arne Welzel, Corelight)
+
+    Avoids loosing state on a connection value when a connection is flipped.
+
+    Fixes up the NTP baseline as well where this was visible: analyzer_confirmation_info()
+    was raised for a connection value which was immediately forgotten due to
+    the subsequent connection flipping.
+
+    Closed #3028
+
+  * Conn: Remove is_version_sep() (Arne Welzel, Corelight)
+
+    Leftover from 2655a653314237f5354fdb644bf642d0d5867f63.
+
+  * Remove icmp_conn leftovers (Arne Welzel, Corelight)
+
+    Roughly 2.5 years ago all events taking the ``icmp_conn`` parameter were
+    removed with 44ad614094db3c57fef7fb226c539f6f090a5421 and the NetVar.cc
+    type not populated anymore.
+
+    Remove the left-overs in script land, too.
+
 6.1.0-dev.164 | 2023-07-05 10:23:27 -0700
 
   * Bump Spicy to latest release. (Benjamin Bannier, Corelight)

NEWS

@@ -42,6 +42,13 @@ New Functionality
 Changed Functionality
 ---------------------
 
+- A connection's value is now updated in-place when its directionality is
+  flipped due to Zeek's heuristics (for example, SYN/SYN-ACK reversal or
+  protocol specific approaches).
+  Previously, a connection's value was discarded when flipped, including any
+  values set in a ``new_connection()`` handler. A new ``connection_flipped()``
+  event is added to allow updating custom state in script-land.
+
 Removed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-6.1.0-dev.164
+6.1.0-dev.170

scripts/base/frameworks/notice/main.zeek

@@ -81,10 +81,6 @@ export {
 		## reference to the actual connection will be deleted after
 		## applying the notice policy.
 		conn:           connection     &optional;
-		## A shorthand way of giving the uid and id to a notice.  The
-		## reference to the actual connection will be deleted after
-		## applying the notice policy.
-		iconn:          icmp_conn      &optional;
 
 		## A file record if the notice is related to a file.  The
 		## reference to the actual fa_file record will be deleted after
@@ -108,7 +104,7 @@ export {
 		file_desc:      string          &log &optional;
 
 		## The transport protocol. Filled automatically when either
-		## *conn*, *iconn* or *p* is specified.
+		## *conn* or *p* is specified.
 		proto:          transport_proto &log &optional;
 
 		## The :zeek:type:`Notice::Type` of the notice.
@@ -668,15 +664,6 @@ function apply_policy(n: Notice::Info)
 	if ( n?$p )
 		n$proto = get_port_transport_proto(n$p);
 
-	if ( n?$iconn )
-		{
-		n$proto = icmp;
-		if ( ! n?$src )
-			n$src = n$iconn$orig_h;
-		if ( ! n?$dst )
-			n$dst = n$iconn$resp_h;
-		}
-
 	if ( ! n?$email_body_sections )
 		n$email_body_sections = vector();
 	if ( ! n?$email_delay_tokens )

scripts/base/init-bare.zeek

@@ -231,21 +231,6 @@ type flow_id : record {
 	dst_p: port;	##< The destination port number.
 } &log;
 
-## Specifics about an ICMP conversation. ICMP events typically pass this in
-## addition to :zeek:type:`conn_id`.
-##
-## .. zeek:see:: icmp_echo_reply icmp_echo_request icmp_redirect icmp_sent
-##    icmp_time_exceeded icmp_unreachable
-type icmp_conn: record {
-	orig_h: addr;	##< The originator's IP address.
-	resp_h: addr;	##< The responder's IP address.
-	itype: count;	##< The ICMP type of the packet that triggered the instantiation of the record.
-	icode: count;	##< The ICMP code of the packet that triggered the instantiation of the record.
-	len: count;	##< The length of the ICMP payload of the packet that triggered the instantiation of the record.
-	hlim: count;	##< The encapsulating IP header's Hop Limit value.
-	v6: bool;	##< True if it's an ICMPv6 packet.
-};
-
 ## Specifics about an ICMP conversation/packet.
 ## ICMP events typically pass this in addition to :zeek:type:`conn_id`.
 ##

src/Conn.cc

@@ -217,6 +217,29 @@ void Connection::HistoryThresholdEvent(EventHandlerPtr e, bool is_orig, uint32_t
 	EnqueueEvent(e, nullptr, GetVal(), val_mgr->Bool(is_orig), val_mgr->Count(threshold));
 	}
 
+namespace
+	{
+// Flip everything that needs to be flipped in the connection
+// record that is known on this level. This needs to align
+// with GetVal() and connection's layout in init-bare.
+void flip_conn_val(const RecordValPtr& conn_val)
+	{
+	// Flip the the conn_id (c$id).
+	const auto& id_val = conn_val->GetField<zeek::RecordVal>(0);
+	const auto& tmp_addr = id_val->GetField<zeek::AddrVal>(0);
+	const auto& tmp_port = id_val->GetField<zeek::PortVal>(1);
+	id_val->Assign(0, id_val->GetField<zeek::AddrVal>(2));
+	id_val->Assign(1, id_val->GetField<zeek::PortVal>(3));
+	id_val->Assign(2, tmp_addr);
+	id_val->Assign(3, tmp_port);
+
+	// Flip the endpoints within connection.
+	const auto& tmp_endp = conn_val->GetField<zeek::RecordVal>(1);
+	conn_val->Assign(1, conn_val->GetField(2));
+	conn_val->Assign(2, tmp_endp);
+	}
+	}
+
 const RecordValPtr& Connection::GetVal()
 	{
 	if ( ! conn_val )
@@ -315,18 +338,6 @@ void Connection::AppendAddl(const char* str)
 	cv->Assign(6, util::fmt(format, old, str));
 	}
 
-// Returns true if the character at s separates a version number.
-static inline bool is_version_sep(const char* s, const char* end)
-	{
-	return
-		// foo-1.2.3
-		(s < end - 1 && ispunct(s[0]) && isdigit(s[1])) ||
-		// foo-v1.2.3
-		(s < end - 2 && ispunct(s[0]) && tolower(s[1]) == 'v' && isdigit(s[2])) ||
-		// foo 1.2.3
-		isspace(s[0]);
-	}
-
 void Connection::Match(detail::Rule::PatternType type, const u_char* data, int len, bool is_orig,
                        bool bol, bool eol, bool clear_state)
 	{
@@ -370,7 +381,8 @@ void Connection::FlipRoles()
 	resp_flow_label = orig_flow_label;
 	orig_flow_label = tmp_flow;
 
-	conn_val = nullptr;
+	if ( conn_val )
+		flip_conn_val(conn_val);
 
 	if ( adapter )
 		adapter->FlipRoles();
@@ -378,6 +390,9 @@ void Connection::FlipRoles()
 	analyzer_mgr->ApplyScheduledAnalyzers(this);
 
 	AddHistory('^');
+
+	if ( connection_flipped )
+		EnqueueEvent(connection_flipped, nullptr, GetVal());
 	}
 
 void Connection::Describe(ODesc* d) const

src/NetVar.cc

@@ -15,7 +15,6 @@ zeek::RecordType* endpoint_stats;
 zeek::RecordType* connection_type;
 zeek::RecordType* fa_file_type;
 zeek::RecordType* fa_metadata_type;
-zeek::RecordType* icmp_conn;
 zeek::RecordType* icmp_context;
 zeek::RecordType* SYN_packet;
 zeek::RecordType* pcap_packet;

src/event.bif

@@ -194,6 +194,17 @@ event connection_reused%(c: connection%);
 ##    new_connection new_connection_contents partial_connection
 event connection_status_update%(c: connection%);
 
+## Generated for a connection when the direction was flipped by Zeek's
+## heuristics and originator and responder roles were reversed. If state is
+## kept on a connection record for originator and responder, this event
+## can be used to update or reset such state. The ``orig`` and ``resp`` fields
+## as well as the contents of the ``id`` field reflect the post-flip state.
+##
+## c: The connection.
+##
+## .. zeek:see:: connection_established new_connection
+event connection_flipped%(c: connection%);
+
 ## Generated for a connection over IPv6 when one direction has changed
 ## the flow label that it's using.
 ##

testing/btest/Baseline/core.connection_flipped/out

@@ -0,0 +1,4 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+1362692526.939084, new_connection, [orig_h=192.150.187.43, orig_p=80/tcp, resp_h=141.142.228.5, resp_p=59856/tcp, extra_id=42], H, 1362692526.939084
+1362692526.939344, connection_flipped, [orig_h=141.142.228.5, orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp, extra_id=42], Hs^, 1362692526.939084
+1362692527.080972, connection_state_remove, [orig_h=141.142.228.5, orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp, extra_id=42], Hs^ADadFf, 1362692526.939084

testing/btest/Baseline/scripts.base.protocols.ntp.misordered-ntp/conn.log

@@ -7,5 +7,5 @@
 #open XXXX-XX-XX-XX-XX-XX
 #fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	service	duration	orig_bytes	resp_bytes	conn_state	local_orig	local_resp	missed_bytes	history	orig_pkts	orig_ip_bytes	resp_pkts	resp_ip_bytes	tunnel_parents
 #types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	192.168.1.95	123	17.253.4.253	123	udp	-	0.959285	96	0	S0	T	F	0	D^	2	152	0	0	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	192.168.1.95	123	17.253.4.253	123	udp	ntp	0.959285	96	0	S0	T	F	0	D^	2	152	0	0	-
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.policy.misc.dump-events/really-all-events.log

@@ -8016,6 +8016,9 @@ XXXXXXXXXX.XXXXXX event_queue_flush_point
 XXXXXXXXXX.XXXXXX event_queue_flush_point
 XXXXXXXXXX.XXXXXX Broker::log_flush
 XXXXXXXXXX.XXXXXX run_sync_hook
+XXXXXXXXXX.XXXXXX connection_flipped
+                  [0] c: connection      = [id=[orig_h=192.168.133.100, orig_p=49336/tcp, resp_h=74.125.71.189, resp_p=443/tcp], orig=[size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=58:b0:35:86:54:8d], resp=[size=85, state=3, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=cc:b2:55:f4:62:92], start_time=XXXXXXXXXX.XXXXXX, duration=0 secs, service={\x0a\x0a}, history=^d, uid=CP5puj4I8PtEU4qzYg, tunnel=<uninitialized>, vlan=<uninitialized>, inner_vlan=<uninitialized>, dpd=<uninitialized>, dpd_state=<uninitialized>, service_violation={\x0a\x0a}, removal_hooks=<uninitialized>, conn=<uninitialized>, extract_orig=F, extract_resp=F, thresholds=<uninitialized>, dce_rpc=<uninitialized>, dce_rpc_state=<uninitialized>, dce_rpc_backing=<uninitialized>, dhcp=<uninitialized>, dnp3=<uninitialized>, dns=<uninitialized>, dns_state=<uninitialized>, ftp=<uninitialized>, ftp_data_reuse=F, ssl=<uninitialized>, http=<uninitialized>, http_state=<uninitialized>, irc=<uninitialized>, krb=<uninitialized>, modbus=<uninitialized>, mqtt=<uninitialized>, mqtt_state=<uninitialized>, mysql=<uninitialized>, ntlm=<uninitialized>, ntp=<uninitialized>, radius=<uninitialized>, rdp=<uninitialized>, rfb=<uninitialized>, sip=<uninitialized>, sip_state=<uninitialized>, snmp=<uninitialized>, smb_state=<uninitialized>, smtp=<uninitialized>, smtp_state=<uninitialized>, socks=<uninitialized>, ssh=<uninitialized>, syslog=<uninitialized>]
+
 XXXXXXXXXX.XXXXXX new_connection_contents
                   [0] c: connection      = [id=[orig_h=192.168.133.100, orig_p=49336/tcp, resp_h=74.125.71.189, resp_p=443/tcp], orig=[size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=58:b0:35:86:54:8d], resp=[size=85, state=3, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=cc:b2:55:f4:62:92], start_time=XXXXXXXXXX.XXXXXX, duration=0 secs, service={\x0a\x0a}, history=^d, uid=CP5puj4I8PtEU4qzYg, tunnel=<uninitialized>, vlan=<uninitialized>, inner_vlan=<uninitialized>, dpd=<uninitialized>, dpd_state=<uninitialized>, service_violation={\x0a\x0a}, removal_hooks=<uninitialized>, conn=<uninitialized>, extract_orig=F, extract_resp=F, thresholds=<uninitialized>, dce_rpc=<uninitialized>, dce_rpc_state=<uninitialized>, dce_rpc_backing=<uninitialized>, dhcp=<uninitialized>, dnp3=<uninitialized>, dns=<uninitialized>, dns_state=<uninitialized>, ftp=<uninitialized>, ftp_data_reuse=F, ssl=<uninitialized>, http=<uninitialized>, http_state=<uninitialized>, irc=<uninitialized>, krb=<uninitialized>, modbus=<uninitialized>, mqtt=<uninitialized>, mqtt_state=<uninitialized>, mysql=<uninitialized>, ntlm=<uninitialized>, ntp=<uninitialized>, radius=<uninitialized>, rdp=<uninitialized>, rfb=<uninitialized>, sip=<uninitialized>, sip_state=<uninitialized>, snmp=<uninitialized>, smb_state=<uninitialized>, smtp=<uninitialized>, smtp_state=<uninitialized>, socks=<uninitialized>, ssh=<uninitialized>, syslog=<uninitialized>]
 

testing/btest/core/connection_flipped.zeek

@@ -0,0 +1,29 @@
+# @TEST-DOC: A connection flip does not reset the ConnVal. Regression test for #3028.
+
+# @TEST-EXEC: zeek -b -r $TRACES/tcp/handshake-reorder.trace %INPUT >out
+# @TEST-EXEC: TEST_DIFF_CANONIFIER= btest-diff out
+
+redef record conn_id += {
+	extra_id: count &optional;
+};
+
+redef record connection += {
+	my_timestamp: time &optional;
+};
+
+event new_connection(c: connection)
+	{
+	c$id$extra_id = 42;
+	c$my_timestamp = network_time();
+	print network_time(), "new_connection", c$id, c$history, c$my_timestamp;
+	}
+
+event connection_flipped(c: connection)
+	{
+	print network_time(), "connection_flipped", c$id, c$history, c$my_timestamp;
+	}
+
+event connection_state_remove(c: connection)
+	{
+	print network_time(), "connection_state_remove", c$id, c$history, c$my_timestamp;
+	}