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 0000000000..f1d481449f Binary files /dev/null and b/testing/btest/Traces/smtp/smtp-bdat-cmd-chunk-size-overflow.pcap differ diff --git a/testing/btest/Traces/smtp/smtp-bdat-cmd-negative-chunk-size.pcap b/testing/btest/Traces/smtp/smtp-bdat-cmd-negative-chunk-size.pcap new file mode 100644 index 0000000000..f10c5b45e7 Binary files /dev/null and b/testing/btest/Traces/smtp/smtp-bdat-cmd-negative-chunk-size.pcap differ 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; +}