From ae2a5c83a4ad7370a1c92d11009783745c7d2ebf Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 15 Jan 2024 16:44:49 +0100 Subject: [PATCH] SMTP: No state update for bad BDAT commands OSS-Fuzz found that providing an invalid BDAT line would tickle an assert in UpdateState(). The BDAT state was never initialized, but within UpdateState() that was expected. This also removes the AnalyzerViolation() call for bad BDAT commands and instead raises a weird. The SMTP analyzer is very lax and not triggering the violation allows to parse the server's response to such an invalid command. PCAP files produced by a custom Python SMTP client against Postfix. --- src/analyzer/protocol/smtp/SMTP.cc | 56 +++++++++-------- src/analyzer/protocol/smtp/SMTP.h | 1 + .../out | 58 ++++++++++++++++++ .../smtp.log | 13 ++++ .../weird.log | 13 ++++ .../Traces/smtp/smtp-bdat-cmd-invalid.pcap | Bin 0 -> 7836 bytes .../base/protocols/smtp/bdat-cmd-invalid.test | 18 ++++++ 7 files changed, 134 insertions(+), 25 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/out create mode 100644 testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/smtp.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.log create mode 100644 testing/btest/Traces/smtp/smtp-bdat-cmd-invalid.pcap create mode 100644 testing/btest/scripts/base/protocols/smtp/bdat-cmd-invalid.test diff --git a/src/analyzer/protocol/smtp/SMTP.cc b/src/analyzer/protocol/smtp/SMTP.cc index 6ba7838f31..b70dd91ba8 100644 --- a/src/analyzer/protocol/smtp/SMTP.cc +++ b/src/analyzer/protocol/smtp/SMTP.cc @@ -263,31 +263,12 @@ void SMTP_Analyzer::ProcessLine(int length, const char* line, bool orig) { RequestEvent(cmd_len, cmd, data_len, line); } - // For the BDAT command, parse out the chunk-size from the line - // and switch the ContentLineAnalyzer into plain delivery mode - // assuming things look valid. - if ( cmd_code == detail::SMTP_CMD_BDAT ) { - const auto [chunk_size, is_last_chunk, error] = detail::parse_bdat_arg(end_of_line - line, line); - if ( ! error ) { - assert(chunk_size >= 0); - auto* cl = orig ? cl_orig : cl_resp; - cl->SetPlainDelivery(chunk_size); + // See above, might have already done so. + bool do_update_state = cmd_code != detail::SMTP_CMD_END_OF_DATA; - if ( ! bdat ) { - assert(! mail); - // This is the first BDAT chunk. - BeginData(orig); - bdat = std::make_unique(Conn(), mail, - zeek::BifConst::SMTP::bdat_max_line_length); - } - - bdat->NextChunk(is_last_chunk ? detail::ChunkType::Last : detail::ChunkType::Intermediate, - chunk_size); - } - else { - AnalyzerViolation(error, line, length); - } - } + if ( cmd_code == detail::SMTP_CMD_BDAT ) + // Do not update state if this isn't a valid BDAT command. + do_update_state = ProcessBdatArg(end_of_line - line, line, orig); else if ( bdat ) { // Non-BDAT command from client but still have BDAT state, // close it out. This can happen when a client started to @@ -297,7 +278,7 @@ void SMTP_Analyzer::ProcessLine(int length, const char* line, bool orig) { EndData(); } - if ( cmd_code != detail::SMTP_CMD_END_OF_DATA ) + if ( do_update_state ) UpdateState(cmd_code, 0, orig); } } @@ -866,6 +847,31 @@ void SMTP_Analyzer::UnexpectedReply(int cmd_code, int reply_code) { void SMTP_Analyzer::ProcessData(int length, const char* line) { mail->Deliver(length, line, true /* trailing_CRLF */); } +bool SMTP_Analyzer::ProcessBdatArg(int arg_len, const char* arg, bool orig) { + // For the BDAT command, parse out the chunk-size from the line + // and switch the ContentLineAnalyzer into plain delivery mode + // assuming things look valid. + const auto [chunk_size, is_last_chunk, error] = detail::parse_bdat_arg(arg_len, arg); + if ( error ) { + Weird("smtp_invalid_bdat_command", error); + return false; + } + + auto* cl = orig ? cl_orig : cl_resp; + cl->SetPlainDelivery(chunk_size); + + if ( ! bdat ) { + // This is the first BDAT chunk. + BeginData(orig); + bdat = std::make_unique(Conn(), mail, zeek::BifConst::SMTP::bdat_max_line_length); + } + + bdat->NextChunk(is_last_chunk ? detail::ChunkType::Last : detail::ChunkType::Intermediate, chunk_size); + + // All good. + return true; +} + void SMTP_Analyzer::BeginData(bool orig) { state = detail::SMTP_IN_DATA; skip_data = false; // reset the flag at the beginning of the mail diff --git a/src/analyzer/protocol/smtp/SMTP.h b/src/analyzer/protocol/smtp/SMTP.h index d93c110666..389a45022d 100644 --- a/src/analyzer/protocol/smtp/SMTP.h +++ b/src/analyzer/protocol/smtp/SMTP.h @@ -59,6 +59,7 @@ protected: void NewReply(int reply_code, bool orig); void ProcessExtension(int ext_len, const char* ext); void ProcessData(int length, const char* line); + bool ProcessBdatArg(int arg_len, const char* arg, bool orig); void UpdateState(int cmd_code, int reply_code, bool orig); diff --git a/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/out b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/out new file mode 100644 index 0000000000..97bc40c19e --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/out @@ -0,0 +1,58 @@ +### 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, +smtp_reply, CHhAvVGS1DHFjwGM9, F, 521, BDAT, 5.5.4 Syntax: BDAT count [LAST] +smtp_request, CHhAvVGS1DHFjwGM9, T, QUIT, +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 220, >, example.com ESMTP Postfix (Debian/GNU) +smtp_request, ClEkJM2Vm5giqnMf4h, T, EHLO, localhost +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, example.com +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, PIPELINING +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, SIZE 10240000 +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, ETRN +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, STARTTLS +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, ENHANCEDSTATUSCODES +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, 8BITMIME +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, DSN +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, SMTPUTF8 +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, EHLO, CHUNKING +smtp_request, ClEkJM2Vm5giqnMf4h, T, MAIL, FROM: +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, MAIL, 2.1.0 Ok +smtp_request, ClEkJM2Vm5giqnMf4h, T, RCPT, TO: +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 250, RCPT, 2.1.5 Ok +smtp_request, ClEkJM2Vm5giqnMf4h, T, BDAT, 1234 SCRAMBLE +smtp_reply, ClEkJM2Vm5giqnMf4h, F, 521, BDAT, 5.5.4 Syntax: BDAT count [LAST] +smtp_request, ClEkJM2Vm5giqnMf4h, T, QUIT, +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 220, >, example.com ESMTP Postfix (Debian/GNU) +smtp_request, C4J4Th3PJpwUYZZ6gc, T, EHLO, localhost +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, example.com +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, PIPELINING +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, SIZE 10240000 +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, ETRN +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, STARTTLS +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, ENHANCEDSTATUSCODES +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, 8BITMIME +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, DSN +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, SMTPUTF8 +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, EHLO, CHUNKING +smtp_request, C4J4Th3PJpwUYZZ6gc, T, MAIL, FROM: +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, MAIL, 2.1.0 Ok +smtp_request, C4J4Th3PJpwUYZZ6gc, T, RCPT, TO: +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 250, RCPT, 2.1.5 Ok +smtp_request, C4J4Th3PJpwUYZZ6gc, T, BDAT, SCRAMBLE +smtp_reply, C4J4Th3PJpwUYZZ6gc, F, 521, BDAT, 5.5.4 Syntax: BDAT count [LAST] +smtp_request, C4J4Th3PJpwUYZZ6gc, T, QUIT, diff --git a/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/smtp.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/smtp.log new file mode 100644 index 0000000000..243f3ca9e8 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/smtp.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 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 33472 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) +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 127.0.0.1 52364 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) +XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 127.0.0.1 46862 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.smtp.bdat-cmd-invalid/weird.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.log new file mode 100644 index 0000000000..541c903010 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.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 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 33472 127.0.0.1 25 smtp_invalid_bdat_command BDAT not followed by a valid chunk-size F zeek SMTP +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 127.0.0.1 52364 127.0.0.1 25 smtp_invalid_bdat_command BDAT chunk-size followed by junk F zeek SMTP +XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 127.0.0.1 46862 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-invalid.pcap b/testing/btest/Traces/smtp/smtp-bdat-cmd-invalid.pcap new file mode 100644 index 0000000000000000000000000000000000000000..353cfc2d366319ce27039ff464dbddedd485fd9a GIT binary patch literal 7836 zcmdU!eQZJKD4iGn^i!7zC!p!6YGZ9OVO? zz*tr)4bhCUQaZYnLjM@s&?eF~kQH?mW2}571co}vv~>g1stG~pkOFI(I(y&uo_l?+ z<$G1yzj~x2a-HDk{GM~}^)E`Q(U`hd5Cmublk<|`-$^zM!oxyYLvu$+Tx!ZSnKQG^ z=H;0|e;`Hh2u^=%oxirGMa)>!(CTliuW4;uELu!v(^4^`w4q(xB7NObQ`=Zm=ieyS zv`OF1%--P-wPf$;@O3n|cjV-l%dYhuOcAlhW5y;JOJWjxT0T z5eAK&;Z^W8PJT=iY=V$)*u7F%IVI2hQ+P9+&z%y|KX~`pDE{Q()>R#=xM2*}DmDYJz1=T2pA{l^U8-8novy36oKH4Axa)<(p` zz+5+gRR~|>$7rr;~kXHnq6kFJVwmZJvS+Oal69UiHw{av*-`i1X`N>rrKaYw0WFfx9ASGwQp(& zi5bOyUqelER;jCUu`yLz2d;2}c zo#6_&#>o%LTx|FSF}|OWF*oVn>0yO&0y6Kk*~`ksreJMNQ$6IwT*!*gwE>vgxqL42 z+$J-b=REK{3Av)CAE}rpV!0+*0L*2$-Wk3I*Esn>xdwgZ$OvCc$bh~4?neq^0CJtk z$u(zkdudF?7u*iFt<2$axJspKk7J!p%r@s(%rK;vY~BhN`=+;`!s{*bu-9Go0#}i( z*yAnmR(guci*4*1%L^S|r^9KJeyrFd|8F=tmEMx&^s-oFuXH_2SV6$d@qVq4UE8l= z5ACT%&d$!wB-|@k!0%Y*kShB?%x^aniGH{IzAFqHA>iZ(`E?s!B*r5N8FQ1y4jL53 zP4GM5EO3;GB^BjPYkrsC-?&maLM!kLbprr1wlEO#^WN+VyWtuqKZsxGbJH2BmHF%p zUHRbMzDMz1y>>(4-va(k*iJFWlx;GL<&DNvKCOm*#8@n6VtXbrz-RAY6vi6Jw5YJSz-vs!_eXcT z*c&3}^4jg;I=IHk58|x#Rgk+?GKaal4ep*scj6g^vlcjOa&xl9T<8@R(bL)7UK6s4 zNET~@Pz=QCvI38H1KbqIk3XaThIQyTeVOu_`|8zY_`KvRATyY+H1JgjdlG$eT*(s0 zXp(f1uao0N5$t<7`9bF|-!v5&GvAUKgFQle*A@S5{7LM5tKQ~%Sk)7+;_K=j#xN^O-wf zASZU7_hLjCc-DTKTpZEve8Zn1qv-ore2?_PaTVe}&7>vz6>SdUKTZ&T*C z<@Iox?_%bqcU0@?jb-LLU&{I7h<4{Qr4r+J2^rA&F8oVj3_<4JFW31N0#iHJ@Hlxs zD>IqryWlyB_rcOO6>}(-YeL#e+Dpe!aYVcG4S!Ay<@u*#z+OIfS7BU&TtnZi&es9i zwf&l=)1D5op0*v zHa(|C%6nX$chfv@dVrjksdyi`LvrJBF5`-Ch0z44DcDrRTu%tWTobno%0lQ zv7(9sXJHxqVkQX4i|>HL&y+^AJK%7XTq<1V@(8%Jqst6g1R{C04BksQxats@w7%zrM=TB*kvo}{d&Wd_T7k+MPuv`p7x@Ey=zR2s2^XPo?? zeYBcpQ;wrDgY6^c7zCeVJ*yQ)3-}Cbcfda2==e$_&apU-i#X5$Ei=@d#J(=*GA;|` z>#s6{`5FXY<*+AFOR|d55@Ybatosj}N3=U&buBWkSILZlnL_%@hb|pLv)BRM@9@2> ze;>ZC?qxh{k1k7~JbVWnn0YyP-UYnF*a3$(t65o9{rl7`{;VAUmiGQsZ=_rs;&L4Z z9=f|XeqMIhzYjSvcY84+418wX_|xVQ?G6}fMaG$hGDAqGJmr`N@7-U)lSsOE`}ZT` z>vzE0upXVve#*RFUJu(#%)E^nz0X?5v$fvTO%;y5|`3d@b!>F(=SyjigWuMv|@F4JX zd^S?n_|K_Nz-Jcu^r?83m{0!Mw&rGF1PXwFlON=>sJe+5C2@NuF~H{vcvt4L4l-G& z1M>Ax`J2Wr_U5>U3-1I*wEJIZD|uA@j>0@SS@?yL8|<**JNzX8hb zjmtg;e!b}T{u$Le8e)F=&*TbSMkqvC9WsMu^-@;ofA>#nG3q&=cRme_*x?!{KcxOw z-Ap;0GK1ZBn4=PWj=7W74DJ56;J>=Qg2bta<9tpW=zsU$QF9XeGV3xzo#c!AyCU|B R3zgt&0`??&|E)xf{{s)mhNb`j literal 0 HcmV?d00001 diff --git a/testing/btest/scripts/base/protocols/smtp/bdat-cmd-invalid.test b/testing/btest/scripts/base/protocols/smtp/bdat-cmd-invalid.test new file mode 100644 index 0000000000..c127d2c14d --- /dev/null +++ b/testing/btest/scripts/base/protocols/smtp/bdat-cmd-invalid.test @@ -0,0 +1,18 @@ +# @TEST-DOC: Test invalid BDAT lines. Pcaps generated with a Python client against Postfix. +# +# @TEST-EXEC: zeek -b -r $TRACES/smtp/smtp-bdat-cmd-invalid.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; +}