From c23d605286d0cbc765ad53c5c34b94c1dfd8e1bd Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 19 Jan 2024 11:43:57 +0100 Subject: [PATCH] SMTP/BDAT: Fix int/int64_t/uint64_t confusion The BDAT analyzer should be supporting uint64_t sized chunks reasonably well, but the ContentLine analyzer does not, And also, I totally got types for RemainingChunkSize() and in DeliverStream() wrong, resulting in overflows and segfaults when very large chunk sizes were used. Tickled by OSS-Fuzz. Actually running the fuzzer locally only took a few minutes to find the crash, too. Embarrassing. --- src/analyzer/protocol/smtp/BDAT.cc | 22 +++++++++++++++++- src/analyzer/protocol/smtp/BDAT.h | 2 +- src/analyzer/protocol/smtp/SMTP.cc | 9 ++++--- .../out | 20 ++++++++++++++++ .../smtp.log | 11 +++++++++ .../weird.log | 11 +++++++++ .../out | 20 ++++++++++++++++ .../smtp.log | 11 +++++++++ .../weird.log | 11 +++++++++ .../smtp-bdat-cmd-chunk-size-overflow.pcap | Bin 0 -> 2214 bytes .../smtp-bdat-cmd-negative-chunk-size.pcap | Bin 0 -> 2196 bytes .../protocols/bdat-chunk-size-overflow.test | 18 ++++++++++++++ .../protocols/bdat-negative-chunk-size.test | 18 ++++++++++++++ 13 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/out create mode 100644 testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/smtp.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/weird.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/out create mode 100644 testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/smtp.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/weird.log create mode 100644 testing/btest/Traces/smtp/smtp-bdat-cmd-chunk-size-overflow.pcap create mode 100644 testing/btest/Traces/smtp/smtp-bdat-cmd-negative-chunk-size.pcap create mode 100644 testing/btest/scripts/base/protocols/bdat-chunk-size-overflow.test create mode 100644 testing/btest/scripts/base/protocols/bdat-negative-chunk-size.test diff --git a/src/analyzer/protocol/smtp/BDAT.cc b/src/analyzer/protocol/smtp/BDAT.cc index 99e511ef89..ddb02f525f 100644 --- a/src/analyzer/protocol/smtp/BDAT.cc +++ b/src/analyzer/protocol/smtp/BDAT.cc @@ -55,10 +55,16 @@ void SMTP_BDAT_Analyzer::DeliverStream(int len, const u_char* data, bool is_orig assert(mail != nullptr); assert(! IsFinished()); + // We cast to uint64_t, so need to have a positive value. + if ( len < 0 ) { + Weird("smtp_bdat_negative_len"); + return; + } + // Upstream analyzer delivers more data than we're // expecting for the current chunk. Likely a logic // error on their side. Truncate it. - if ( len > RemainingChunkSize() ) { + if ( static_cast(len) > RemainingChunkSize() ) { Weird("smtp_bdat_chunk_overflow"); len = static_cast(RemainingChunkSize()); } @@ -204,6 +210,20 @@ TEST_CASE("chunk size followed by lastjunk") { CHECK(error == std::string("BDAT chunk-size followed by junk")); } +TEST_CASE("huge chunk size") { + std::string line = "15555555557777777777"; + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(line.size(), line.c_str()); + REQUIRE(error == nullptr); + CHECK(chunk_size == 15555555557777777777UL); +} + +TEST_CASE("negative chunk size") { + std::string line = "-42 LAST"; + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(line.size(), line.c_str()); + REQUIRE(error != nullptr); + CHECK(error == std::string("BDAT not followed by a valid chunk-size")); +} + TEST_SUITE_END(); TEST_SUITE_BEGIN("bdat line analyzer"); diff --git a/src/analyzer/protocol/smtp/BDAT.h b/src/analyzer/protocol/smtp/BDAT.h index bbee3f3ef6..a9e7aca197 100644 --- a/src/analyzer/protocol/smtp/BDAT.h +++ b/src/analyzer/protocol/smtp/BDAT.h @@ -88,7 +88,7 @@ public: /** * @return The remaining size of the current chunk. */ - int64_t RemainingChunkSize() const { return remaining_chunk_size; } + uint64_t RemainingChunkSize() const { return remaining_chunk_size; } /** * @return true if the current chunk was started with LAST. diff --git a/src/analyzer/protocol/smtp/SMTP.cc b/src/analyzer/protocol/smtp/SMTP.cc index 1b0e36d8b3..8fa3dd2c98 100644 --- a/src/analyzer/protocol/smtp/SMTP.cc +++ b/src/analyzer/protocol/smtp/SMTP.cc @@ -120,12 +120,15 @@ void SMTP_Analyzer::DeliverStream(int length, const u_char* line, bool orig) { // NOTE: do not use IsOrig() here, because of TURN command. bool is_sender = orig_is_sender ? orig : ! orig; - if ( is_sender && bdat ) { + if ( length > 0 && is_sender && bdat ) { // We're processing BDAT and have switched the ContentLine analyzer // into plain mode to send us the full chunk. Ensure we only use up // as much as we need in case we get more. - int64_t bdat_len = std::min(bdat->RemainingChunkSize(), static_cast(length)); - if ( bdat->RemainingChunkSize() > 0 ) + int bdat_len = length; + if ( bdat->RemainingChunkSize() < static_cast(bdat_len) ) + bdat_len = static_cast(bdat->RemainingChunkSize()); + + if ( bdat_len > 0 ) bdat->NextStream(bdat_len, line, orig); // All BDAT chunks seen? diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/out b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/out new file mode 100644 index 0000000000..8b3b970ebd --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/out @@ -0,0 +1,20 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +smtp_reply, CHhAvVGS1DHFjwGM9, F, 220, >, example.com ESMTP Postfix (Debian/GNU) +smtp_request, CHhAvVGS1DHFjwGM9, T, EHLO, localhost +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, example.com +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, PIPELINING +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, SIZE 10240000 +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, ETRN +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, STARTTLS +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, ENHANCEDSTATUSCODES +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, 8BITMIME +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, DSN +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, SMTPUTF8 +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, CHUNKING +smtp_request, CHhAvVGS1DHFjwGM9, T, MAIL, FROM: +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, MAIL, 2.1.0 Ok +smtp_request, CHhAvVGS1DHFjwGM9, T, RCPT, TO: +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, RCPT, 2.1.5 Ok +smtp_request, CHhAvVGS1DHFjwGM9, T, BDAT, 15555555557777777777 LAST +smtp_reply, CHhAvVGS1DHFjwGM9, F, 521, BDAT, 5.5.4 Syntax: BDAT count [LAST] +smtp_request, CHhAvVGS1DHFjwGM9, T, QUIT, diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/smtp.log b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/smtp.log new file mode 100644 index 0000000000..1e443c9850 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/smtp.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 smtp +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p trans_depth helo mailfrom rcptto date from to cc reply_to msg_id in_reply_to subject x_originating_ip first_received second_received last_reply path user_agent tls fuids +#types time string addr port addr port count string string set[string] string string set[string] set[string] string string string string addr string string string vector[addr] string bool vector[string] +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 41610 127.0.0.1 25 1 localhost zeek@localhost root@localhost - - - - - - - - - - - 521 5.5.4 Syntax: BDAT count [LAST] 127.0.0.1,127.0.0.1 - F (empty) +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/weird.log b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/weird.log new file mode 100644 index 0000000000..d96b345ed9 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/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 41610 127.0.0.1 25 smtp_huge_bdat_chunk 15555555557777777777 F zeek SMTP +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/out b/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/out new file mode 100644 index 0000000000..005ed7cb64 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/out @@ -0,0 +1,20 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +smtp_reply, CHhAvVGS1DHFjwGM9, F, 220, >, example.com ESMTP Postfix (Debian/GNU) +smtp_request, CHhAvVGS1DHFjwGM9, T, EHLO, localhost +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, example.com +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, PIPELINING +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, SIZE 10240000 +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, ETRN +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, STARTTLS +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, ENHANCEDSTATUSCODES +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, 8BITMIME +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, DSN +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, SMTPUTF8 +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, EHLO, CHUNKING +smtp_request, CHhAvVGS1DHFjwGM9, T, MAIL, FROM: +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, MAIL, 2.1.0 Ok +smtp_request, CHhAvVGS1DHFjwGM9, T, RCPT, TO: +smtp_reply, CHhAvVGS1DHFjwGM9, F, 250, RCPT, 2.1.5 Ok +smtp_request, CHhAvVGS1DHFjwGM9, T, BDAT, -1 LAST +smtp_reply, CHhAvVGS1DHFjwGM9, F, 521, BDAT, 5.5.4 Syntax: BDAT count [LAST] +smtp_request, CHhAvVGS1DHFjwGM9, T, QUIT, diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/smtp.log b/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/smtp.log new file mode 100644 index 0000000000..36157a4d00 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/smtp.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 smtp +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p trans_depth helo mailfrom rcptto date from to cc reply_to msg_id in_reply_to subject x_originating_ip first_received second_received last_reply path user_agent tls fuids +#types time string addr port addr port count string string set[string] string string set[string] set[string] string string string string addr string string string vector[addr] string bool vector[string] +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 54028 127.0.0.1 25 1 localhost zeek@localhost root@localhost - - - - - - - - - - - 521 5.5.4 Syntax: BDAT count [LAST] 127.0.0.1,127.0.0.1 - F (empty) +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/weird.log b/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/weird.log new file mode 100644 index 0000000000..80fbb98f8c --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/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 54028 127.0.0.1 25 smtp_invalid_bdat_command BDAT not followed by a valid chunk-size F zeek SMTP +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/smtp/smtp-bdat-cmd-chunk-size-overflow.pcap b/testing/btest/Traces/smtp/smtp-bdat-cmd-chunk-size-overflow.pcap new file mode 100644 index 0000000000000000000000000000000000000000..f1d481449fa7da1c6463a735caa16a826dab9d33 GIT binary patch literal 2214 zcmaKsUuaWz6vuywX_^))Q#Ys}^M^3l7>h|vnl=<^+FWh6{9Epg|J$a@u z#iPYPoXo|jIod;#~yWwv>7>%dL? z(Qb z{8lp31^FV7yFm%pJmfhjZzs=J1zGpJ0Q5UEpE=%~`yD(wG`~niuV=2h**KtZ%|pbz zT0r7JKoE7WchKuhtE0ZfX%NFm3bi6O;W!oLQ6D>Ugth zk3k%PJYlIfk>;bN!re2saoyVkHJbzVA#oXlcd=nRrG@#8dIqZtb-kKOk|xFA(&>XpbZ% zo6_G@qFF^r$olhCZ#2~-#bhO_#uI#3Hl2)go-P)TcNwSOsoj2In}=mU2G=~K)~z0;+b4yZcKfyoxcxHz zw|qNhQLDgVc);uT`gr=d#;*e3rk^0CnG<}ypJnK07W+B-#LB0BRy^zwSj9CDIZnx; zHT%(s(9f4U4B|8Br;_c-C|pTLI!~5L<&WB#|HrnSA!;Wf-0SVkLOZM2&R43TwFFwv zL^~m)oeJ=6+G(RKAO2#mLOb)+&igFLU&3}`cbixNYQZ%RwUd*>L>v~5^}aPCwBv%W zX}_n-E+lrxq6+ti#DtCEiKwi!XeT~(zljZj7Or_na=&_vN(u@|y`=fYt@-s>(oZ`L z$%c+=%dSws&qLmjH^}8vYm9AOiC<|HF-mxS7M|&G1$o{ZYAsem1nG8b0 zLPGF~;GQ%rWLrWQg3%a-IRQ~@GSuk93=w@dJ`%Ssi!q(Q-|ybj+zaFWl9Q~b?dSXF z*LJ@>_+rAv+)U2$GGhun(0V_~|MyxAdmaA9V;v9E7;95~?W}#Z@A_GI1D?6HtYNWy zdmdjo?)vx1FTXP8a=-AU+vV}@8T;WwyjNDOszLJlRwpuJNN$^vWY#dZ);IO_WuPyN zZ>W5rd}SviU6Qv0xdW8&Scg2fs2j-h=aOuCUI6;o$cxHiQyq9z48KT4uUGfhumYg) zScizc;tmr3c`S*h*D>@uQm<4Vp05%|kr)gHcz(F=#Jhuee}C}=*Rlyc&C|vAhTc9t z%y-1{N00XvcJEK-cGh}P{U)e88qUp5lYW{=UIukiqbL0TiC^S=>d1dAC$B8=GA$PrJc$W!EvA zLJ2*8^QlZ~?0eeno0{o`?#iltJJelp*2B7?G(6U!?$YYZ zM7*>jv0f=(USI8Q3HDt|L`0l-W>Sf8+v$9MpuM{Kea7x@JoKd9OEDk->&gRiIPVL;McIKsfvz-F8 z6UKJtj#{-!s%$4zZRb_sJG9e4S)TgEo(@AhXRw{dZ|r<*=f^j^>>$*F$2!zbRNYL( zkELU?Z-WT!T&aHZO5JKhJolI$_`;fQ;=mzG=oe~5T8tQeJf==*QhpGvyuaL)aNwj#J zglfv?-~(;^FDqd+JqL3_H?r>ou0!^%#J&0yHwD=zM||)p*<|Np_D)BlctF`Jl4xe1 OgzRZ(v(mWEPW&If^buYF literal 0 HcmV?d00001 diff --git a/testing/btest/scripts/base/protocols/bdat-chunk-size-overflow.test b/testing/btest/scripts/base/protocols/bdat-chunk-size-overflow.test new file mode 100644 index 0000000000..9d6785e71e --- /dev/null +++ b/testing/btest/scripts/base/protocols/bdat-chunk-size-overflow.test @@ -0,0 +1,18 @@ +# @TEST-DOC: Test a BDAT line with an overflowing integer size. Pcaps generated with a Python client against Postfix. +# +# @TEST-EXEC: zeek -r $TRACES/smtp/smtp-bdat-cmd-chunk-size-overflow.pcap %INPUT >out +# @TEST-EXEC: btest-diff smtp.log +# @TEST-EXEC: btest-diff weird.log +# @TEST-EXEC: btest-diff out + +@load base/protocols/conn +@load base/protocols/smtp + +event smtp_request(c: connection, is_orig: bool, command: string, arg: string) { + print "smtp_request", c$uid, is_orig, command, arg; +} + +event smtp_reply(c: connection, is_orig: bool, code: count, cmd: string, + msg: string, cont_resp: bool) { + print "smtp_reply", c$uid, is_orig, code, cmd, msg; +} diff --git a/testing/btest/scripts/base/protocols/bdat-negative-chunk-size.test b/testing/btest/scripts/base/protocols/bdat-negative-chunk-size.test new file mode 100644 index 0000000000..1e074025c2 --- /dev/null +++ b/testing/btest/scripts/base/protocols/bdat-negative-chunk-size.test @@ -0,0 +1,18 @@ +# @TEST-DOC: Test a BDAT line with a negative size. Pcaps generated with a Python client against Postfix. +# +# @TEST-EXEC: zeek -r $TRACES/smtp/smtp-bdat-cmd-negative-chunk-size.pcap %INPUT >out +# @TEST-EXEC: btest-diff smtp.log +# @TEST-EXEC: btest-diff weird.log +# @TEST-EXEC: btest-diff out + +@load base/protocols/conn +@load base/protocols/smtp + +event smtp_request(c: connection, is_orig: bool, command: string, arg: string) { + print "smtp_request", c$uid, is_orig, command, arg; +} + +event smtp_reply(c: connection, is_orig: bool, code: count, cmd: string, + msg: string, cont_resp: bool) { + print "smtp_reply", c$uid, is_orig, code, cmd, msg; +}