Merge remote-tracking branch 'security/topic/awelzel/215-pop3-mail-null-deref'

* security/topic/awelzel/215-pop3-mail-null-deref:
  POP3: Rework unbounded pending command fix

(cherry picked from commit 7fea32c6edc5d4d14646366f87c9208c8c9cf555)

src/analyzer/protocol/pop3/POP3.cc

@@ -227,18 +227,16 @@ void POP3_Analyzer::ProcessRequest(int length, const char* line)
 		cmds.push_back(std::string(line));
 
 		// Prevent unbounded state growth of cmds if there are no matching
-		// server replies by just processing commands even if we didn't see
+		// server replies by simply dropping the oldest command.
-		// the server response.
 		//
-		// This may be caused by packet drops, one-sided traffic, analyzing
+		// This may be caused by packet drops of the server side, one-sided
-		// the wrong protocol (Redis), etc.
+		// traffic, or analyzing the wrong protocol (Redis), etc.
 		if ( zeek::BifConst::POP3::max_pending_commands > 0 )
 			{
 			if ( cmds.size() > zeek::BifConst::POP3::max_pending_commands )
 				{
 				Weird("pop3_client_too_many_pending_commands");
 
-				ProcessClientCmd();
 				cmds.pop_front();
 				}
 			}
@@ -660,10 +658,6 @@ void POP3_Analyzer::ProcessReply(int length, const char* line)
 			Weird("pop3_server_command_unknown", (tokens.size() > 0 ? tokens[0].c_str() : "???"));
 			if ( subState == detail::POP3_WOK )
 				subState = detail::POP3_OK;
-
-			// If we're not in state AUTH and receive "some" response,
-			// assume it was for the last command from the client.
-			FinishClientCmd();
 			}
 
 		return;

testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/analyzer.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	analyzer
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	cause	analyzer_kind	analyzer_name	uid	fuid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	failure_reason	failure_data
+#types	time	string	string	string	string	string	addr	port	addr	port	string	string
+XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	58854	127.0.0.1	110	unknown server command (GARBAGE)	GARBAGE (and LIST response missing .)
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/conn.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
+#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	127.0.0.1	58854	127.0.0.1	110	tcp	-	0.151387	20	253	RSTO	T	T	0	ShAdDaFR	20	1056	16	1093	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/weird.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	weird
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	name	addl	notice	peer	source
+#types	time	string	addr	port	addr	port	string	string	bool	string	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	58854	127.0.0.1	110	pop3_server_command_unknown	GARBAGE	F	zeek	POP3
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log

@@ -8,9 +8,9 @@
 #fields	ts	cause	analyzer_kind	analyzer_name	uid	fuid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	failure_reason	failure_data
 #types	time	string	string	string	string	string	addr	port	addr	port	string	string
 XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	too many unknown client commands	-
-XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	too many unknown client commands	-
+XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	unknown server command (+PONG)	+PONG
-XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	too many unknown client commands	-
+XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	unknown server command (+PONG)	+PONG
-XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	too many unknown client commands	-
+XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	unknown server command (+PONG)	+PONG
-XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	too many unknown client commands	-
+XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	unknown server command (+PONG)	+PONG
-XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	too many unknown client commands	-
+XXXXXXXXXX.XXXXXX	violation	protocol	POP3	CHhAvVGS1DHFjwGM9	-	127.0.0.1	59954	127.0.0.1	6379	unknown server command (+PONG)	+PONG
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.pop3.redis/out

@@ -1,4 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 CHhAvVGS1DHFjwGM9, pop3_reply, F, OK, 
-CHhAvVGS1DHFjwGM9, pop3_request, T, AUTH, 
+CHhAvVGS1DHFjwGM9, pop3_reply, F, OK, 
 CHhAvVGS1DHFjwGM9, pop3_reply, F, OK, 

testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log

@@ -9,4 +9,5 @@
 #types	time	string	addr	port	addr	port	string	string	bool	string	string
 XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	59954	127.0.0.1	6379	pop3_client_command_unknown	*2	F	zeek	POP3
 XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	59954	127.0.0.1	6379	pop3_client_too_many_pending_commands	-	F	zeek	POP3
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	59954	127.0.0.1	6379	pop3_server_command_unknown	+PONG	F	zeek	POP3
 #close XXXX-XX-XX-XX-XX-XX

testing/btest/scripts/base/protocols/pop3/bad-list-retr-crafted.zeek

@@ -0,0 +1,14 @@
+# @TEST-DOC: Crafted pcap causing crashes due to mail not initialized.
+# @TEST-EXEC: zeek -b -r $TRACES/pop3/bad-list-retr-crafted.pcap %INPUT
+# @TEST-EXEC: btest-diff conn.log
+# @TEST-EXEC: btest-diff weird.log
+# @TEST-EXEC: btest-diff analyzer.log
+
+@load base/frameworks/notice/weird
+@load base/protocols/conn
+@load base/protocols/pop3
+
+event zeek_init()
+	{
+	Analyzer::register_for_port(Analyzer::ANALYZER_POP3, 110/tcp);
+	}

testing/btest/scripts/base/protocols/pop3/redis.zeek

@@ -9,6 +9,8 @@
 @load base/protocols/conn
 @load base/protocols/pop3
 
+redef POP3::max_unknown_client_commands = 3;
+
 event pop3_request(c: connection, is_orig: bool, cmd: string, arg: string)
 	{
 	print c$uid, "pop3_request", is_orig, cmd, arg;