Merge remote-tracking branch 'origin/topic/johanna/gh-4251-discussion'

* origin/topic/johanna/gh-4251-discussion:
  Add two protocol mismatch testcases
  Remove violating t panalyzer from services field again

CHANGES

@@ -1,3 +1,12 @@
+7.2.0-dev.269 | 2025-03-06 13:12:31 +0000
+
+  * Remove violating analyzer from services field again (Johanna Amann, Corelight)
+
+    This reverts some of the recent DPD changes; specifically violations
+    trigger removal from the services field, again, by default.
+
+    Discussion in GH-4521
+
 7.2.0-dev.265 | 2025-03-05 19:23:53 +0100
 
   * ZAM: bif-tracking updates for blocking_lookup_hostname() (Arne Welzel, Corelight)

NEWS

@@ -48,22 +48,14 @@ Changed Functionality
   It will be retained till Zeek 8.1 to prevent script errors, and raises a
   deprecation warning.
 
-  The way failed services interact with the ``service`` field in the connection
-  log also changed. In the past, protocol analyzers that were confirmed and
-  later failed were removed from the ``service`` field in some cases. This
-  commonly lead to the case that a protocol log exists, while the service is
-  not listed in the connection.log - so, e.g., an etry in ``http.log`` existing
-  without ``http`` showing up in the connection log.
-
-  Now, protocol analyzers that raised a confirmation event will always be added to
-  the ``service`` field in the connection log, and the entry will be retained
-  even if the analyzer raises a violation later.
-
   To extend the visibility of protocol violations, a new option
   ``DPD::track_removed_services_in_connection`` was added. Enabling it causes
-  failed analyzers to be logged to the ``service`` field of the connection log,
+  failed analyzers to no longer be removed from the ``service`` field of the
-  with a prepended "-". So a connection that attached the ``ssl`` analyzer
+  connection log. Instead, analyzers are never removed after they are
-  which later failed due to a protocol error will be logged as ``ssl,-ssl``.
+  confirmed.  Instead, failed analyzers are logged by additionally adding an
+  entry with a prepended "-". So a connection that attached the ``ssl``
+  analyzer which later failed due to a protocol error will be logged as
+  ``ssl,-ssl``.
 
   This change also adds a new policy script,
   ``protocols/conn/failed-service-logging.zeek``. Loading this script adds the

VERSION

@@ -1 +1 @@
-7.2.0-dev.265
+7.2.0-dev.269

scripts/base/frameworks/analyzer/dpd.zeek

@@ -36,7 +36,11 @@ export {
 	## Set to 0 to never ignore protocol violations.
 	option ignore_violations_after = 10 * 1024;
 
-	## Add removed services to conn.log, with a - in front of them.
+	## Change behavior of service field in conn.log:
+	## Failed services are no longer removed. Instead, for a failed
+	## service, a second entry with a "-" in front of it is added.
+	## E.g. a http connection with a violation would be logged as
+	## "http,-http".
 	option track_removed_services_in_connection = F;
 }
 
@@ -77,7 +81,16 @@ event analyzer_violation_info(atype: AllAnalyzers::Tag, info: AnalyzerViolationI
 	local analyzer = Analyzer::name(atype);
 	# If the service hasn't been confirmed yet, or already failed,
 	# don't generate a log message for the protocol violation.
-	if ( analyzer !in c$service || analyzer in c$service_violation )
+	if ( analyzer !in c$service )
+		return;
+
+	# If removed service tracking is active, don't delete the service here.
+	if ( ! track_removed_services_in_connection )
+		delete c$service[analyzer];
+
+	# if statement is separate, to allow repeated removal of service, in case there are several
+	# confirmation and violation events
+	if ( analyzer in c$service_violation )
 		return;
 
 	add c$service_violation[analyzer];

testing/btest/Baseline/core.pppoe-over-qinq/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	ip_proto
 #types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	1.1.1.1	20394	2.2.2.2	443	tcp	ssl	273.626833	11352	4984	SF	F	F	0	ShADdtaTTtFf	44	25283	42	13001	-	6
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	1.1.1.1	20394	2.2.2.2	443	tcp	-	273.626833	11352	4984	SF	F	F	0	ShADdtaTTtFf	44	25283	42	13001	-	6
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.frameworks.analyzer.request-response-protocol-differences/conn-http-to-ssh.log

@@ -0,0 +1,11 @@
+### 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	ip_proto
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	40896	127.0.0.1	22	tcp	-	0.001845	75	78	RSTR	T	T	0	ShADadFr	7	447	6	398	-	6
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.frameworks.analyzer.request-response-protocol-differences/conn-https-to-http.log

@@ -0,0 +1,11 @@
+### 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	ip_proto
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	50382	127.0.0.1	80	tcp	-	0.028273	517	468	RSTO	T	T	0	ShADadfR	5	785	4	684	-	6
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/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	ip_proto
 #types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	tcp	ftp	9.891089	34	71	SF	T	T	0	ShAdDaFf	13	718	10	599	-	6
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	tcp	-	9.891089	34	71	SF	T	T	0	ShAdDaFf	13	718	10	599	-	6
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/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	ip_proto
 #types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51344	127.0.0.1	21	tcp	ftp	10.862185	34	74	SF	T	T	0	ShAdDaFf	13	718	10	602	-	6
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51344	127.0.0.1	21	tcp	-	10.862185	34	74	SF	T	T	0	ShAdDaFf	13	718	10	602	-	6
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/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	ip_proto
 #types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51346	127.0.0.1	21	tcp	ftp	11.705309	34	68	SF	T	T	0	ShAdDaFf	13	718	10	596	-	6
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51346	127.0.0.1	21	tcp	-	11.705309	34	68	SF	T	T	0	ShAdDaFf	13	718	10	596	-	6
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.modbus.events/conn-first.log

@@ -14,6 +14,6 @@ XXXXXXXXXX.XXXXXX	CtPZjS20MLrsMUOJi2	10.0.0.9	3082	10.0.0.3	502	tcp	modbus	177.0
 XXXXXXXXXX.XXXXXX	CUM0KZ3MLUfNB0cl11	10.0.0.57	2585	10.0.0.8	502	tcp	-	76.561880	926	0	SF	T	T	0	ShADafF	8	1254	7	288	-	6
 XXXXXXXXXX.XXXXXX	CmES5u32sYpV7JYN	10.0.0.8	502	10.0.0.57	4446	tcp	-	155.114237	128	0	SF	T	T	0	ShADaFf	16	776	15	608	-	6
 XXXXXXXXXX.XXXXXX	CP5puj4I8PtEU4qzYg	192.168.66.235	2582	166.161.16.230	502	tcp	-	2.905078	0	0	S0	T	F	0	S	2	96	0	0	-	6
-XXXXXXXXXX.XXXXXX	C37jN32gN3y3AZzyf6	192.168.66.235	2582	166.161.16.230	502	tcp	modbus	85.560847	1692	1278	S1	T	F	0	ShADad	167	8380	181	8522	-	6
+XXXXXXXXXX.XXXXXX	C37jN32gN3y3AZzyf6	192.168.66.235	2582	166.161.16.230	502	tcp	-	85.560847	1692	1278	S1	T	F	0	ShADad	167	8380	181	8522	-	6
 XXXXXXXXXX.XXXXXX	C3eiCBGOLw3VtHfOj	10.1.1.234	51411	10.10.5.85	502	tcp	modbus	2100.811351	237936	4121200	S2	T	T	0	ShADdaF	39659	2300216	20100	5166412	-	6
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.postgresql.bad-backend-message/conn.cut

@@ -1,3 +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	service
-XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	54958	127.0.0.1	5432	postgresql
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	54958	127.0.0.1	5432	-

testing/btest/scripts/base/frameworks/analyzer/request-response-protocol-differences.zeek

@@ -0,0 +1,10 @@
+# This test checks behavior when the originator and responder protocols
+# differ - e.g. when a HTTPS client connects to a HTTP port.
+#
+# @TEST-EXEC: zeek -r ${TRACES}/http/http-to-ssh.pcap
+# @TEST-EXEC: mv conn.log conn-http-to-ssh.log
+# @TEST-EXEC: zeek -r ${TRACES}/tls/https-to-http.pcap
+# @TEST-EXEC: mv conn.log conn-https-to-http.log
+# @TEST-EXEC: btest-diff conn-http-to-ssh.log
+# @TEST-EXEC: btest-diff conn-https-to-http.log
+