diff --git a/CHANGES b/CHANGES index aaeab45fb9..eba74fc287 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,12 @@ +7.0.2-5 | 2024-10-04 10:46:01 -0700 + + * Merge remote-tracking branch 'security/topic/awelzel/215-pop3-mail-null-deref' (Christian Kreibich, Corelight) + + * security/topic/awelzel/215-pop3-mail-null-deref: + POP3: Rework unbounded pending command fix + + (cherry picked from commit 7fea32c6edc5d4d14646366f87c9208c8c9cf555) + 7.0.2-4 | 2024-10-04 10:28:13 -0700 * Update docs submodule [nomail] [skip ci] (Christian Kreibich, Corelight) diff --git a/VERSION b/VERSION index 0f7e9dcaac..189fa8c988 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -7.0.2-4 +7.0.2-5 diff --git a/src/analyzer/protocol/pop3/POP3.cc b/src/analyzer/protocol/pop3/POP3.cc index a65c8410ba..fc11b4ca41 100644 --- a/src/analyzer/protocol/pop3/POP3.cc +++ b/src/analyzer/protocol/pop3/POP3.cc @@ -206,16 +206,14 @@ void POP3_Analyzer::ProcessRequest(int length, const char* line) { 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. + // server replies by simply dropping the oldest command. // - // This may be caused by packet drops, one-sided traffic, analyzing - // the wrong protocol (Redis), etc. + // This may be caused by packet drops of the server side, one-sided + // 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(); } } @@ -583,10 +581,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; } diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/analyzer.log b/testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/analyzer.log new file mode 100644 index 0000000000..23f62ab8e7 --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/conn.log b/testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/conn.log new file mode 100644 index 0000000000..1ead1294bf --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/weird.log b/testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/weird.log new file mode 100644 index 0000000000..b661991b9e --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log index 3c1d391690..3f9f9ae848 100644 --- a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/analyzer.log +++ b/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 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 unknown server command (+PONG) +PONG +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 unknown server command (+PONG) +PONG +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 unknown server command (+PONG) +PONG #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 index de83d41ef3..fe3bec3f1d 100644 --- a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/out +++ b/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, diff --git a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log b/testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log index 36a176032c..7ac280b3c0 100644 --- a/testing/btest/Baseline/scripts.base.protocols.pop3.redis/weird.log +++ b/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 diff --git a/testing/btest/Traces/pop3/bad-list-retr-crafted.pcap b/testing/btest/Traces/pop3/bad-list-retr-crafted.pcap new file mode 100644 index 0000000000..7543e833e1 Binary files /dev/null and b/testing/btest/Traces/pop3/bad-list-retr-crafted.pcap differ diff --git a/testing/btest/scripts/base/protocols/pop3/bad-list-retr-crafted.zeek b/testing/btest/scripts/base/protocols/pop3/bad-list-retr-crafted.zeek new file mode 100644 index 0000000000..2aedeac81e --- /dev/null +++ b/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); + } diff --git a/testing/btest/scripts/base/protocols/pop3/redis.zeek b/testing/btest/scripts/base/protocols/pop3/redis.zeek index d19845aae2..137baf459c 100644 --- a/testing/btest/scripts/base/protocols/pop3/redis.zeek +++ b/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;