diff --git a/CHANGES b/CHANGES index c326132f58..5aeeff15b1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,30 @@ +6.2.0-dev.456 | 2024-01-23 21:53:07 +0100 + + * btest/smtp/bdat: Move tests into proper directory (Arne Welzel, Corelight) + + * BDAT: Harden parse_bdat_arg() (Arne Welzel, Corelight) + + 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. + + * SMTP: Reset ContentLineAnalyzer plain delivery on EndData() (Arne Welzel, Corelight) + + 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. + + * SMTP: Add SMTP_IN_BDAT state (Arne Welzel, Corelight) + + 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. + 6.2.0-dev.451 | 2024-01-23 12:40:53 -0700 * Remove setting non-existent session history for IPTunnel (Tim Wojtulewicz, Corelight) diff --git a/VERSION b/VERSION index bb262889e5..494fa0ecff 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.2.0-dev.451 +6.2.0-dev.456 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/src/analyzer/protocol/smtp/SMTP.cc b/src/analyzer/protocol/smtp/SMTP.cc index 8fa3dd2c98..e717742cdc 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"); @@ -899,6 +899,12 @@ void SMTP_Analyzer::BeginData(bool orig) { 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(); } 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); 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 87% 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 index 2129c89e16..efc3169f89 100644 --- 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 @@ -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 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