From: Philippe Antoine <pantoine@oisf.net>
Date: Tue, 14 Nov 2023 20:51:37 +0000 (+0100)
Subject: smtp: avoid creating empty transaction
X-Git-Tag: suricata-8.0.0-beta1~1793
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=61f2e4e1e5b34dfd8ae44d1c15253e2da65f6e6a;p=thirdparty%2Fsuricata.git

smtp: avoid creating empty transaction

Ticket: 6477

So as to avoid ending up with too many empty transactions.

This happens when Suricata sees a DATA command in the current
transaction but did not have a confirmation response for it.
Then, if Suricata receives another DATA command, it will
create another new transaction, even if the previous one
is empty. And so, a malicious client can create many empty
transactions by just sending a repeated amount of DATA commands
without having a confirmation code for them.

Suricata cannot use state->current_command == SMTP_COMMAND_DATA
to prevent this attack and needs to resort to a new boolean
is_data because the malicious client may send another dummy command
after each DATA command.

This patch leaves only one call to SMTPTransactionCreate
---

diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c
index 20eb8f526a..c61421cf01 100644
--- a/src/app-layer-smtp.c
+++ b/src/app-layer-smtp.c
@@ -819,7 +819,7 @@ static inline void SMTPTransactionComplete(SMTPState *state)
 {
     DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL);
     if (state->curr_tx)
-        state->curr_tx->done = 1;
+        state->curr_tx->done = true;
 }
 
 /**
@@ -1215,36 +1215,19 @@ static int SMTPProcessRequest(SMTPState *state, Flow *f, AppLayerParserState *ps
             state->current_command = SMTP_COMMAND_STARTTLS;
         } else if (line->len >= 4 && SCMemcmpLowercase("data", line->buf, 4) == 0) {
             state->current_command = SMTP_COMMAND_DATA;
-            if (smtp_config.raw_extraction) {
-                if (state->tx_cnt > 1 && !state->curr_tx->done) {
-                    // we did not close the previous tx, set error
-                    SMTPSetEvent(state, SMTP_DECODER_EVENT_UNPARSABLE_CONTENT);
-                    FileCloseFile(&tx->files_ts, &smtp_config.sbcfg, NULL, 0, FILE_TRUNCATED);
-                    tx = SMTPTransactionCreate(state);
-                    if (tx == NULL)
-                        return -1;
-                    state->curr_tx = tx;
-                    TAILQ_INSERT_TAIL(&state->tx_list, tx, next);
-                    tx->tx_id = state->tx_cnt++;
-                }
+            if (state->curr_tx->is_data) {
+                // We did not receive a confirmation from server
+                // And now client sends a next DATA
+                SMTPSetEvent(state, SMTP_DECODER_EVENT_UNPARSABLE_CONTENT);
+                SCReturnInt(0);
+            } else if (smtp_config.raw_extraction) {
                 if (FileOpenFileWithId(&tx->files_ts, &smtp_config.sbcfg, state->file_track_id++,
                             (uint8_t *)rawmsgname, strlen(rawmsgname), NULL, 0,
                             FILE_NOMD5 | FILE_NOMAGIC) == 0) {
                     SMTPNewFile(tx, tx->files_ts.tail);
                 }
             } else if (smtp_config.decode_mime) {
-                if (tx->mime_state) {
-                    /* We have 2 chained mails and did not detect the end
-                     * of first one. So we start a new transaction. */
-                    tx->mime_state->state_flag = PARSE_ERROR;
-                    SMTPSetEvent(state, SMTP_DECODER_EVENT_UNPARSABLE_CONTENT);
-                    tx = SMTPTransactionCreate(state);
-                    if (tx == NULL)
-                        return -1;
-                    state->curr_tx = tx;
-                    TAILQ_INSERT_TAIL(&state->tx_list, tx, next);
-                    tx->tx_id = state->tx_cnt++;
-                }
+                DEBUG_VALIDATE_BUG_ON(tx->mime_state);
                 tx->mime_state = MimeDecInitParser(f, SMTPProcessDataChunk);
                 if (tx->mime_state == NULL) {
                     return MIME_DEC_ERR_MEM;
@@ -1260,6 +1243,7 @@ static int SMTPProcessRequest(SMTPState *state, Flow *f, AppLayerParserState *ps
                     tx->msg_tail = tx->mime_state->msg;
                 }
             }
+            state->curr_tx->is_data = true;
             /* Enter immediately data mode without waiting for server reply */
             if (state->parser_state & SMTP_PARSER_STATE_PIPELINING_SERVER) {
                 state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE;
diff --git a/src/app-layer-smtp.h b/src/app-layer-smtp.h
index 7977922eba..33b81d026a 100644
--- a/src/app-layer-smtp.h
+++ b/src/app-layer-smtp.h
@@ -75,7 +75,12 @@ typedef struct SMTPTransaction_ {
 
     AppLayerTxData tx_data;
 
-    int done;
+    /** the tx is complete and can be logged and cleaned */
+    bool done;
+    /** the tx has seen a DATA command */
+    // another DATA command within the same context
+    // will trigger an app-layer event.
+    bool is_data;
     /** the first message contained in the session */
     MimeDecEntity *msg_head;
     /** the last message contained in the session */