From 353c467fb41a14cb0f347bd04d8148e90092c64a Mon Sep 17 00:00:00 2001 From: Christian Kreibich Date: Fri, 4 Oct 2024 10:31:38 -0700 Subject: [PATCH] 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 | 12 +++--------- .../analyzer.log | 11 +++++++++++ .../conn.log | 11 +++++++++++ .../weird.log | 11 +++++++++++ .../analyzer.log | 10 +++++----- .../scripts.base.protocols.pop3.redis/out | 2 +- .../scripts.base.protocols.pop3.redis/weird.log | 1 + .../btest/Traces/pop3/bad-list-retr-crafted.pcap | Bin 0 -> 3253 bytes .../protocols/pop3/bad-list-retr-crafted.zeek | 14 ++++++++++++++ .../btest/scripts/base/protocols/pop3/redis.zeek | 2 ++ 10 files changed, 59 insertions(+), 15 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/analyzer.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/conn.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.pop3.bad-list-retr-crafted/weird.log create mode 100644 testing/btest/Traces/pop3/bad-list-retr-crafted.pcap create mode 100644 testing/btest/scripts/base/protocols/pop3/bad-list-retr-crafted.zeek diff --git a/src/analyzer/protocol/pop3/POP3.cc b/src/analyzer/protocol/pop3/POP3.cc index 0d46a0e847..5ad6950e27 100644 --- a/src/analyzer/protocol/pop3/POP3.cc +++ b/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 - // 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(); } } @@ -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; 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 0000000000000000000000000000000000000000..7543e833e159d7ae7f1bb4f7052517bdec857371 GIT binary patch literal 3253 zcmaKu?{8C87{|}bQgMjC5)xl5@tB2JMks4>IO{YUQbsMyezb^UT-R=QTV?Gfy%!ZD zFfS@9;U6&0zzdf7!Z$)9Vl*l*B_heuVe{IeV*r@GtQ~yKFPCm%l3Ib z&-b34cH-2hM|`ZB$tsl?tAcO7UAZvO9jsx`!fR}`ma-^g&Hf85ti|7WaUc8vc5$BN zzpj6}2Y>UhuQWM!fH7ZnX|me4wC42og;xA|)siK5A-Tts%oviJPf9Y&7w1RU=8ge9 zb@;*3cauJyjC4sJ0`dl!gsm1iKjxn%=b|JF=M>P7kKFHL|9%CIRjw~m(e(=;rT~Sl z77@FgH;{NTAc?~DIJ%BJ((TU!Yf8$c@2*H z#(RN_w7?ZPPFdhAEpS91hs2NmtYMqM99u0~AmX1P;&eqKEYSJ+BGJZ3w!{)i*YsoH z-Z^S#4+XPPaz!v_AsB;98#S(trV~KSf+DtBWcs4Bmx#3$iC~JyRz!`ac3W%rhSs(y z52W+`9JP7Duy&ex%iy`JWo7e&Jh;kT<(%g#{!0|mtQ;&tH{jGP&?@)pgK?Er^}4s}9I4Jou%cQ=s&hIRRd0Q^%AnubAUd2=$UILa2ifa(GQWT(6PG z@W6@V#s|#dVe?hX9n%71?)*>hFVX_#mq~7PZN7;j$>?J+k|+?jf-$yQB;MxiC-K!~ z;!RR~)0aB2J=Pvw^PEwzvSvQQLqYK*H+yT(llBEUL_A5^m(C8mk-y;WewEy(nTq8hvRtk+(K6$m@VjLBh$ITjL(=kmQYR*d44yIj zcXM-q+whtK4!Z7!xBIW;J}DBntPV(*4x5 zP86Cuyfv?rX8C1}m&Q10{-Hxr^Z7mZczqQ6jfiKYrn2}rUebwMi^M6o{>Qqu#>7Xl zUBLCYm2VJtqKs>i=>rhx37w0k9{@29Nn)!-NA@Y_eF`)vi6YQ(2-HT?W4GT^(R2#m nPBOa<_vwEE==H$FR*TR9#t_~7y+n%;Z2Zm6YQ|2(2Y~+o^L#$H literal 0 HcmV?d00001 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;