From 9a510b8035e6a83fb149cc447a61f847e786140f Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 23 Jan 2024 15:03:37 +0100 Subject: [PATCH 1/4] SMTP: Add SMTP_IN_BDAT state Initially this reused SMTP_IN_DATA, but separating into SMTP_IN_BDAT to avoid spurious EndData() calls upon a server's reply. The client should usually continue to send the full in-flight chunk still. --- src/analyzer/protocol/smtp/SMTP.cc | 24 ++++++++++++------------ src/analyzer/protocol/smtp/SMTP.h | 3 ++- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/analyzer/protocol/smtp/SMTP.cc b/src/analyzer/protocol/smtp/SMTP.cc index 8fa3dd2c98..185a3b46bd 100644 --- a/src/analyzer/protocol/smtp/SMTP.cc +++ b/src/analyzer/protocol/smtp/SMTP.cc @@ -84,7 +84,7 @@ void SMTP_Analyzer::Undelivered(uint64_t seq, int len, bool is_orig) { Unexpected(is_orig, "content gap", buf_len, buf); - if ( state == detail::SMTP_IN_DATA ) { + if ( state == detail::SMTP_IN_DATA || state == detail::SMTP_CMD_BDAT ) { // Record the SMTP data gap and terminate the // ongoing mail transaction. if ( mail ) @@ -202,7 +202,7 @@ void SMTP_Analyzer::ProcessLine(int length, const char* line, bool orig) { expect_recver = true; } - else if ( state == detail::SMTP_IN_DATA && ! bdat ) { + else if ( state == detail::SMTP_IN_DATA ) { // Check "." for end of data for non-BDAT transfers. expect_recver = false; // ?? MAY server respond to mail data? @@ -591,21 +591,21 @@ void SMTP_Analyzer::UpdateState(int cmd_code, int reply_code, bool orig) { UnexpectedCommand(cmd_code, reply_code); assert(bdat); - state = detail::SMTP_IN_DATA; + state = detail::SMTP_IN_BDAT; break; case 250: break; // server accepted BDAT transfer. - case 421: state = detail::SMTP_QUIT; break; - + case 421: case 500: case 501: case 503: case 451: case 554: - // Client may continue sending chunks if pipelined. We don't - // call EndData() here as it might be interesting what the - // client does send, even if the server isn't accepting it. + // Client will continue completing the inflight chunk no matter + // what the server replies, so we don't call EndData() here as + // it might be interesting what the client does actually send, + // even if the server isn't accepting it. break; default: @@ -618,7 +618,7 @@ void SMTP_Analyzer::UpdateState(int cmd_code, int reply_code, bool orig) { case detail::SMTP_CMD_END_OF_DATA: switch ( reply_code ) { case 0: - if ( st != detail::SMTP_IN_DATA ) + if ( st != detail::SMTP_IN_DATA && st != detail::SMTP_IN_BDAT ) UnexpectedCommand(cmd_code, reply_code); EndData(); state = detail::SMTP_AFTER_DATA; @@ -875,7 +875,7 @@ bool SMTP_Analyzer::ProcessBdatArg(int arg_len, const char* arg, bool orig) { if ( ! bdat ) { // This is the first BDAT chunk. - BeginData(orig); + BeginData(orig, detail::SMTP_IN_BDAT); bdat = std::make_unique(Conn(), mail, zeek::BifConst::SMTP::bdat_max_line_length); } @@ -885,8 +885,8 @@ bool SMTP_Analyzer::ProcessBdatArg(int arg_len, const char* arg, bool orig) { return true; } -void SMTP_Analyzer::BeginData(bool orig) { - state = detail::SMTP_IN_DATA; +void SMTP_Analyzer::BeginData(bool orig, detail::SMTP_State new_state) { + state = new_state; skip_data = false; // reset the flag at the beginning of the mail if ( mail != nullptr ) { Weird("smtp_nested_mail_transaction"); diff --git a/src/analyzer/protocol/smtp/SMTP.h b/src/analyzer/protocol/smtp/SMTP.h index 389a45022d..1d255bbed3 100644 --- a/src/analyzer/protocol/smtp/SMTP.h +++ b/src/analyzer/protocol/smtp/SMTP.h @@ -35,6 +35,7 @@ enum SMTP_State { SMTP_QUIT, // 10: after QUIT SMTP_AFTER_GAP, // 11: after a gap is detected SMTP_GAP_RECOVERY, // 12: after the first reply after a gap + SMTP_IN_BDAT, // 13: receiving BDAT chunk via plain delivery }; } // namespace detail @@ -63,7 +64,7 @@ protected: void UpdateState(int cmd_code, int reply_code, bool orig); - void BeginData(bool orig); + void BeginData(bool orig, detail::SMTP_State new_state = detail::SMTP_IN_DATA); void EndData(); int ParseCmd(int cmd_len, const char* cmd); From bc357c6ca1be3338abe2fc4e0ca4e47dfe10f99d Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 23 Jan 2024 15:38:24 +0100 Subject: [PATCH 2/4] SMTP: Reset ContentLineAnalyzer plain delivery on EndData() When resetting the BDAT state, we also need to switch the ContentLine analyzer back into line mode, otherwise we're feeding plain delivery data through ProcessLine(), possibly violating some assumptions about null termination. Do it for both ContentLineAnalyzers - only one of them will be in plain delivery mode anyhow, but we don't keep state which one it was. --- src/analyzer/protocol/smtp/SMTP.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/analyzer/protocol/smtp/SMTP.cc b/src/analyzer/protocol/smtp/SMTP.cc index 185a3b46bd..e717742cdc 100644 --- a/src/analyzer/protocol/smtp/SMTP.cc +++ b/src/analyzer/protocol/smtp/SMTP.cc @@ -899,6 +899,12 @@ void SMTP_Analyzer::BeginData(bool orig, detail::SMTP_State new_state) { void SMTP_Analyzer::EndData() { if ( bdat ) { + if ( bdat->RemainingChunkSize() > 0 ) { + Weird("smtp_bdat_remaining_at_end_data"); + cl_orig->SetPlainDelivery(0); + cl_resp->SetPlainDelivery(0); + } + bdat->Done(); bdat.reset(); } From ce4647a50761c0866edfa5bea4462f1b72d7ab86 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 23 Jan 2024 16:40:03 +0100 Subject: [PATCH 3/4] BDAT: Harden parse_bdat_arg() There implementation assumed that arg is null terminated. Due to the ContentLineAnalyzer wrongly being in plain delivery mode, this assumption was violated. It shouldn't happen anymore, but protect from this anyhow. --- src/analyzer/protocol/smtp/BDAT.cc | 51 +++++++++++++++++-- .../weird.log | 2 +- .../weird.log | 2 +- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/analyzer/protocol/smtp/BDAT.cc b/src/analyzer/protocol/smtp/BDAT.cc index 999b4dfbcb..bd1f8fadf1 100644 --- a/src/analyzer/protocol/smtp/BDAT.cc +++ b/src/analyzer/protocol/smtp/BDAT.cc @@ -10,18 +10,36 @@ namespace zeek::analyzer::smtp::detail { struct BDATCmd parse_bdat_arg(int length, const char* arg) { - const char* arg_end = arg + length; struct BDATCmd r = {0}; + // UINT64_MAX followed by " LAST" is the most we can deal with + // and anyway this would be really weird for a client to use. + // strlen("18446744073709551615 LAST") + constexpr int max_arg_len = 25; + + if ( length <= 0 || length > max_arg_len ) { + r.error = "BDAT argument bad length"; + return r; + } + if ( *arg == '\0' || ! isdigit(*arg) ) { r.error = "BDAT not followed by a valid chunk-size"; return r; } + // Ensure arg is NULL terminated by copying the + // input into a new std::string object so we can use + // strtoull() properly. We do have zeek::util::atoi_n, + // but it's not handling overflows. + // + // The size is bounded, see max_arg_len above. + std::string arg_copy = {arg, static_cast(length)}; + const char* arg_end = arg_copy.c_str() + length; + errno = 0; char* chunk_size_end = nullptr; - uint64_t chunk_size = strtoull(arg, &chunk_size_end, 10); - if ( *chunk_size_end != ' ' && chunk_size_end != arg_end ) { + uint64_t chunk_size = strtoull(arg_copy.c_str(), &chunk_size_end, 10); + if ( *chunk_size_end != ' ' && *chunk_size_end != '\0' ) { r.error = "BDAT chunk-size not valid"; return r; } @@ -239,6 +257,33 @@ TEST_CASE("negative chunk size") { CHECK(error == std::string("BDAT not followed by a valid chunk-size")); } +TEST_CASE("non null terminated") { + // Regression test for buffer overread triggered by non-null + // terminated input from a wrongly configured ContentLineAnalyzer. + const char* input_data = "7777777777"; + auto data = std::make_unique(strlen(input_data)); + memcpy(data.get(), input_data, strlen(input_data)); + + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(strlen(input_data), data.get()); + REQUIRE(error == nullptr); + CHECK(chunk_size == 7777777777); +} + +TEST_CASE("maximum length") { + std::string line = "18446744073709551615 LAST"; + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(line.size(), line.c_str()); + REQUIRE(error == nullptr); + CHECK(chunk_size == 18446744073709551615UL); + CHECK(is_last_chunk == true); +} + +TEST_CASE("maximum length exceeded") { + std::string line = "184467440737095516150 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 argument bad length")); +} + TEST_SUITE_END(); TEST_SUITE_BEGIN("bdat line analyzer"); diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log index 2129c89e16..efc3169f89 100644 --- a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log @@ -7,5 +7,5 @@ #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 33762 127.0.0.1 25 smtp_invalid_bdat_command BDAT chunk-size too large F zeek SMTP +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 33762 127.0.0.1 25 smtp_invalid_bdat_command BDAT argument bad length F zeek SMTP #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 index 541c903010..783e430c0f 100644 --- 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 @@ -7,7 +7,7 @@ #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 CHhAvVGS1DHFjwGM9 127.0.0.1 33472 127.0.0.1 25 smtp_invalid_bdat_command BDAT argument bad length 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 From 7373549de4f03299f6f79db2b1530ee57288413e Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 23 Jan 2024 17:20:34 +0100 Subject: [PATCH 4/4] btest/smtp/bdat: Move tests into proper directory --- .../out | 0 .../smtp.log | 0 .../weird.log | 0 .../out | 0 .../smtp.log | 0 .../weird.log | 0 .../out | 0 .../smtp.log | 0 .../weird.log | 0 .../base/protocols/{ => smtp}/bdat-chunk-size-overflow.test | 0 .../base/protocols/{ => smtp}/bdat-chunk-size-overflow2.test | 0 .../base/protocols/{ => smtp}/bdat-negative-chunk-size.test | 0 12 files changed, 0 insertions(+), 0 deletions(-) rename testing/btest/Baseline/{scripts.base.protocols.bdat-chunk-size-overflow => scripts.base.protocols.smtp.bdat-chunk-size-overflow}/out (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-chunk-size-overflow => scripts.base.protocols.smtp.bdat-chunk-size-overflow}/smtp.log (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-chunk-size-overflow => scripts.base.protocols.smtp.bdat-chunk-size-overflow}/weird.log (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-chunk-size-overflow2 => scripts.base.protocols.smtp.bdat-chunk-size-overflow2}/out (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-chunk-size-overflow2 => scripts.base.protocols.smtp.bdat-chunk-size-overflow2}/smtp.log (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-chunk-size-overflow2 => scripts.base.protocols.smtp.bdat-chunk-size-overflow2}/weird.log (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-negative-chunk-size => scripts.base.protocols.smtp.bdat-negative-chunk-size}/out (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-negative-chunk-size => scripts.base.protocols.smtp.bdat-negative-chunk-size}/smtp.log (100%) rename testing/btest/Baseline/{scripts.base.protocols.bdat-negative-chunk-size => scripts.base.protocols.smtp.bdat-negative-chunk-size}/weird.log (100%) rename testing/btest/scripts/base/protocols/{ => smtp}/bdat-chunk-size-overflow.test (100%) rename testing/btest/scripts/base/protocols/{ => smtp}/bdat-chunk-size-overflow2.test (100%) rename testing/btest/scripts/base/protocols/{ => smtp}/bdat-negative-chunk-size.test (100%) diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/out b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow/out similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/out rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow/out diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/smtp.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow/smtp.log similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/smtp.log rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow/smtp.log diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/weird.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow/weird.log similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow/weird.log rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow/weird.log diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/out b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow2/out similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/out rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow2/out diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/smtp.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow2/smtp.log similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/smtp.log rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow2/smtp.log diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow2/weird.log similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-chunk-size-overflow2/weird.log diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/out b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-negative-chunk-size/out similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/out rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-negative-chunk-size/out diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/smtp.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-negative-chunk-size/smtp.log similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/smtp.log rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-negative-chunk-size/smtp.log diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/weird.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-negative-chunk-size/weird.log similarity index 100% rename from testing/btest/Baseline/scripts.base.protocols.bdat-negative-chunk-size/weird.log rename to testing/btest/Baseline/scripts.base.protocols.smtp.bdat-negative-chunk-size/weird.log diff --git a/testing/btest/scripts/base/protocols/bdat-chunk-size-overflow.test b/testing/btest/scripts/base/protocols/smtp/bdat-chunk-size-overflow.test similarity index 100% rename from testing/btest/scripts/base/protocols/bdat-chunk-size-overflow.test rename to testing/btest/scripts/base/protocols/smtp/bdat-chunk-size-overflow.test diff --git a/testing/btest/scripts/base/protocols/bdat-chunk-size-overflow2.test b/testing/btest/scripts/base/protocols/smtp/bdat-chunk-size-overflow2.test similarity index 100% rename from testing/btest/scripts/base/protocols/bdat-chunk-size-overflow2.test rename to testing/btest/scripts/base/protocols/smtp/bdat-chunk-size-overflow2.test diff --git a/testing/btest/scripts/base/protocols/bdat-negative-chunk-size.test b/testing/btest/scripts/base/protocols/smtp/bdat-negative-chunk-size.test similarity index 100% rename from testing/btest/scripts/base/protocols/bdat-negative-chunk-size.test rename to testing/btest/scripts/base/protocols/smtp/bdat-negative-chunk-size.test