Merge branch 'topic/awelzel/3235-dont-flip-broadcasts'

* topic/awelzel/3235-dont-flip-broadcasts:
  testing: Bump external test suite
  dhcp: Handle is_orig=T for connections from server to 255.255.255.255
  IPBasedAnalyzer: Don't flip connections when destination is broadcast

CHANGES

@@ -1,3 +1,15 @@
+6.1.0-dev.336 | 2023-08-28 12:19:45 +0200
+
+  * testing: Bump external test suite (Arne Welzel, Corelight)
+
+  * dhcp: Handle is_orig=T for connections from server to 255.255.255.255 (Arne Welzel, Corelight)
+
+    This works around the new semantics of is_orig=T for "connections"
+    from DHCP servers to broadcast addresses. IMO, having the server address
+    as originator in the conn.log is still more intuitive.
+
+  * GH-3235: IPBasedAnalyzer: Don't flip connections when destination is broadcast (Arne Welzel, Corelight)
+
 6.1.0-dev.331 | 2023-08-25 21:38:40 +0200
 
   * BTests for any/vector-of-any fixes (Vern Paxson, Corelight)

NEWS

@@ -78,6 +78,10 @@ Changed Functionality
   ``policy/protocols/conn/community-id-logging.zeek`` was loaded before. This
   was fairly unusual and hard to debug behavior.
 
+- Connections to broadcast addresses are not flipped based on ``likely_server_ports``
+  anymore. Previously, broadcast packets originating from a likely server port
+  resulted in 255.255.255.255 being the originator in ``conn.log``.
+
 Removed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-6.1.0-dev.331
+6.1.0-dev.336

scripts/base/protocols/dhcp/main.zeek

@@ -204,11 +204,16 @@ event DHCP::aggregate_msgs(ts: time, id: conn_id, uid: string, is_orig: bool, ms
 
 	log_info$msg_types += DHCP::message_types[msg$m_type];
 
+	# The is_orig flag is T for "connections" initiated by servers
+	# to broadcast addresses, otherwise is_orig indicates that this
+	# is a DHCP client.
+	local is_client = is_orig && (id$orig_h == 0.0.0.0 || id$orig_p == 68/udp || id$resp_p == 67/udp);
+
 	# Let's watch for messages in any DHCP message type
 	# and split them out based on client and server.
 	if ( options?$message )
 		{
-		if ( is_orig )
+		if ( is_client )
 			log_info$client_message = options$message;
 		else
 			log_info$server_message = options$message;
@@ -218,7 +223,7 @@ event DHCP::aggregate_msgs(ts: time, id: conn_id, uid: string, is_orig: bool, ms
 	# expiration handling.
 	log_info$last_message_ts = ts;
 
-	if ( is_orig ) # client requests
+	if ( is_client ) # client requests
 		{
 		# Assign the client addr in case this is a session
 		# of only INFORM messages (no lease handed out).
@@ -246,13 +251,28 @@ event DHCP::aggregate_msgs(ts: time, id: conn_id, uid: string, is_orig: bool, ms
 		{
 		# Only log the address of the server if it handed out
 		# an IP address.
-		if ( msg$yiaddr != 0.0.0.0 &&
-		     id$resp_h != 255.255.255.255 )
+		if ( msg$yiaddr != 0.0.0.0 )
 			{
+			if ( is_orig )
+				{
+				# This is a server message and is_orig is T.
+				# This means it's a DHCP server broadcasting
+				# and the server is the originator.
+				log_info$server_addr = id$orig_h;
+				log_info$server_port = id$orig_p;
+				log_info$client_port = id$resp_p;
+				}
+			else
+				{
+				# When a server sends to a non-broadcast
+				# address, Zeek's connection flipping is
+				# in effect and the server is the responder
+				# instead.
 				log_info$server_addr = id$resp_h;
 				log_info$server_port = id$resp_p;
 				log_info$client_port = id$orig_p;
 				}
+			}
 
 		# Only use the client hardware address from the server
 		# if we didn't already pick one up from the client.

src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc

@@ -171,7 +171,7 @@ zeek::Connection* IPBasedAnalyzer::NewConn(const ConnTuple* id, const detail::Co
 	                                  pkt->ip_hdr->FlowLabel(), pkt);
 	conn->SetTransport(transport);
 
-	if ( flip )
+	if ( flip && ! id->dst_addr.IsBroadcast() )
 		conn->FlipRoles();
 
 	BuildSessionAnalyzerTree(conn);

testing/btest/Baseline/core.udp-broadcast-no-flip/conn.log.cut

@@ -0,0 +1,3 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	history
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	192.168.1.1	40190	255.255.255.255	7437	D

testing/btest/Baseline/scripts.base.protocols.dhcp.dhcp-all-msg-types/conn.log

@@ -0,0 +1,14 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+#separator \x09
+#set_separator	,
+#empty_field	(empty)
+#unset_field	-
+#path	conn
+#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	0.0.0.0	68	255.255.255.255	67	udp	dhcp	5.099034	1560	0	S0	T	T	0	D	6	1728	0	0	-
+XXXXXXXXXX.XXXXXX	ClEkJM2Vm5giqnMf4h	128.2.6.97	68	128.2.6.152	67	udp	dhcp	-	-	-	SHR	F	F	0	^d	0	0	1	395	-
+XXXXXXXXXX.XXXXXX	CtPZjS20MLrsMUOJi2	128.2.6.189	68	128.2.6.152	67	udp	dhcp	-	-	-	SHR	F	F	0	^d	0	0	1	395	-
+XXXXXXXXXX.XXXXXX	C4J4Th3PJpwUYZZ6gc	128.2.6.152	67	255.255.255.255	68	udp	dhcp	-	-	-	S0	F	T	0	D	1	328	0	0	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/core/udp-broadcast-no-flip.zeek

@@ -0,0 +1,9 @@
+# @TEST-DOC: Pcap contains broadcast with port 40190 to port 7437. Set likely_server_ports to 40190 but don't expect this connection to be flipped.
+
+# @TEST-EXEC: zeek -b -r $TRACES/udp-broadcast.pcap %INPUT
+# @TEST-EXEC: zeek-cut -m ts uid id.orig_h id.orig_p id.resp_h id.resp_p history < conn.log > conn.log.cut
+# @TEST-EXEC: btest-diff conn.log.cut
+
+@load base/protocols/conn
+
+redef likely_server_ports += { 40190/udp };

testing/btest/scripts/base/protocols/dhcp/dhcp-all-msg-types.zeek

@@ -3,6 +3,8 @@
 # but only one lease should show up in the logs.
 
 # @TEST-EXEC: zeek -b -r $TRACES/dhcp/dhcp.trace %INPUT
+# @TEST-EXEC: btest-diff conn.log
 # @TEST-EXEC: btest-diff dhcp.log
 
+@load base/protocols/conn
 @load base/protocols/dhcp

testing/external/commit-hash.zeek-testing

@@ -1 +1 @@
-5218e6b23477a8fc21c17655c9d955fb80c7de4a
+cfc57a8a0513d8651c8946739e13ee0d8cfaad52