Merge remote-tracking branch 'security/topic/awelzel/125-ftp-timeout-three'

* security/topic/awelzel/125-ftp-timeout-three:
  testing/ftp: Add tests and pcaps with invalid reply lines
  ftp: Harden reply handing a bit and don't raise bad replies to script-land
  ftp: ignore invalid commands

CHANGES

@@ -1,3 +1,26 @@
+5.2.0-dev.597 | 2023-02-01 10:47:21 -0700
+
+  * testing/ftp: Add tests and pcaps with invalid reply lines (Arne Welzel, Corelight)
+
+    These have been created artificially. The tests show that for an
+    invalid reply line without a numeric code, with a numeric code < 100
+    or a numeric code not followed by a space we now raise an analyzer
+    violation and disable the analyzer.
+
+  * ftp: Harden reply handing a bit and don't raise bad replies to script-land (Arne Welzel, Corelight)
+
+    This improves runtime of the oss-fuzz generated traffic in #125.
+    Specifically, that reproducers included a 064- reply code that was
+    interpreted as needing to be continued.
+
+    Also, return after AnalyzerViolations() for server replies rather than
+    propagating bad replies them to script-land. This trusts server's to
+    generally behave according to specification.
+
+  * ftp: ignore invalid commands (Arne Welzel, Corelight)
+
+    Do not propagate wrong FTP commands to script land.
+
 5.2.0-dev.593 | 2023-02-01 11:39:15 +0100
 
   * Provide infrastructure to migrate legacy analyzers to in-tree

VERSION

@@ -1 +1 @@
-5.2.0-dev.593
+5.2.0-dev.597

src/analyzer/protocol/ftp/FTP.cc

@@ -63,6 +63,20 @@ static uint32_t get_reply_code(int len, const char* line)
 		return 0;
 	}
 
+// The minimal length of an FTP command is 3 characters (PWD, MKD,
+// RMD, ...) and should only contain printable ascii.
+static bool is_ftp_cmd(int len, const char* s)
+	{
+	if ( len < 3 )
+		return false;
+
+	for ( int i = 0; i < len; i++ )
+		if ( ! isprint(s[i]) || isspace(s[i]) )
+			return false;
+
+	return true;
+	}
+
 void FTP_Analyzer::DeliverStream(int length, const u_char* data, bool orig)
 	{
 	analyzer::tcp::TCP_ApplicationAnalyzer::DeliverStream(length, data, orig);
@@ -91,10 +105,13 @@ void FTP_Analyzer::DeliverStream(int length, const u_char* data, bool orig)
 		util::get_word(end_of_line - line, line, cmd_len, cmd);
 		line = util::skip_whitespace(line + cmd_len, end_of_line);
 
-		if ( cmd_len == 0 )
+		if ( ! is_ftp_cmd(cmd_len, cmd) )
 			{
-			// Weird("FTP command missing", end_of_line - orig_line, orig_line);
-			cmd_str = new StringVal("<missing>");
+			if ( AnalyzerConfirmed() )
+				Weird("FTP_invalid_command");
+
+			// Ignore the whole line.
+			return;
 			}
 		else if ( BifConst::FTP::max_command_length > 0 &&
 		          static_cast<zeek_uint_t>(cmd_len) > BifConst::FTP::max_command_length )
@@ -149,25 +166,39 @@ void FTP_Analyzer::DeliverStream(int length, const u_char* data, bool orig)
 			}
 		else
 			{ // a new reply
-			if ( reply_code > 0 && length > 3 && line[3] == '-' )
+			cont_resp = 0;
+
+			if ( reply_code == 0 )
+				{
+				AnalyzerViolation("non-numeric reply code", (const char*)data, length);
+				return;
+				}
+			else if ( reply_code < 100 )
+				{
+				AnalyzerViolation("invalid reply code", (const char*)data, length);
+				return;
+				}
+			else if ( length > 3 && line[3] == '-' )
 				{ // a continued reply
 				pending_reply = reply_code;
 				line = util::skip_whitespace(line + 4, end_of_line);
 				cont_resp = 1;
 				}
+			else if ( length > 3 && line[3] != ' ' )
+				{
+				// This is a proper reply code, but there's no space after
+				// the reply code even though the line is long enough.
+				AnalyzerViolation("invalid reply line", (const char*)data, length);
+				return;
+				}
 			else
 				{ // a self-contained reply
-				if ( reply_code > 0 )
-					line += 3;
-				else
-					AnalyzerViolation("non-numeric reply code", (const char*)data, length);
+				line += 3;
 
 				if ( line < end_of_line )
 					line = util::skip_whitespace(line, end_of_line);
 				else
 					line = end_of_line;
-
-				cont_resp = 0;
 				}
 			}
 

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/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	51354	127.0.0.1	21	tcp	-	9.891089	34	71	SF	-	-	0	ShAdDaFf	13	718	10	599	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/dpd.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	dpd
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	analyzer	failure_reason
+#types	time	string	addr	port	addr	port	enum	string	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	tcp	FTP	non-numeric reply code [99 PASV invalid]
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/ftp.log

@@ -0,0 +1,13 @@
+### 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	ftp
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	user	password	command	arg	mime_type	file_size	reply_code	reply_msg	data_channel.passive	data_channel.orig_h	data_channel.resp_h	data_channel.resp_p	fuid
+#types	time	string	addr	port	addr	port	string	string	string	string	string	count	count	string	bool	addr	addr	port	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	zeek	-	USER	zeek	-	-	230	USER OK	-	-	-	-	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	zeek	<hidden>	PASS	zeek	-	-	230	PASS OK	-	-	-	-	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51354	127.0.0.1	21	zeek	<hidden>	PASV	-	-	-	230	PASS OK	-	-	-	-	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/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	51344	127.0.0.1	21	tcp	-	10.862185	34	74	SF	-	-	0	ShAdDaFf	13	718	10	602	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/dpd.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	dpd
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	analyzer	failure_reason
+#types	time	string	addr	port	addr	port	enum	string	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51344	127.0.0.1	21	tcp	FTP	non-numeric reply code [SYST not supported]
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/ftp.log

@@ -0,0 +1,13 @@
+### 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	ftp
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	user	password	command	arg	mime_type	file_size	reply_code	reply_msg	data_channel.passive	data_channel.orig_h	data_channel.resp_h	data_channel.resp_p	fuid
+#types	time	string	addr	port	addr	port	string	string	string	string	string	count	count	string	bool	addr	addr	port	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51344	127.0.0.1	21	zeek	-	USER	zeek	-	-	230	USER OK	-	-	-	-	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51344	127.0.0.1	21	zeek	<hidden>	PASS	zeek	-	-	230	PASS OK	-	-	-	-	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51344	127.0.0.1	21	zeek	<hidden>	SYST	-	-	-	230	PASS OK	-	-	-	-	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/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	51346	127.0.0.1	21	tcp	-	11.705309	34	68	SF	-	-	0	ShAdDaFf	13	718	10	596	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/dpd.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	dpd
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	analyzer	failure_reason
+#types	time	string	addr	port	addr	port	enum	string	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51346	127.0.0.1	21	tcp	FTP	invalid reply line [230_no_space]
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/ftp.log

@@ -0,0 +1,13 @@
+### 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	ftp
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	user	password	command	arg	mime_type	file_size	reply_code	reply_msg	data_channel.passive	data_channel.orig_h	data_channel.resp_h	data_channel.resp_p	fuid
+#types	time	string	addr	port	addr	port	string	string	string	string	string	count	count	string	bool	addr	addr	port	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51346	127.0.0.1	21	zeek	-	USER	zeek	-	-	230	USER OK	-	-	-	-	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51346	127.0.0.1	21	zeek	<hidden>	PASS	zeek	-	-	230	PASS OK	-	-	-	-	-
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	127.0.0.1	51346	127.0.0.1	21	zeek	<hidden>	RETR	ftp://127.0.0.1/.	-	-	230	PASS OK	-	-	-	-	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/scripts/base/protocols/ftp/ftp-invalid-reply-code.zeek

@@ -0,0 +1,11 @@
+# @TEST-DOC: Th server replies with a line that does not contain a numeric code.: violation.
+# @TEST-EXEC: zeek -b -r $TRACES/ftp/ftp-invalid-reply-code.pcap %INPUT
+# @TEST-EXEC: btest-diff conn.log
+# @TEST-EXEC: btest-diff ftp.log
+# @TEST-EXEC: btest-diff dpd.log
+# @TEST-EXEC: test ! -f reporter.log
+
+@load base/protocols/conn
+@load base/protocols/ftp
+
+redef FTP::logged_commands += { "USER", "PASS", "SYST", "QUIT" };

testing/btest/scripts/base/protocols/ftp/ftp-missing-reply-code.zeek

@@ -0,0 +1,11 @@
+# @TEST-DOC: Th server replies with a line that does not contain a numeric code.: violation.
+# @TEST-EXEC: zeek -b -r $TRACES/ftp/ftp-missing-reply-code.pcap %INPUT
+# @TEST-EXEC: btest-diff conn.log
+# @TEST-EXEC: btest-diff ftp.log
+# @TEST-EXEC: btest-diff dpd.log
+# @TEST-EXEC: test ! -f reporter.log
+
+@load base/protocols/conn
+@load base/protocols/ftp
+
+redef FTP::logged_commands += { "USER", "PASS", "SYST", "QUIT" };

testing/btest/scripts/base/protocols/ftp/ftp-missing-space-after-reply-code.zeek

@@ -0,0 +1,11 @@
+# @TEST-DOC: Th server replies with a line that does not contain a numeric code.: violation.
+# @TEST-EXEC: zeek -b -r $TRACES/ftp/ftp-missing-space-after-reply-code.pcap %INPUT
+# @TEST-EXEC: btest-diff conn.log
+# @TEST-EXEC: btest-diff ftp.log
+# @TEST-EXEC: btest-diff dpd.log
+# @TEST-EXEC: test ! -f reporter.log
+
+@load base/protocols/conn
+@load base/protocols/ftp
+
+redef FTP::logged_commands += { "USER", "PASS", "SYST", "QUIT" };