diff --git a/CHANGES b/CHANGES index fd71ce976c..68b219beae 100644 --- a/CHANGES +++ b/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 diff --git a/VERSION b/VERSION index 3e671bf2d5..d68991bf7d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -5.2.0-dev.593 +5.2.0-dev.597 diff --git a/src/analyzer/protocol/ftp/FTP.cc b/src/analyzer/protocol/ftp/FTP.cc index bfdd42da34..f0dcdeb5a4 100644 --- a/src/analyzer/protocol/ftp/FTP.cc +++ b/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(""); + if ( AnalyzerConfirmed() ) + Weird("FTP_invalid_command"); + + // Ignore the whole line. + return; } else if ( BifConst::FTP::max_command_length > 0 && static_cast(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; } } diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/conn.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/conn.log new file mode 100644 index 0000000000..dd5edb3ab7 --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/dpd.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/dpd.log new file mode 100644 index 0000000000..4efd80fa0f --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/ftp.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-invalid-reply-code/ftp.log new file mode 100644 index 0000000000..cd149015f5 --- /dev/null +++ b/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 PASS zeek - - 230 PASS OK - - - - - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 51354 127.0.0.1 21 zeek PASV - - - 230 PASS OK - - - - - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/conn.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/conn.log new file mode 100644 index 0000000000..f8ad05cb2d --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/dpd.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/dpd.log new file mode 100644 index 0000000000..f0301d0f7d --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/ftp.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-reply-code/ftp.log new file mode 100644 index 0000000000..f14e2c92ab --- /dev/null +++ b/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 PASS zeek - - 230 PASS OK - - - - - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 51344 127.0.0.1 21 zeek SYST - - - 230 PASS OK - - - - - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/conn.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/conn.log new file mode 100644 index 0000000000..b3cbeecb64 --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/dpd.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/dpd.log new file mode 100644 index 0000000000..00876f2723 --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/ftp.log b/testing/btest/Baseline/scripts.base.protocols.ftp.ftp-missing-space-after-reply-code/ftp.log new file mode 100644 index 0000000000..bcc46520f9 --- /dev/null +++ b/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 PASS zeek - - 230 PASS OK - - - - - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 51346 127.0.0.1 21 zeek RETR ftp://127.0.0.1/. - - 230 PASS OK - - - - - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/ftp/ftp-invalid-reply-code.pcap b/testing/btest/Traces/ftp/ftp-invalid-reply-code.pcap new file mode 100644 index 0000000000..09de02ed0c Binary files /dev/null and b/testing/btest/Traces/ftp/ftp-invalid-reply-code.pcap differ diff --git a/testing/btest/Traces/ftp/ftp-missing-reply-code.pcap b/testing/btest/Traces/ftp/ftp-missing-reply-code.pcap new file mode 100644 index 0000000000..3f32d6aa73 Binary files /dev/null and b/testing/btest/Traces/ftp/ftp-missing-reply-code.pcap differ diff --git a/testing/btest/Traces/ftp/ftp-missing-space-after-reply-code.pcap b/testing/btest/Traces/ftp/ftp-missing-space-after-reply-code.pcap new file mode 100644 index 0000000000..6c1f7c5bfc Binary files /dev/null and b/testing/btest/Traces/ftp/ftp-missing-space-after-reply-code.pcap differ diff --git a/testing/btest/scripts/base/protocols/ftp/ftp-invalid-reply-code.zeek b/testing/btest/scripts/base/protocols/ftp/ftp-invalid-reply-code.zeek new file mode 100644 index 0000000000..6f154784d5 --- /dev/null +++ b/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" }; diff --git a/testing/btest/scripts/base/protocols/ftp/ftp-missing-reply-code.zeek b/testing/btest/scripts/base/protocols/ftp/ftp-missing-reply-code.zeek new file mode 100644 index 0000000000..5459984174 --- /dev/null +++ b/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" }; diff --git a/testing/btest/scripts/base/protocols/ftp/ftp-missing-space-after-reply-code.zeek b/testing/btest/scripts/base/protocols/ftp/ftp-missing-space-after-reply-code.zeek new file mode 100644 index 0000000000..ca0f2b78dd --- /dev/null +++ b/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" };