Merge remote-tracking branch 'origin/topic/awelzel/smtp-bdat-follow-up-3'

* origin/topic/awelzel/smtp-bdat-follow-up-3:
  btest/smtp/bdat: Move tests into proper directory
  BDAT: Harden parse_bdat_arg()
  SMTP: Reset ContentLineAnalyzer plain delivery on EndData()
  SMTP: Add SMTP_IN_BDAT state

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)

VERSION

@@ -1 +1 @@
-6.2.0-dev.451
+6.2.0-dev.456

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<std::string::size_type>(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<char[]>(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");

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<detail::SMTP_BDAT_Analyzer>(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();
     }

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);

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

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