diff --git a/NEWS b/NEWS index d9e7e2cd0d..b053a97b9d 100644 --- a/NEWS +++ b/NEWS @@ -74,6 +74,11 @@ Changed Functionality * The ``mysql.log`` for user change commands will contain *just* the username instead of the remaining parts of the command, including auth plugin data. +* The POP3 parser has been hardened to avoid unbounded state growth in the + face of one-sided traffic capture or when enabled for non-POP3 traffic. + Concretely, the Redis protocol's AUTH mechanism enables the POP3 analyzer + for such connections through DPD. + Removed Functionality --------------------- diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek index 1da63d14b1..f01c4496aa 100644 --- a/scripts/base/init-bare.zeek +++ b/scripts/base/init-bare.zeek @@ -2903,6 +2903,22 @@ export { } # end export +module POP3; + +export { + ## How many commands a POP3 client may have pending + ## before Zeek forcefully removes the oldest. + ## + ## Setting this value to 0 removes the limit. + const max_pending_commands = 10 &redef; + + ## How many invalid commands a POP3 client may use + ## before Zeek starts raising analyzer violations. + ## + ## Setting this value to 0 removes the limit. + const max_unknown_client_commands = 10 &redef; + +} # end export module Threading; diff --git a/src/analyzer/protocol/pop3/CMakeLists.txt b/src/analyzer/protocol/pop3/CMakeLists.txt index f5283b17e4..11e0e43439 100644 --- a/src/analyzer/protocol/pop3/CMakeLists.txt +++ b/src/analyzer/protocol/pop3/CMakeLists.txt @@ -5,4 +5,5 @@ zeek_add_plugin( POP3.cc Plugin.cc BIFS + consts.bif events.bif) diff --git a/src/analyzer/protocol/pop3/POP3.cc b/src/analyzer/protocol/pop3/POP3.cc index ed3a8f2aa1..c20574dcf5 100644 --- a/src/analyzer/protocol/pop3/POP3.cc +++ b/src/analyzer/protocol/pop3/POP3.cc @@ -12,6 +12,7 @@ #include "zeek/Base64.h" #include "zeek/Reporter.h" #include "zeek/analyzer/Manager.h" +#include "zeek/analyzer/protocol/pop3/consts.bif.h" #include "zeek/analyzer/protocol/pop3/events.bif.h" namespace zeek::analyzer::pop3 { @@ -41,6 +42,7 @@ POP3_Analyzer::POP3_Analyzer(Connection* conn) : analyzer::tcp::TCP_ApplicationA authLines = 0; mail = nullptr; + unknown_client_cmds = 0; cl_orig = new analyzer::tcp::ContentLine_Analyzer(conn, true); AddSupportAnalyzer(cl_orig); @@ -205,6 +207,21 @@ void POP3_Analyzer::ProcessRequest(int length, const char* line) { // keep a list of pending commands. cmds.emplace_back(line); + // Prevent unbounded state growth of cmds if there are no matching + // server replies by just processing commands even if we didn't see + // the server response. + // + // This may be caused by packet drops, one-sided traffic, 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(); + } + } + if ( cmds.size() == 1 ) // Not waiting for another server response, // so we can process it immediately. @@ -236,10 +253,19 @@ void POP3_Analyzer::ProcessClientCmd() { if ( cmd_code == -1 ) { if ( ! waitingForAuthentication ) { - Weird("pop3_client_command_unknown"); + Weird("pop3_client_command_unknown", (tokens.size() > 0 ? tokens[0].c_str() : "???")); if ( subState == detail::POP3_WOK ) subState = detail::POP3_OK; + + ++unknown_client_cmds; + + if ( zeek::BifConst::POP3::max_unknown_client_commands > 0 ) { + if ( unknown_client_cmds > zeek::BifConst::POP3::max_unknown_client_commands ) { + AnalyzerViolation("too many unknown client commands"); + } + } } + return; } @@ -299,6 +325,7 @@ void POP3_Analyzer::ProcessClientCmd() { if ( masterState == detail::POP3_AUTHORIZATION ) { POP3Event(pop3_request, true, cmd, message); if ( ! *message ) { + // This is the client requesting a list of AUTH mechanisms available. requestForMultiLine = true; state = detail::AUTH; subState = detail::POP3_WOK; @@ -555,9 +582,13 @@ void POP3_Analyzer::ProcessReply(int length, const char* line) { AnalyzerViolation(util::fmt("unknown server command (%s)", (tokens.size() > 0 ? tokens[0].c_str() : "???")), line, length); - Weird("pop3_server_command_unknown"); + 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; } diff --git a/src/analyzer/protocol/pop3/POP3.h b/src/analyzer/protocol/pop3/POP3.h index 629387d402..45f5fb53ab 100644 --- a/src/analyzer/protocol/pop3/POP3.h +++ b/src/analyzer/protocol/pop3/POP3.h @@ -106,6 +106,7 @@ protected: analyzer::mime::MIME_Mail* mail; std::list cmds; + zeek_uint_t unknown_client_cmds; private: bool tls; diff --git a/src/analyzer/protocol/pop3/consts.bif b/src/analyzer/protocol/pop3/consts.bif new file mode 100644 index 0000000000..29fcd14e3d --- /dev/null +++ b/src/analyzer/protocol/pop3/consts.bif @@ -0,0 +1,2 @@ +const POP3::max_pending_commands: count; +const POP3::max_unknown_client_commands: count; diff --git a/testing/btest/Baseline/core.max-analyzer-violations/weird.log b/testing/btest/Baseline/core.max-analyzer-violations/weird.log index 99b5e14621..8d0cb822e6 100644 --- a/testing/btest/Baseline/core.max-analyzer-violations/weird.log +++ b/testing/btest/Baseline/core.max-analyzer-violations/weird.log @@ -7,7 +7,7 @@ #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 58246 127.0.0.1 110 pop3_server_command_unknown - F zeek POP3 +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 58246 127.0.0.1 110 pop3_server_command_unknown + F zeek POP3 XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 58246 127.0.0.1 110 line_terminated_with_single_CR - F zeek CONTENTLINE XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 58246 127.0.0.1 110 too_many_analyzer_violations - F zeek POP3 #close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/coverage.bare-load-baseline/canonified_loaded_scripts.log b/testing/btest/Baseline/coverage.bare-load-baseline/canonified_loaded_scripts.log index 98ee7f266d..8bbbcdb3d6 100644 --- a/testing/btest/Baseline/coverage.bare-load-baseline/canonified_loaded_scripts.log +++ b/testing/btest/Baseline/coverage.bare-load-baseline/canonified_loaded_scripts.log @@ -195,6 +195,7 @@ scripts/base/init-frameworks-and-bifs.zeek build/scripts/base/bif/plugins/Zeek_NTLM.events.bif.zeek build/scripts/base/bif/plugins/Zeek_NTP.types.bif.zeek build/scripts/base/bif/plugins/Zeek_NTP.events.bif.zeek + build/scripts/base/bif/plugins/Zeek_POP3.consts.bif.zeek build/scripts/base/bif/plugins/Zeek_POP3.events.bif.zeek build/scripts/base/bif/plugins/Zeek_RADIUS.events.bif.zeek build/scripts/base/bif/plugins/Zeek_RDP.events.bif.zeek diff --git a/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log b/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log index 576ada097d..247696f3a7 100644 --- a/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log +++ b/testing/btest/Baseline/coverage.default-load-baseline/canonified_loaded_scripts.log @@ -195,6 +195,7 @@ scripts/base/init-frameworks-and-bifs.zeek build/scripts/base/bif/plugins/Zeek_NTLM.events.bif.zeek build/scripts/base/bif/plugins/Zeek_NTP.types.bif.zeek build/scripts/base/bif/plugins/Zeek_NTP.events.bif.zeek + build/scripts/base/bif/plugins/Zeek_POP3.consts.bif.zeek build/scripts/base/bif/plugins/Zeek_POP3.events.bif.zeek build/scripts/base/bif/plugins/Zeek_RADIUS.events.bif.zeek build/scripts/base/bif/plugins/Zeek_RDP.events.bif.zeek diff --git a/testing/btest/Baseline/plugins.hooks/output b/testing/btest/Baseline/plugins.hooks/output index b6296e53ee..6c43a53234 100644 --- a/testing/btest/Baseline/plugins.hooks/output +++ b/testing/btest/Baseline/plugins.hooks/output @@ -385,6 +385,7 @@ 0.000000 MetaHookPost LoadFile(0, ./Zeek_NetBIOS.functions.bif.zeek, <...>/Zeek_NetBIOS.functions.bif.zeek) -> -1 0.000000 MetaHookPost LoadFile(0, ./Zeek_NoneWriter.none.bif.zeek, <...>/Zeek_NoneWriter.none.bif.zeek) -> -1 0.000000 MetaHookPost LoadFile(0, ./Zeek_PE.events.bif.zeek, <...>/Zeek_PE.events.bif.zeek) -> -1 +0.000000 MetaHookPost LoadFile(0, ./Zeek_POP3.consts.bif.zeek, <...>/Zeek_POP3.consts.bif.zeek) -> -1 0.000000 MetaHookPost LoadFile(0, ./Zeek_POP3.events.bif.zeek, <...>/Zeek_POP3.events.bif.zeek) -> -1 0.000000 MetaHookPost LoadFile(0, ./Zeek_RADIUS.events.bif.zeek, <...>/Zeek_RADIUS.events.bif.zeek) -> -1 0.000000 MetaHookPost LoadFile(0, ./Zeek_RDP.events.bif.zeek, <...>/Zeek_RDP.events.bif.zeek) -> -1 @@ -682,6 +683,7 @@ 0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_NetBIOS.functions.bif.zeek, <...>/Zeek_NetBIOS.functions.bif.zeek) -> (-1, ) 0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_NoneWriter.none.bif.zeek, <...>/Zeek_NoneWriter.none.bif.zeek) -> (-1, ) 0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_PE.events.bif.zeek, <...>/Zeek_PE.events.bif.zeek) -> (-1, ) +0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_POP3.consts.bif.zeek, <...>/Zeek_POP3.consts.bif.zeek) -> (-1, ) 0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_POP3.events.bif.zeek, <...>/Zeek_POP3.events.bif.zeek) -> (-1, ) 0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_RADIUS.events.bif.zeek, <...>/Zeek_RADIUS.events.bif.zeek) -> (-1, ) 0.000000 MetaHookPost LoadFileExtended(0, ./Zeek_RDP.events.bif.zeek, <...>/Zeek_RDP.events.bif.zeek) -> (-1, ) @@ -1311,6 +1313,7 @@ 0.000000 MetaHookPre LoadFile(0, ./Zeek_NetBIOS.functions.bif.zeek, <...>/Zeek_NetBIOS.functions.bif.zeek) 0.000000 MetaHookPre LoadFile(0, ./Zeek_NoneWriter.none.bif.zeek, <...>/Zeek_NoneWriter.none.bif.zeek) 0.000000 MetaHookPre LoadFile(0, ./Zeek_PE.events.bif.zeek, <...>/Zeek_PE.events.bif.zeek) +0.000000 MetaHookPre LoadFile(0, ./Zeek_POP3.consts.bif.zeek, <...>/Zeek_POP3.consts.bif.zeek) 0.000000 MetaHookPre LoadFile(0, ./Zeek_POP3.events.bif.zeek, <...>/Zeek_POP3.events.bif.zeek) 0.000000 MetaHookPre LoadFile(0, ./Zeek_RADIUS.events.bif.zeek, <...>/Zeek_RADIUS.events.bif.zeek) 0.000000 MetaHookPre LoadFile(0, ./Zeek_RDP.events.bif.zeek, <...>/Zeek_RDP.events.bif.zeek) @@ -1608,6 +1611,7 @@ 0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_NetBIOS.functions.bif.zeek, <...>/Zeek_NetBIOS.functions.bif.zeek) 0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_NoneWriter.none.bif.zeek, <...>/Zeek_NoneWriter.none.bif.zeek) 0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_PE.events.bif.zeek, <...>/Zeek_PE.events.bif.zeek) +0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_POP3.consts.bif.zeek, <...>/Zeek_POP3.consts.bif.zeek) 0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_POP3.events.bif.zeek, <...>/Zeek_POP3.events.bif.zeek) 0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_RADIUS.events.bif.zeek, <...>/Zeek_RADIUS.events.bif.zeek) 0.000000 MetaHookPre LoadFileExtended(0, ./Zeek_RDP.events.bif.zeek, <...>/Zeek_RDP.events.bif.zeek) @@ -2236,6 +2240,7 @@ 0.000000 | HookLoadFile ./Zeek_NetBIOS.functions.bif.zeek <...>/Zeek_NetBIOS.functions.bif.zeek 0.000000 | HookLoadFile ./Zeek_NoneWriter.none.bif.zeek <...>/Zeek_NoneWriter.none.bif.zeek 0.000000 | HookLoadFile ./Zeek_PE.events.bif.zeek <...>/Zeek_PE.events.bif.zeek +0.000000 | HookLoadFile ./Zeek_POP3.consts.bif.zeek <...>/Zeek_POP3.consts.bif.zeek 0.000000 | HookLoadFile ./Zeek_POP3.events.bif.zeek <...>/Zeek_POP3.events.bif.zeek 0.000000 | HookLoadFile ./Zeek_RADIUS.events.bif.zeek <...>/Zeek_RADIUS.events.bif.zeek 0.000000 | HookLoadFile ./Zeek_RDP.events.bif.zeek <...>/Zeek_RDP.events.bif.zeek @@ -2533,6 +2538,7 @@ 0.000000 | HookLoadFileExtended ./Zeek_NetBIOS.functions.bif.zeek <...>/Zeek_NetBIOS.functions.bif.zeek 0.000000 | HookLoadFileExtended ./Zeek_NoneWriter.none.bif.zeek <...>/Zeek_NoneWriter.none.bif.zeek 0.000000 | HookLoadFileExtended ./Zeek_PE.events.bif.zeek <...>/Zeek_PE.events.bif.zeek +0.000000 | HookLoadFileExtended ./Zeek_POP3.consts.bif.zeek <...>/Zeek_POP3.consts.bif.zeek 0.000000 | HookLoadFileExtended ./Zeek_POP3.events.bif.zeek <...>/Zeek_POP3.events.bif.zeek 0.000000 | HookLoadFileExtended ./Zeek_RADIUS.events.bif.zeek <...>/Zeek_RADIUS.events.bif.zeek 0.000000 | HookLoadFileExtended ./Zeek_RDP.events.bif.zeek <...>/Zeek_RDP.events.bif.zeek diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log new file mode 100644 index 0000000000..3c1d391690 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log @@ -0,0 +1,16 @@ +### 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 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 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 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 - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/conn.log b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/conn.log new file mode 100644 index 0000000000..deeefbdba5 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/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 59954 127.0.0.1 6379 tcp - 0.002030 848 370 SF T T 0 ShADadfF 58 3872 58 3394 - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/out b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/out new file mode 100644 index 0000000000..de83d41ef3 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/out @@ -0,0 +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, diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log new file mode 100644 index 0000000000..36a176032c --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log @@ -0,0 +1,12 @@ +### 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 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 +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/pop3/redis-50-pings.pcap b/testing/btest/Traces/pop3/redis-50-pings.pcap new file mode 100644 index 0000000000..4b12508307 Binary files /dev/null and b/testing/btest/Traces/pop3/redis-50-pings.pcap differ diff --git a/testing/btest/scripts/base/protocols/pop3/redis.zeek b/testing/btest/scripts/base/protocols/pop3/redis.zeek new file mode 100644 index 0000000000..d19845aae2 --- /dev/null +++ b/testing/btest/scripts/base/protocols/pop3/redis.zeek @@ -0,0 +1,20 @@ +# @TEST-DOC: The POP3 signature triggered on Redis traffic. Ensure the analyzer is eventually removed to avoid. +# @TEST-EXEC: zeek -C -b -r $TRACES/pop3/redis-50-pings.pcap %INPUT >out +# @TEST-EXEC: btest-diff conn.log +# @TEST-EXEC: btest-diff out +# @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 pop3_request(c: connection, is_orig: bool, cmd: string, arg: string) + { + print c$uid, "pop3_request", is_orig, cmd, arg; + } + +event pop3_reply(c: connection, is_orig: bool, cmd: string, arg: string) + { + print c$uid, "pop3_reply", is_orig, cmd, arg; + }