From 40b6e6dacf394488d782899d58e17d71dcb02057 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 30 Jun 2026 12:16:26 -0600 Subject: [PATCH 01/11] smtp: add firewall progress states Add minimal SMTP progress states to support envelope validation before moving to data. Update SMTP, file and email keywords to hook into the appropriate states. Purposefully kept minimal for now as to not break the current idea of an SMTP transaction, which is probably not ideal for firewall mode. Ticket: #8393 (cherry picked from commit c2728eee017c26f639e230394651d5377c2836b6) --- src/app-layer-smtp.c | 61 ++++++++++++++++++++++++++++++++++++++++-- src/app-layer-smtp.h | 16 +++++++++++ src/detect-email.c | 43 ++++++++++++++++------------- src/detect-file-data.c | 5 +++- 4 files changed, 104 insertions(+), 21 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index 7938a94a0429..3c58bce83b74 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -186,6 +186,52 @@ static const char *SMTPGetFrameNameById(const uint8_t frame_id) return name; } +static SCEnumCharMap smtp_state_client_table[] = { + { "request_started", SMTP_REQUEST_STARTED }, + { "request_data", SMTP_REQUEST_DATA }, + { "request_complete", SMTP_REQUEST_COMPLETE }, + { NULL, -1 }, +}; + +static SCEnumCharMap smtp_state_server_table[] = { + { "response_started", SMTP_RESPONSE_STARTED }, + { "response_data", SMTP_RESPONSE_DATA }, + { "response_complete", SMTP_RESPONSE_COMPLETE }, + { NULL, -1 }, +}; + +static int SMTPStateGetStateIdByName(const char *name, const uint8_t direction) +{ + SCEnumCharMap *map = + direction == STREAM_TOSERVER ? smtp_state_client_table : smtp_state_server_table; + int id = SCMapEnumNameToValue(name, map); + if (id < 0) { + return -1; + } + return id; +} + +static const char *SMTPStateGetStateNameById(const int id, const uint8_t direction) +{ + SCEnumCharMap *map = + direction == STREAM_TOSERVER ? smtp_state_client_table : smtp_state_server_table; + return SCMapEnumValueToName(id, map); +} + +static inline void SMTPSetProgressTS(SMTPTransaction *tx, uint8_t progress) +{ + if (tx != NULL && tx->progress_ts < progress) { + tx->progress_ts = progress; + } +} + +static inline void SMTPSetProgressTC(SMTPTransaction *tx, uint8_t progress) +{ + if (tx != NULL && tx->progress_tc < progress) { + tx->progress_tc = progress; + } +} + typedef struct SMTPThreadCtx_ { MpmThreadCtx *smtp_mpm_thread_ctx; PrefilterRuleStore *pmq; @@ -940,6 +986,7 @@ static int SMTPProcessReply( } } else if (IsReplyToCommand(state, SMTP_COMMAND_DATA)) { if (reply_code == SMTP_REPLY_354) { + SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_DATA); /* Next comes the mail for the DATA command in toserver direction */ state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; } else { @@ -950,6 +997,8 @@ static int SMTPProcessReply( } SMTPSetEvent(state, SMTP_DECODER_EVENT_DATA_COMMAND_REJECTED); } + } else if (IsReplyToCommand(state, SMTP_COMMAND_BDAT)) { + SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_DATA); } else if (IsReplyToCommand(state, SMTP_COMMAND_RSET)) { if (reply_code == SMTP_REPLY_250 && state->curr_tx && !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { @@ -1185,6 +1234,7 @@ static int SMTPProcessRequest( state->current_command = SMTP_COMMAND_STARTTLS; } else if (line->len >= 4 && SCMemcmpLowercase("data", line->buf, 4) == 0) { state->current_command = SMTP_COMMAND_DATA; + SMTPSetProgressTS(tx, SMTP_REQUEST_DATA); if (state->curr_tx->is_data) { // We did not receive a confirmation from server // And now client sends a next DATA @@ -1225,6 +1275,7 @@ static int SMTPProcessRequest( SCReturnInt(-1); } state->current_command = SMTP_COMMAND_BDAT; + SMTPSetProgressTS(tx, SMTP_REQUEST_DATA); state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; } else if (line->len >= 4 && ((SCMemcmpLowercase("helo", line->buf, 4) == 0) || SCMemcmpLowercase("ehlo", line->buf, 4) == 0)) { @@ -1790,7 +1841,10 @@ static void *SMTPStateGetTx(void *state, uint64_t id) static int SMTPStateGetAlstateProgress(void *vtx, uint8_t direction) { SMTPTransaction *tx = vtx; - return tx->done; + if (direction & STREAM_TOSERVER) { + return tx->done ? SMTP_REQUEST_COMPLETE : tx->progress_ts; + } + return tx->done ? SMTP_RESPONSE_COMPLETE : tx->progress_tc; } static AppLayerGetFileState SMTPGetTxFiles(void *txv, uint8_t direction) @@ -1893,9 +1947,12 @@ void RegisterSMTPParsers(void) AppLayerParserRegisterGetTxIterator(IPPROTO_TCP, ALPROTO_SMTP, SMTPGetTxIterator); AppLayerParserRegisterTxDataFunc(IPPROTO_TCP, ALPROTO_SMTP, SMTPGetTxData); AppLayerParserRegisterStateDataFunc(IPPROTO_TCP, ALPROTO_SMTP, SMTPGetStateData); - AppLayerParserRegisterStateProgressCompletionStatus(ALPROTO_SMTP, 1, 1); + AppLayerParserRegisterStateProgressCompletionStatus( + ALPROTO_SMTP, SMTP_REQUEST_COMPLETE, SMTP_RESPONSE_COMPLETE); AppLayerParserRegisterGetFrameFuncs( IPPROTO_TCP, ALPROTO_SMTP, SMTPGetFrameIdByName, SMTPGetFrameNameById); + AppLayerParserRegisterGetStateFuncs( + IPPROTO_TCP, ALPROTO_SMTP, SMTPStateGetStateIdByName, SMTPStateGetStateNameById); } else { SCLogInfo("Parser disabled for %s protocol. Protocol detection still on.", proto_name); } diff --git a/src/app-layer-smtp.h b/src/app-layer-smtp.h index cd9c614b966a..3054ba1b761d 100644 --- a/src/app-layer-smtp.h +++ b/src/app-layer-smtp.h @@ -69,6 +69,18 @@ typedef struct SMTPString_ { TAILQ_ENTRY(SMTPString_) next; } SMTPString; +enum SMTPRequestProgress { + SMTP_REQUEST_STARTED = 0, + SMTP_REQUEST_DATA = 1, + SMTP_REQUEST_COMPLETE = 2, +}; + +enum SMTPResponseProgress { + SMTP_RESPONSE_STARTED = 0, + SMTP_RESPONSE_DATA = 1, + SMTP_RESPONSE_COMPLETE = 2, +}; + typedef struct SMTPTransaction_ { /** id of this tx, starting at 0 */ uint64_t tx_id; @@ -77,6 +89,10 @@ typedef struct SMTPTransaction_ { /** the tx is complete and can be logged and cleaned */ bool done; + /** to-server firewall progress state. */ + uint8_t progress_ts; + /** to-client firewall progress state. */ + uint8_t progress_tc; /** the tx has seen a DATA command */ // another DATA command within the same context // will trigger an app-layer event. diff --git a/src/detect-email.c b/src/detect-email.c index 26bd4974ce53..f5538c83dd5d 100644 --- a/src/detect-email.c +++ b/src/detect-email.c @@ -235,8 +235,9 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailFromSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_from_buffer_id = SCDetectHelperBufferMpmRegister( - "email.from", "MIME EMAIL FROM", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailFromData); + g_mime_email_from_buffer_id = + SCDetectHelperBufferProgressMpmRegister("email.from", "MIME EMAIL FROM", ALPROTO_SMTP, + STREAM_TOSERVER, GetMimeEmailFromData, SMTP_REQUEST_DATA); kw.name = "email.subject"; kw.desc = "'Subject' field from an email"; @@ -244,8 +245,9 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailSubjectSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_subject_buffer_id = SCDetectHelperBufferMpmRegister("email.subject", - "MIME EMAIL SUBJECT", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailSubjectData); + g_mime_email_subject_buffer_id = + SCDetectHelperBufferProgressMpmRegister("email.subject", "MIME EMAIL SUBJECT", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailSubjectData, SMTP_REQUEST_DATA); kw.name = "email.to"; kw.desc = "'To' field from an email"; @@ -253,8 +255,8 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailToSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_to_buffer_id = SCDetectHelperBufferMpmRegister( - "email.to", "MIME EMAIL TO", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailToData); + g_mime_email_to_buffer_id = SCDetectHelperBufferProgressMpmRegister("email.to", "MIME EMAIL TO", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailToData, SMTP_REQUEST_DATA); kw.name = "email.cc"; kw.desc = "'Cc' field from an email"; @@ -262,8 +264,8 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailCcSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_cc_buffer_id = SCDetectHelperBufferMpmRegister( - "email.cc", "MIME EMAIL CC", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailCcData); + g_mime_email_cc_buffer_id = SCDetectHelperBufferProgressMpmRegister("email.cc", "MIME EMAIL CC", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailCcData, SMTP_REQUEST_DATA); kw.name = "email.date"; kw.desc = "'Date' field from an email"; @@ -271,8 +273,9 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailDateSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_date_buffer_id = SCDetectHelperBufferMpmRegister( - "email.date", "MIME EMAIL DATE", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailDateData); + g_mime_email_date_buffer_id = + SCDetectHelperBufferProgressMpmRegister("email.date", "MIME EMAIL DATE", ALPROTO_SMTP, + STREAM_TOSERVER, GetMimeEmailDateData, SMTP_REQUEST_DATA); kw.name = "email.message_id"; kw.desc = "'Message-Id' field from an email"; @@ -280,8 +283,9 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailMessageIdSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_message_id_buffer_id = SCDetectHelperBufferMpmRegister("email.message_id", - "MIME EMAIL Message-Id", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailMessageIdData); + g_mime_email_message_id_buffer_id = + SCDetectHelperBufferProgressMpmRegister("email.message_id", "MIME EMAIL Message-Id", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailMessageIdData, SMTP_REQUEST_DATA); kw.name = "email.x_mailer"; kw.desc = "'X-Mailer' field from an email"; @@ -289,8 +293,9 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailXMailerSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_x_mailer_buffer_id = SCDetectHelperBufferMpmRegister("email.x_mailer", - "MIME EMAIL X-Mailer", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailXMailerData); + g_mime_email_x_mailer_buffer_id = + SCDetectHelperBufferProgressMpmRegister("email.x_mailer", "MIME EMAIL X-Mailer", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailXMailerData, SMTP_REQUEST_DATA); kw.name = "email.url"; kw.desc = "'Url' extracted from an email"; @@ -298,8 +303,9 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailUrlSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_url_buffer_id = SCDetectHelperMultiBufferMpmRegister( - "email.url", "MIME EMAIL URL", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailUrlData); + g_mime_email_url_buffer_id = + SCDetectHelperMultiBufferProgressMpmRegister("email.url", "MIME EMAIL URL", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailUrlData, SMTP_REQUEST_DATA); kw.name = "email.received"; kw.desc = "'Received' field from an email"; @@ -307,6 +313,7 @@ void DetectEmailRegister(void) kw.Setup = DetectMimeEmailReceivedSetup; kw.flags = SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; SCDetectHelperKeywordRegister(&kw); - g_mime_email_received_buffer_id = SCDetectHelperMultiBufferMpmRegister("email.received", - "MIME EMAIL RECEIVED", ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailReceivedData); + g_mime_email_received_buffer_id = + SCDetectHelperMultiBufferProgressMpmRegister("email.received", "MIME EMAIL RECEIVED", + ALPROTO_SMTP, STREAM_TOSERVER, GetMimeEmailReceivedData, SMTP_REQUEST_DATA); } diff --git a/src/detect-file-data.c b/src/detect-file-data.c index e5f28d8b9f4b..77f68de01ca1 100644 --- a/src/detect-file-data.c +++ b/src/detect-file-data.c @@ -93,7 +93,10 @@ DetectFileHandlerProtocol_t al_protocols[ALPROTO_WITHFILES_MAX] = { .direction = SIG_FLAG_TOSERVER | SIG_FLAG_TOCLIENT, .to_client_progress = HTTP2StateDataServer, .to_server_progress = HTTP2StateDataClient }, - { .alproto = ALPROTO_SMTP, .direction = SIG_FLAG_TOSERVER }, { .alproto = ALPROTO_UNKNOWN } + { .alproto = ALPROTO_SMTP, + .direction = SIG_FLAG_TOSERVER, + .to_server_progress = SMTP_REQUEST_DATA }, + { .alproto = ALPROTO_UNKNOWN } }; void DetectFileRegisterProto( From 2b3dd686d087a9743000a6eb5258b9ef95b3df81 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Fri, 3 Jul 2026 13:06:04 -0600 Subject: [PATCH 02/11] smtp: complete transactions by progress state Add directionality to completion states, and replace tx->done by checking for both directions being complete. This means that the transaction is now not complete until the server responds to the clients of data marker, previously the tx was completed when the client send end of data without waiting for the server response. This keeps smtp:response_complete from being exposed before the server response is parsed. Ticket: #8393 (cherry picked from commit 7b31f41878b557f121d471fe56f3ca0e94ec4a36) --- src/app-layer-smtp.c | 44 +++++++++++++++++++++++++++++++++++++------- src/app-layer-smtp.h | 2 -- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index 3c58bce83b74..dd230621332c 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -232,6 +232,12 @@ static inline void SMTPSetProgressTC(SMTPTransaction *tx, uint8_t progress) } } +static bool SMTPTransactionIsComplete(const SMTPTransaction *tx) +{ + return tx && tx->progress_ts == SMTP_REQUEST_COMPLETE && + tx->progress_tc == SMTP_RESPONSE_COMPLETE; +} + typedef struct SMTPThreadCtx_ { MpmThreadCtx *smtp_mpm_thread_ctx; PrefilterRuleStore *pmq; @@ -755,8 +761,28 @@ static void SetMimeEvents(SMTPState *state, uint32_t events) static inline void SMTPTransactionComplete(SMTPState *state) { DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL); - if (state->curr_tx) - state->curr_tx->done = true; + if (state->curr_tx) { + SMTPSetProgressTS(state->curr_tx, SMTP_REQUEST_COMPLETE); + SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_COMPLETE); + } +} + +static inline void SMTPTransactionCompleteTS(SMTPState *state) +{ + DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL); + if (state->curr_tx) { + SMTPSetProgressTS(state->curr_tx, SMTP_REQUEST_COMPLETE); + SCLogDebug("marked tx as ts complete"); + } +} + +static inline void SMTPTransactionCompleteTC(SMTPState *state) +{ + DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL); + if (state->curr_tx) { + SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_COMPLETE); + SCLogDebug("marked tx as tc complete"); + } } /** @@ -793,8 +819,7 @@ static int SMTPProcessCommandDATA( FileFlowToFlags(f, STREAM_TOSERVER)); } } - SMTPTransactionComplete(state); - SCLogDebug("marked tx as done"); + SMTPTransactionCompleteTS(state); } else if (smtp_config.raw_extraction) { // message not over, store the line. This is a substitution of // ProcessDataChunk @@ -999,6 +1024,10 @@ static int SMTPProcessReply( } } else if (IsReplyToCommand(state, SMTP_COMMAND_BDAT)) { SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_DATA); + } else if (IsReplyToCommand(state, SMTP_COMMAND_DATA_MODE)) { + if (!(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { + SMTPTransactionCompleteTC(state); + } } else if (IsReplyToCommand(state, SMTP_COMMAND_RSET)) { if (reply_code == SMTP_REPLY_250 && state->curr_tx && !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { @@ -1200,7 +1229,8 @@ static int SMTPProcessRequest( if (line->len == 0 && line->delim_len == 0) { return 0; } - if (state->curr_tx == NULL || (state->curr_tx->done && !NoNewTx(state, line))) { + if (state->curr_tx == NULL || + (SMTPTransactionIsComplete(state->curr_tx) && !NoNewTx(state, line))) { tx = SMTPTransactionCreate(state); if (tx == NULL) return -1; @@ -1842,9 +1872,9 @@ static int SMTPStateGetAlstateProgress(void *vtx, uint8_t direction) { SMTPTransaction *tx = vtx; if (direction & STREAM_TOSERVER) { - return tx->done ? SMTP_REQUEST_COMPLETE : tx->progress_ts; + return tx->progress_ts; } - return tx->done ? SMTP_RESPONSE_COMPLETE : tx->progress_tc; + return tx->progress_tc; } static AppLayerGetFileState SMTPGetTxFiles(void *txv, uint8_t direction) diff --git a/src/app-layer-smtp.h b/src/app-layer-smtp.h index 3054ba1b761d..7dd05d1235eb 100644 --- a/src/app-layer-smtp.h +++ b/src/app-layer-smtp.h @@ -87,8 +87,6 @@ typedef struct SMTPTransaction_ { AppLayerTxData tx_data; - /** the tx is complete and can be logged and cleaned */ - bool done; /** to-server firewall progress state. */ uint8_t progress_ts; /** to-client firewall progress state. */ From 02cf3806edabc0222cf312c0887bd2e2d9809d7d Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 7 Jul 2026 15:41:02 -0600 Subject: [PATCH 03/11] smtp: handle pipelined replies on owning tx Track the transaction id for each queued SMTP command so replies can update the transaction that created the command instead of always using the current transaction. Ticket: #8393 (cherry picked from commit e2a62dd1c086fa2ed6ddf544366d5f2a1d6d4c93) --- src/app-layer-smtp.c | 121 +++++++++++++++++++++++++++++++------------ src/app-layer-smtp.h | 2 + 2 files changed, 91 insertions(+), 32 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index dd230621332c..84072aeb0d1e 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -98,6 +98,9 @@ #define SMTP_DEFAULT_MAX_TX 256 +/* command buffer tx id for commands with no owning transaction */ +#define SMTP_NO_TX_ID UINT64_MAX + typedef struct SMTPInput_ { /* current input that is being parsed */ const uint8_t *buf; @@ -229,13 +232,13 @@ static inline void SMTPSetProgressTC(SMTPTransaction *tx, uint8_t progress) { if (tx != NULL && tx->progress_tc < progress) { tx->progress_tc = progress; + tx->tx_data.updated_tc = true; } } -static bool SMTPTransactionIsComplete(const SMTPTransaction *tx) +static bool SMTPTransactionRequestIsComplete(const SMTPTransaction *tx) { - return tx && tx->progress_ts == SMTP_REQUEST_COMPLETE && - tx->progress_tc == SMTP_RESPONSE_COMPLETE; + return tx && tx->progress_ts == SMTP_REQUEST_COMPLETE; } typedef struct SMTPThreadCtx_ { @@ -665,7 +668,8 @@ static AppLayerResult SMTPGetLine(Flow *f, StreamSlice *slice, SMTPState *state, } } -static int SMTPInsertCommandIntoCommandBuffer(uint8_t command, SMTPState *state) +static int SMTPInsertCommandIntoCommandBuffer( + SMTPState *state, uint8_t command, const SMTPTransaction *tx) { SCEnter(); void *ptmp; @@ -680,12 +684,26 @@ static int SMTPInsertCommandIntoCommandBuffer(uint8_t command, SMTPState *state) sizeof(uint8_t) * (state->cmds_buffer_len + increment)); if (ptmp == NULL) { SCFree(state->cmds); + SCFree(state->cmds_tx_ids); state->cmds = NULL; + state->cmds_tx_ids = NULL; SCLogDebug("SCRealloc failure"); return -1; } state->cmds = ptmp; + ptmp = SCRealloc( + state->cmds_tx_ids, sizeof(uint64_t) * (state->cmds_buffer_len + increment)); + if (ptmp == NULL) { + SCFree(state->cmds); + SCFree(state->cmds_tx_ids); + state->cmds = NULL; + state->cmds_tx_ids = NULL; + SCLogDebug("SCRealloc failure"); + return -1; + } + state->cmds_tx_ids = ptmp; + state->cmds_buffer_len += increment; } if (state->cmds_cnt >= 1 && @@ -704,6 +722,7 @@ static int SMTPInsertCommandIntoCommandBuffer(uint8_t command, SMTPState *state) } state->cmds[state->cmds_cnt] = command; + state->cmds_tx_ids[state->cmds_cnt] = tx != NULL ? tx->tx_id : SMTP_NO_TX_ID; state->cmds_cnt++; return 0; @@ -758,29 +777,29 @@ static void SetMimeEvents(SMTPState *state, uint32_t events) } } -static inline void SMTPTransactionComplete(SMTPState *state) +static inline void SMTPTransactionComplete(SMTPTransaction *tx) { - DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL); - if (state->curr_tx) { - SMTPSetProgressTS(state->curr_tx, SMTP_REQUEST_COMPLETE); - SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_COMPLETE); + DEBUG_VALIDATE_BUG_ON(tx == NULL); + if (tx) { + SMTPSetProgressTS(tx, SMTP_REQUEST_COMPLETE); + SMTPSetProgressTC(tx, SMTP_RESPONSE_COMPLETE); } } -static inline void SMTPTransactionCompleteTS(SMTPState *state) +static inline void SMTPTransactionCompleteTS(SMTPTransaction *tx) { - DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL); - if (state->curr_tx) { - SMTPSetProgressTS(state->curr_tx, SMTP_REQUEST_COMPLETE); + DEBUG_VALIDATE_BUG_ON(tx == NULL); + if (tx) { + SMTPSetProgressTS(tx, SMTP_REQUEST_COMPLETE); SCLogDebug("marked tx as ts complete"); } } -static inline void SMTPTransactionCompleteTC(SMTPState *state) +static inline void SMTPTransactionCompleteTC(SMTPTransaction *tx) { - DEBUG_VALIDATE_BUG_ON(state->curr_tx == NULL); - if (state->curr_tx) { - SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_COMPLETE); + DEBUG_VALIDATE_BUG_ON(tx == NULL); + if (tx) { + SMTPSetProgressTC(tx, SMTP_RESPONSE_COMPLETE); SCLogDebug("marked tx as tc complete"); } } @@ -807,7 +826,7 @@ static int SMTPProcessCommandDATA( * acknowledged with a reply. We insert a dummy command to * the command buffer to be used by the reply handler to match * the reply received */ - SMTPInsertCommandIntoCommandBuffer(SMTP_COMMAND_DATA_MODE, state); + SMTPInsertCommandIntoCommandBuffer(state, SMTP_COMMAND_DATA_MODE, tx); if (smtp_config.raw_extraction) { /* we use this as the signal that message data is complete. */ FileCloseFile(&tx->files_ts, &smtp_config.sbcfg, NULL, 0, 0); @@ -819,7 +838,7 @@ static int SMTPProcessCommandDATA( FileFlowToFlags(f, STREAM_TOSERVER)); } } - SMTPTransactionCompleteTS(state); + SMTPTransactionCompleteTS(tx); } else if (smtp_config.raw_extraction) { // message not over, store the line. This is a substitution of // ProcessDataChunk @@ -916,8 +935,35 @@ static int SMTPProcessCommandDATA( static inline bool IsReplyToCommand(const SMTPState *state, const uint8_t cmd) { - return (state->cmds_idx < state->cmds_buffer_len && - state->cmds[state->cmds_idx] == cmd); + return (state->cmds_idx < state->cmds_cnt && state->cmds[state->cmds_idx] == cmd); +} + +static SMTPTransaction *SMTPStateGetTxById(SMTPState *state, uint64_t tx_id) +{ + SMTPTransaction *tx = NULL; + TAILQ_FOREACH (tx, &state->tx_list, next) { + if (tx->tx_id == tx_id) { + return tx; + } + if (tx->tx_id > tx_id) { + break; + } + } + return NULL; +} + +static SMTPTransaction *SMTPGetReplyTx(SMTPState *state) +{ + if (state->cmds_idx >= state->cmds_cnt) { + return state->curr_tx; + } + + /* a command with no owning tx, or whose tx is gone, must not resolve + * to another tx */ + if (state->cmds_tx_ids[state->cmds_idx] == SMTP_NO_TX_ID) { + return NULL; + } + return SMTPStateGetTxById(state, state->cmds_tx_ids[state->cmds_idx]); } static int SMTPProcessReply( @@ -930,8 +976,9 @@ static int SMTPProcessReply( return 0; // to continue processing further } - if (state->curr_tx) { - state->curr_tx->tx_data.updated_tc = true; + SMTPTransaction *reply_tx = SMTPGetReplyTx(state); + if (reply_tx != NULL) { + reply_tx->tx_data.updated_tc = true; } /* the reply code has to contain at least 3 bytes, to hold the 3 digit * reply code */ @@ -1002,8 +1049,8 @@ static int SMTPProcessReply( if (!SCAppLayerRequestProtocolTLSUpgrade(f)) { SMTPSetEvent(state, SMTP_DECODER_EVENT_FAILED_PROTOCOL_CHANGE); } - if (state->curr_tx) { - SMTPTransactionComplete(state); + if (reply_tx) { + SMTPTransactionComplete(reply_tx); } } else { /* decoder event */ @@ -1011,7 +1058,7 @@ static int SMTPProcessReply( } } else if (IsReplyToCommand(state, SMTP_COMMAND_DATA)) { if (reply_code == SMTP_REPLY_354) { - SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_DATA); + SMTPSetProgressTC(reply_tx, SMTP_RESPONSE_DATA); /* Next comes the mail for the DATA command in toserver direction */ state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; } else { @@ -1023,15 +1070,15 @@ static int SMTPProcessReply( SMTPSetEvent(state, SMTP_DECODER_EVENT_DATA_COMMAND_REJECTED); } } else if (IsReplyToCommand(state, SMTP_COMMAND_BDAT)) { - SMTPSetProgressTC(state->curr_tx, SMTP_RESPONSE_DATA); + SMTPSetProgressTC(reply_tx, SMTP_RESPONSE_DATA); } else if (IsReplyToCommand(state, SMTP_COMMAND_DATA_MODE)) { if (!(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { - SMTPTransactionCompleteTC(state); + SMTPTransactionCompleteTC(reply_tx); } } else if (IsReplyToCommand(state, SMTP_COMMAND_RSET)) { - if (reply_code == SMTP_REPLY_250 && state->curr_tx && + if (reply_code == SMTP_REPLY_250 && reply_tx && !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { - SMTPTransactionComplete(state); + SMTPTransactionComplete(reply_tx); } } else { /* we don't care for any other command for now */ @@ -1230,7 +1277,7 @@ static int SMTPProcessRequest( return 0; } if (state->curr_tx == NULL || - (SMTPTransactionIsComplete(state->curr_tx) && !NoNewTx(state, line))) { + (SMTPTransactionRequestIsComplete(state->curr_tx) && !NoNewTx(state, line))) { tx = SMTPTransactionCreate(state); if (tx == NULL) return -1; @@ -1336,7 +1383,7 @@ static int SMTPProcessRequest( /* Every command is inserted into a command buffer, to be matched * against reply(ies) sent by the server */ - if (SMTPInsertCommandIntoCommandBuffer(state->current_command, state) == -1) { + if (SMTPInsertCommandIntoCommandBuffer(state, state->current_command, tx) == -1) { SCReturnInt(-1); } @@ -1586,6 +1633,12 @@ void *SMTPStateAlloc(void *orig_state, AppProto proto_orig) SCFree(smtp_state); return NULL; } + smtp_state->cmds_tx_ids = SCMalloc(sizeof(uint64_t) * SMTP_COMMAND_BUFFER_STEPS); + if (smtp_state->cmds_tx_ids == NULL) { + SCFree(smtp_state->cmds); + SCFree(smtp_state); + return NULL; + } smtp_state->cmds_buffer_len = SMTP_COMMAND_BUFFER_STEPS; TAILQ_INIT(&smtp_state->tx_list); @@ -1683,6 +1736,9 @@ static void SMTPStateFree(void *p) if (smtp_state->cmds != NULL) { SCFree(smtp_state->cmds); } + if (smtp_state->cmds_tx_ids != NULL) { + SCFree(smtp_state->cmds_tx_ids); + } if (smtp_state->helo) { SCFree(smtp_state->helo); @@ -4327,6 +4383,7 @@ static int SMTPParserTest14(void) FLOW_DESTROY(&f); return result; } + #endif /* UNITTESTS */ void SMTPParserRegisterTests(void) diff --git a/src/app-layer-smtp.h b/src/app-layer-smtp.h index 7dd05d1235eb..c455eca777a3 100644 --- a/src/app-layer-smtp.h +++ b/src/app-layer-smtp.h @@ -152,6 +152,8 @@ typedef struct SMTPState_ { * stored command in the buffer to match the reply(ies) with the command */ /** the command buffer */ uint8_t *cmds; + /** tx id for each stored command */ + uint64_t *cmds_tx_ids; /** the buffer length */ uint16_t cmds_buffer_len; /** no of commands stored in the above buffer */ From 837e9f81396d63b1355c8e1a4a7c2f5a83c7d7f3 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Wed, 8 Jul 2026 16:40:25 -0600 Subject: [PATCH 04/11] smtp: don't create transaction for trailing quit Also ensures that a quit or rset without a helo still creates a tx. Ticket: #8728 (cherry picked from commit 842b14ee1f716874a134cd9266944978b22a59c2) --- src/app-layer-smtp.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index 84072aeb0d1e..8ef9f6031913 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -95,6 +95,7 @@ /* All other commands are represented by this var */ #define SMTP_COMMAND_OTHER_CMD 5 #define SMTP_COMMAND_RSET 6 +#define SMTP_COMMAND_QUIT 7 #define SMTP_DEFAULT_MAX_TX 256 @@ -1080,6 +1081,11 @@ static int SMTPProcessReply( !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { SMTPTransactionComplete(reply_tx); } + } else if (IsReplyToCommand(state, SMTP_COMMAND_QUIT)) { + if (reply_code == SMTP_REPLY_221 && reply_tx && + !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { + SMTPTransactionComplete(reply_tx); + } } else { /* we don't care for any other command for now */ } @@ -1276,8 +1282,9 @@ static int SMTPProcessRequest( if (line->len == 0 && line->delim_len == 0) { return 0; } - if (state->curr_tx == NULL || - (SMTPTransactionRequestIsComplete(state->curr_tx) && !NoNewTx(state, line))) { + const bool no_new_tx = NoNewTx(state, line); + if ((state->curr_tx == NULL && (state->tx_cnt == 0 || !no_new_tx)) || + (SMTPTransactionRequestIsComplete(state->curr_tx) && !no_new_tx)) { tx = SMTPTransactionCreate(state); if (tx == NULL) return -1; @@ -1293,7 +1300,9 @@ static int SMTPProcessRequest( if (frame != NULL && state->curr_tx) { AppLayerFrameSetTxId(frame, state->curr_tx->tx_id); } - tx->tx_data.updated_ts = true; + if (tx != NULL) { + tx->tx_data.updated_ts = true; + } state->toserver_data_count += (line->len + line->delim_len); @@ -1377,6 +1386,8 @@ static int SMTPProcessRequest( // Resets chunk index in case of connection reuse state->bdat_chunk_idx = 0; state->current_command = SMTP_COMMAND_RSET; + } else if (line->len >= 4 && SCMemcmpLowercase("quit", line->buf, 4) == 0) { + state->current_command = SMTP_COMMAND_QUIT; } else { state->current_command = SMTP_COMMAND_OTHER_CMD; } @@ -2851,7 +2862,7 @@ static int SMTPParserTest02(void) goto end; } if (smtp_state->cmds_cnt != 1 || smtp_state->cmds_idx != 0 || - smtp_state->cmds[0] != SMTP_COMMAND_OTHER_CMD || + smtp_state->cmds[0] != SMTP_COMMAND_QUIT || smtp_state->parser_state != SMTP_PARSER_STATE_FIRST_REPLY_SEEN) { printf("smtp parser in inconsistent state\n"); goto end; @@ -3333,7 +3344,7 @@ static int SMTPParserTest05(void) goto end; } if (smtp_state->cmds_cnt != 1 || smtp_state->cmds_idx != 0 || - smtp_state->cmds[0] != SMTP_COMMAND_OTHER_CMD || + smtp_state->cmds[0] != SMTP_COMMAND_QUIT || smtp_state->parser_state != (SMTP_PARSER_STATE_FIRST_REPLY_SEEN | SMTP_PARSER_STATE_PIPELINING_SERVER)) { printf("smtp parser in inconsistent state\n"); @@ -4356,7 +4367,7 @@ static int SMTPParserTest14(void) goto end; } if (smtp_state->cmds_cnt != 1 || smtp_state->cmds_idx != 0 || - smtp_state->cmds[0] != SMTP_COMMAND_OTHER_CMD || + smtp_state->cmds[0] != SMTP_COMMAND_QUIT || smtp_state->parser_state != SMTP_PARSER_STATE_FIRST_REPLY_SEEN) { printf("smtp parser in inconsistent state l.%d\n", __LINE__); goto end; From 348797487209195a9b92282fac0d91e6a5c8eba8 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Mon, 13 Jul 2026 11:16:17 -0600 Subject: [PATCH 05/11] smtp: check transaction before to-client completion Make sure the transaction still exists before completing it in the to-client direction. A pipelined RSET reply may already have completed and freed it while a later end-of-DATA marker still refers to it. Found by OSS-Fuzz testcase 5498180758994944. Bug #8739. (cherry picked from commit 62fdb771b0ce67bda80d58b156661980eb9f56e9) --- src/app-layer-smtp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index 8ef9f6031913..da0090564bf3 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -1073,7 +1073,7 @@ static int SMTPProcessReply( } else if (IsReplyToCommand(state, SMTP_COMMAND_BDAT)) { SMTPSetProgressTC(reply_tx, SMTP_RESPONSE_DATA); } else if (IsReplyToCommand(state, SMTP_COMMAND_DATA_MODE)) { - if (!(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { + if (reply_tx && !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { SMTPTransactionCompleteTC(reply_tx); } } else if (IsReplyToCommand(state, SMTP_COMMAND_RSET)) { From 9f7602aa1694b211f48f372d9059249a625fa4c7 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Mon, 13 Jul 2026 12:08:55 -0600 Subject: [PATCH 06/11] smtp: move transaction completion helpers Simply makes the follow diff a little easier to read. (cherry picked from commit 2676d1bc83d18e6219bd8d45068efe5654d56629) --- src/app-layer-smtp.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index da0090564bf3..b3a61f850307 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -237,6 +237,24 @@ static inline void SMTPSetProgressTC(SMTPTransaction *tx, uint8_t progress) } } +static inline void SMTPTransactionCompleteTS(SMTPTransaction *tx) +{ + DEBUG_VALIDATE_BUG_ON(tx == NULL); + if (tx) { + SMTPSetProgressTS(tx, SMTP_REQUEST_COMPLETE); + SCLogDebug("marked tx as ts complete"); + } +} + +static inline void SMTPTransactionCompleteTC(SMTPTransaction *tx) +{ + DEBUG_VALIDATE_BUG_ON(tx == NULL); + if (tx) { + SMTPSetProgressTC(tx, SMTP_RESPONSE_COMPLETE); + SCLogDebug("marked tx as tc complete"); + } +} + static bool SMTPTransactionRequestIsComplete(const SMTPTransaction *tx) { return tx && tx->progress_ts == SMTP_REQUEST_COMPLETE; @@ -787,24 +805,6 @@ static inline void SMTPTransactionComplete(SMTPTransaction *tx) } } -static inline void SMTPTransactionCompleteTS(SMTPTransaction *tx) -{ - DEBUG_VALIDATE_BUG_ON(tx == NULL); - if (tx) { - SMTPSetProgressTS(tx, SMTP_REQUEST_COMPLETE); - SCLogDebug("marked tx as ts complete"); - } -} - -static inline void SMTPTransactionCompleteTC(SMTPTransaction *tx) -{ - DEBUG_VALIDATE_BUG_ON(tx == NULL); - if (tx) { - SMTPSetProgressTC(tx, SMTP_RESPONSE_COMPLETE); - SCLogDebug("marked tx as tc complete"); - } -} - /** * \retval 0 ok * \retval -1 error From 3128bc67664c4374008c7ccc0de4de06b1538711 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 21 Jul 2026 16:51:26 -0600 Subject: [PATCH 07/11] smtp: complete BDAT transactions at LAST Track the BDAT LAST marker so the final chunk and its reply complete the transaction in each direction, preventing a following MAIL FROM from being merged into the previous transaction. Ticket: #8741 (cherry picked from commit ec0fec1a37f9fc949f2214cf1a43e8c9b1147248) --- src/app-layer-smtp.c | 105 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 84 insertions(+), 21 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index b3a61f850307..a3a5140da54e 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -96,6 +96,8 @@ #define SMTP_COMMAND_OTHER_CMD 5 #define SMTP_COMMAND_RSET 6 #define SMTP_COMMAND_QUIT 7 +/* Pseudo command used to match the final BDAT reply to its transaction. */ +#define SMTP_COMMAND_BDAT_LAST 8 #define SMTP_DEFAULT_MAX_TX 256 @@ -747,7 +749,7 @@ static int SMTPInsertCommandIntoCommandBuffer( return 0; } -static int SMTPProcessCommandBDAT(SMTPState *state, const SMTPLine *line) +static int SMTPProcessCommandBDAT(SMTPState *state, SMTPTransaction *tx, const SMTPLine *line) { SCEnter(); @@ -759,6 +761,9 @@ static int SMTPProcessCommandBDAT(SMTPState *state, const SMTPLine *line) SCReturnInt(-1); } else if (state->bdat_chunk_idx == state->bdat_chunk_len) { state->parser_state &= ~SMTP_PARSER_STATE_COMMAND_DATA_MODE; + if (state->current_command == SMTP_COMMAND_BDAT_LAST) { + SMTPTransactionCompleteTS(tx); + } } SCReturnInt(0); @@ -1072,6 +1077,10 @@ static int SMTPProcessReply( } } else if (IsReplyToCommand(state, SMTP_COMMAND_BDAT)) { SMTPSetProgressTC(reply_tx, SMTP_RESPONSE_DATA); + } else if (IsReplyToCommand(state, SMTP_COMMAND_BDAT_LAST)) { + if (reply_tx && !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { + SMTPTransactionCompleteTC(reply_tx); + } } else if (IsReplyToCommand(state, SMTP_COMMAND_DATA_MODE)) { if (reply_tx && !(state->parser_state & SMTP_PARSER_STATE_PARSING_MULTILINE_REPLY)) { SMTPTransactionCompleteTC(reply_tx); @@ -1113,10 +1122,12 @@ static int SMTPProcessReply( return 0; } -static int SMTPParseCommandBDAT(SMTPState *state, const SMTPLine *line) +static int SMTPParseCommandBDAT(SMTPState *state, const SMTPLine *line, bool *last) { SCEnter(); + *last = false; + int i = 4; while (i < line->len) { if (line->buf[i] != ' ') { @@ -1140,10 +1151,25 @@ static int SMTPParseCommandBDAT(SMTPState *state, const SMTPLine *line) } memcpy(strbuf, line->buf + i, len); strbuf[len] = '\0'; - if (ByteExtractStringUint32(&state->bdat_chunk_len, 10, 0, strbuf) < 0) { + int parsed = ByteExtractStringUint32(&state->bdat_chunk_len, 10, 0, strbuf); + if (parsed < 0) { /* decoder event */ return -1; } + state->bdat_chunk_idx = 0; + + i += parsed; + if (i < line->len && line->buf[i] != ' ') { + return -1; + } + while (i < line->len && line->buf[i] == ' ') { + i++; + } + if (line->len - i == 4 && SCMemcmpLowercase("last", line->buf + i, 4) == 0) { + *last = true; + } else if (i != line->len) { + return -1; + } return 0; } @@ -1356,13 +1382,18 @@ static int SMTPProcessRequest( state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; } } else if (line->len >= 4 && SCMemcmpLowercase("bdat", line->buf, 4) == 0) { - r = SMTPParseCommandBDAT(state, line); + bool last = false; + r = SMTPParseCommandBDAT(state, line, &last); if (r == -1) { SCReturnInt(-1); } - state->current_command = SMTP_COMMAND_BDAT; + state->current_command = last ? SMTP_COMMAND_BDAT_LAST : SMTP_COMMAND_BDAT; SMTPSetProgressTS(tx, SMTP_REQUEST_DATA); - state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; + if (state->bdat_chunk_len > 0) { + state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; + } else if (last) { + SMTPTransactionCompleteTS(tx); + } } else if (line->len >= 4 && ((SCMemcmpLowercase("helo", line->buf, 4) == 0) || SCMemcmpLowercase("ehlo", line->buf, 4) == 0)) { r = SMTPParseCommandHELO(state, line); @@ -1406,7 +1437,8 @@ static int SMTPProcessRequest( return SMTPProcessCommandDATA(state, tx, f, line); case SMTP_COMMAND_BDAT: - return SMTPProcessCommandBDAT(state, line); + case SMTP_COMMAND_BDAT_LAST: + return SMTPProcessCommandBDAT(state, tx, line); default: /* we have nothing to do with any other command at this instant. @@ -1424,16 +1456,37 @@ static inline void ResetLine(SMTPLine *line) } } +static int SMTPPreProcessCommandBDAT( + SMTPState *state, Flow *f, StreamSlice *slice, SMTPInput *input, SMTPLine *line) +{ + if (state->bdat_chunk_idx >= state->bdat_chunk_len) { + /* The BDAT chunk is already complete; data mode was set by another + * command, such as a pipelined DATA reply. Leave data mode and let + * the line parser handle the input as a new command. */ + state->parser_state &= ~SMTP_PARSER_STATE_COMMAND_DATA_MODE; + return 1; + } + uint32_t remaining = state->bdat_chunk_len - state->bdat_chunk_idx; + uint32_t consumed = MIN((uint32_t)input->len, remaining); + line->buf = input->buf + input->consumed; + line->len = consumed; + input->consumed += consumed; + input->len -= consumed; + int ret = SMTPProcessRequest(state, f, input, line, slice); + ResetLine(line); + return ret; +} + /* - * @brief Pre Process the data that comes in DATA mode. + * @brief Pre-process command data. * - * If currently, the command that is being processed is DATA, whatever data - * comes as a part of it must be handled by this function. This is because - * there should be no char limit imposition on the line arriving in the DATA - * mode. Such limits are in place for any lines passed to the GetLine function - * and the lines are capped there at SMTP_LINE_BUFFER_LIMIT. - * One such limit in DATA mode may lead to file data or parts of e-mail being - * truncated if the line were too long. + * If the command being processed is DATA, its data must be handled by this + * function so the line limit used by GetLine is not applied. GetLine caps lines + * at SMTP_LINE_BUFFER_LIMIT, which could truncate file data or parts of an + * e-mail if a line were too long. + * + * BDAT data is octet-counted and must be consumed only up to the declared chunk + * boundary. * * @param state Pointer to the current SMTPState * @param f Pointer to the current Flow @@ -1451,6 +1504,11 @@ static int SMTPPreProcessCommands( DEBUG_VALIDATE_BUG_ON(line->len != 0); DEBUG_VALIDATE_BUG_ON(line->delim_len != 0); + if (state->current_command == SMTP_COMMAND_BDAT || + state->current_command == SMTP_COMMAND_BDAT_LAST) { + return SMTPPreProcessCommandBDAT(state, f, slice, input, line); + } + /* fall back to strict line parsing for mime header parsing */ if (state->curr_tx && state->curr_tx->mime_state && SCMimeSmtpGetState(state->curr_tx->mime_state) < MimeSmtpBody) @@ -1542,7 +1600,8 @@ static AppLayerResult SMTPParse(uint8_t direction, Flow *f, SMTPState *state, /* toserver */ if (direction == 0) { if (((state->current_command == SMTP_COMMAND_DATA) || - (state->current_command == SMTP_COMMAND_BDAT)) && + (state->current_command == SMTP_COMMAND_BDAT) || + (state->current_command == SMTP_COMMAND_BDAT_LAST)) && (state->parser_state & SMTP_PARSER_STATE_COMMAND_DATA_MODE)) { int ret = SMTPPreProcessCommands(state, f, &stream_slice, &input, &line); DEBUG_VALIDATE_BUG_ON(ret != 0 && ret != -1 && ret != 1); @@ -1569,11 +1628,15 @@ static AppLayerResult SMTPParse(uint8_t direction, Flow *f, SMTPState *state, * wherever it had to be */ ResetLine(&line); - /* If DATA mode was entered in the middle of input parsing, exempt it from GetLine as we - * don't want input limits to be exercised on DATA data. Here, SMTPPreProcessCommands - * should either consume all the data or return in case it encounters another boundary. - * In case of another boundary, the control should be passed to SMTPGetLine */ - if ((input.len > 0) && (state->current_command == SMTP_COMMAND_DATA) && + /* If command data mode was entered in the middle of input parsing, first pass it to + * SMTPPreProcessCommands so input limits are not applied to DATA bodies and BDAT data + * is not consumed past its chunk boundary. SMTPPreProcessCommands should either + * consume all remaining input or stop at a MIME or BDAT chunk boundary, after which + * control is passed to SMTPGetLine. */ + if ((input.len > 0) && + ((state->current_command == SMTP_COMMAND_DATA) || + (state->current_command == SMTP_COMMAND_BDAT) || + (state->current_command == SMTP_COMMAND_BDAT_LAST)) && (state->parser_state & SMTP_PARSER_STATE_COMMAND_DATA_MODE)) { int ret = SMTPPreProcessCommands(state, f, &stream_slice, &input, &line); DEBUG_VALIDATE_BUG_ON(ret != 0 && ret != -1 && ret != 1); From eb6d84b3c3065265de6f05c87a62b546ea0d0b03 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 21 Jul 2026 17:29:10 -0600 Subject: [PATCH 08/11] smtp: recover from invalid BDAT command syntax A BDAT command that failed to parse, such as "BDAT 5 X", returned -1, disabling SMTP parsing for the rest of the flow. A server may reject the command and continue the session, leaving following messages uninspected. Instead raise a decoder event and queue the command as an ordinary command. Ticket: #8741 (cherry picked from commit 60e0df6530d5a3cd7fbacd6f94a411c70698d7f2) --- rules/smtp-events.rules | 4 +++- src/app-layer-smtp.c | 22 ++++++++++++++-------- src/app-layer-smtp.h | 1 + 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/rules/smtp-events.rules b/rules/smtp-events.rules index 2898bdc299f4..9c601260133a 100644 --- a/rules/smtp-events.rules +++ b/rules/smtp-events.rules @@ -31,4 +31,6 @@ alert smtp any any -> any any (msg:"SURICATA SMTP duplicate fields"; flow:establ alert smtp any any -> any any (msg:"SURICATA SMTP unparsable content"; flow:established,to_server; app-layer-event:smtp.unparsable_content; flowint:smtp.anomaly.count,+,1; classtype:protocol-command-decode; sid:2220019; rev:1;) alert smtp any any -> any any (msg:"SURICATA SMTP filename truncated"; flow:established,to_server; app-layer-event:smtp.mime_long_filename; flowint:smtp.anomaly.count,+,1; classtype:protocol-command-decode; sid:2220020; rev:1;) alert smtp any any -> any any (msg:"SURICATA SMTP failed protocol change"; flow:established,to_client; app-layer-event:smtp.failed_protocol_change; flowint:smtp.anomaly.count,+,1; classtype:protocol-command-decode; sid:2220021; rev:2;) -# next sid 2220022 +alert smtp any any -> any any (msg:"SURICATA SMTP invalid BDAT command"; flow:established,to_server; app-layer-event:smtp.invalid_bdat; flowint:smtp.anomaly.count,+,1; classtype:protocol-command-decode; sid:2220022; rev:1;) + +# next sid 2220023 diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index a3a5140da54e..7f41a142c434 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -132,6 +132,7 @@ SCEnumCharMap smtp_decoder_event_table[] = { { "MAX_REPLY_LINE_LEN_EXCEEDED", SMTP_DECODER_EVENT_MAX_REPLY_LINE_LEN_EXCEEDED }, { "INVALID_PIPELINED_SEQUENCE", SMTP_DECODER_EVENT_INVALID_PIPELINED_SEQUENCE }, { "BDAT_CHUNK_LEN_EXCEEDED", SMTP_DECODER_EVENT_BDAT_CHUNK_LEN_EXCEEDED }, + { "INVALID_BDAT", SMTP_DECODER_EVENT_INVALID_BDAT }, { "NO_SERVER_WELCOME_MESSAGE", SMTP_DECODER_EVENT_NO_SERVER_WELCOME_MESSAGE }, { "TLS_REJECTED", SMTP_DECODER_EVENT_TLS_REJECTED }, { "DATA_COMMAND_REJECTED", SMTP_DECODER_EVENT_DATA_COMMAND_REJECTED }, @@ -1385,14 +1386,19 @@ static int SMTPProcessRequest( bool last = false; r = SMTPParseCommandBDAT(state, line, &last); if (r == -1) { - SCReturnInt(-1); - } - state->current_command = last ? SMTP_COMMAND_BDAT_LAST : SMTP_COMMAND_BDAT; - SMTPSetProgressTS(tx, SMTP_REQUEST_DATA); - if (state->bdat_chunk_len > 0) { - state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; - } else if (last) { - SMTPTransactionCompleteTS(tx); + /* Invalid BDAT syntax is recoverable: the server rejects the + * command and the session continues. */ + SMTPSetEvent(state, SMTP_DECODER_EVENT_INVALID_BDAT); + state->current_command = SMTP_COMMAND_OTHER_CMD; + r = 0; + } else { + state->current_command = last ? SMTP_COMMAND_BDAT_LAST : SMTP_COMMAND_BDAT; + SMTPSetProgressTS(tx, SMTP_REQUEST_DATA); + if (state->bdat_chunk_len > 0) { + state->parser_state |= SMTP_PARSER_STATE_COMMAND_DATA_MODE; + } else if (last) { + SMTPTransactionCompleteTS(tx); + } } } else if (line->len >= 4 && ((SCMemcmpLowercase("helo", line->buf, 4) == 0) || SCMemcmpLowercase("ehlo", line->buf, 4) == 0)) { diff --git a/src/app-layer-smtp.h b/src/app-layer-smtp.h index c455eca777a3..b6cb3964d281 100644 --- a/src/app-layer-smtp.h +++ b/src/app-layer-smtp.h @@ -38,6 +38,7 @@ enum { SMTP_DECODER_EVENT_MAX_REPLY_LINE_LEN_EXCEEDED, SMTP_DECODER_EVENT_INVALID_PIPELINED_SEQUENCE, SMTP_DECODER_EVENT_BDAT_CHUNK_LEN_EXCEEDED, + SMTP_DECODER_EVENT_INVALID_BDAT, SMTP_DECODER_EVENT_NO_SERVER_WELCOME_MESSAGE, SMTP_DECODER_EVENT_TLS_REJECTED, SMTP_DECODER_EVENT_DATA_COMMAND_REJECTED, From 78299e173eb691cd234c6564b4a457495b60148f Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 11 Aug 2026 16:03:23 -0600 Subject: [PATCH 09/11] smtp: assign response frames to owning transaction SMTPGetLine assigned response frames to the current transaction even when a queued reply belonged to an older transaction. Use the queued command owner so frame EVE output and frame-based detection receive the correct transaction id. Includes some re-org to avoid prototypes for static functions. Ticket: #8741 (cherry picked from commit 57ae57152ce325b16e5c89109d9b680e954454e8) --- src/app-layer-smtp.c | 61 ++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index 7f41a142c434..799571f82255 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -560,6 +560,34 @@ static SMTPTransaction *SMTPTransactionCreate(SMTPState *state) return tx; } +static SMTPTransaction *SMTPStateGetTxById(SMTPState *state, uint64_t tx_id) +{ + SMTPTransaction *tx = NULL; + TAILQ_FOREACH (tx, &state->tx_list, next) { + if (tx->tx_id == tx_id) { + return tx; + } + if (tx->tx_id > tx_id) { + break; + } + } + return NULL; +} + +static SMTPTransaction *SMTPGetReplyTx(SMTPState *state) +{ + if (state->cmds_idx >= state->cmds_cnt) { + return state->curr_tx; + } + + /* a command with no owning tx, or whose tx is gone, must not resolve + * to another tx */ + if (state->cmds_tx_ids[state->cmds_idx] == SMTP_NO_TX_ID) { + return NULL; + } + return SMTPStateGetTxById(state, state->cmds_tx_ids[state->cmds_idx]); +} + static void FlagDetectStateNewFile(SMTPTransaction *tx) { if (tx && tx->tx_data.de_state) { @@ -625,8 +653,9 @@ static AppLayerResult SMTPGetLine(Flow *f, StreamSlice *slice, SMTPState *state, } else if (direction == 1) { frame = AppLayerFrameNewByPointer( f, slice, input->buf + input->consumed, -1, 1, SMTP_FRAME_RESPONSE_LINE); - if (frame != NULL && state->curr_tx) { - AppLayerFrameSetTxId(frame, state->curr_tx->tx_id); + SMTPTransaction *reply_tx = SMTPGetReplyTx(state); + if (frame != NULL && reply_tx != NULL) { + AppLayerFrameSetTxId(frame, reply_tx->tx_id); } } } @@ -945,34 +974,6 @@ static inline bool IsReplyToCommand(const SMTPState *state, const uint8_t cmd) return (state->cmds_idx < state->cmds_cnt && state->cmds[state->cmds_idx] == cmd); } -static SMTPTransaction *SMTPStateGetTxById(SMTPState *state, uint64_t tx_id) -{ - SMTPTransaction *tx = NULL; - TAILQ_FOREACH (tx, &state->tx_list, next) { - if (tx->tx_id == tx_id) { - return tx; - } - if (tx->tx_id > tx_id) { - break; - } - } - return NULL; -} - -static SMTPTransaction *SMTPGetReplyTx(SMTPState *state) -{ - if (state->cmds_idx >= state->cmds_cnt) { - return state->curr_tx; - } - - /* a command with no owning tx, or whose tx is gone, must not resolve - * to another tx */ - if (state->cmds_tx_ids[state->cmds_idx] == SMTP_NO_TX_ID) { - return NULL; - } - return SMTPStateGetTxById(state, state->cmds_tx_ids[state->cmds_idx]); -} - static int SMTPProcessReply( SMTPState *state, Flow *f, SMTPThreadCtx *td, SMTPInput *input, const SMTPLine *line) { From 1fd67c71f56da16d84060436b0f5d671ca5e1104 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Wed, 22 Jul 2026 15:10:00 -0600 Subject: [PATCH 10/11] smtp: handle mid-session helo/ehlo like rset RFC 5321 says a mid-session EHLO should work just like RSET. We more or less ignored it, which meant transaction state could carry over. Treat a HELO/EHLO received during a transaction as RSET once the server accepts it. Ticket: #8715 (cherry picked from commit 0be6e345fdc8152e167deb1c77171f243a21bd1a) --- src/app-layer-smtp.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index 799571f82255..b93f715371b9 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -1407,7 +1407,17 @@ static int SMTPProcessRequest( if (r == -1) { SCReturnInt(-1); } - state->current_command = SMTP_COMMAND_OTHER_CMD; + if (state->curr_tx->mail_from != NULL || !TAILQ_EMPTY(&state->curr_tx->rcpt_to_list) || + state->curr_tx->progress_ts != SMTP_REQUEST_STARTED) { + /* Mid-session HELO/EHLO resets the state as if a RSET + * had been issued (RFC 5321 4.1.4). The progress check + * catches a transaction with no envelope but an attempted + * DATA or BDAT, such as a rejected envelope-less DATA. */ + state->bdat_chunk_idx = 0; + state->current_command = SMTP_COMMAND_RSET; + } else { + state->current_command = SMTP_COMMAND_OTHER_CMD; + } } else if (line->len >= 9 && SCMemcmpLowercase("mail from", line->buf, 9) == 0) { r = SMTPParseCommandMAILFROM(state, line); if (r == -1) { From 0cb09e77342082a921a8beec50986bb7e65517cb Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Mon, 17 Aug 2026 12:11:54 -0600 Subject: [PATCH 11/11] smtp: avoid scan-build false-positive null dereference Scan-build reports a possible NULL dereference that is not reachable. (cherry picked from commit 4481f27657675cca1b24d010e3a8074b928cd313) --- src/app-layer-smtp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/app-layer-smtp.c b/src/app-layer-smtp.c index b93f715371b9..ebf2b0522501 100644 --- a/src/app-layer-smtp.c +++ b/src/app-layer-smtp.c @@ -1344,7 +1344,13 @@ static int SMTPProcessRequest( int r = 0; AppLayerParserTriggerRawStreamInspection(f, STREAM_TOSERVER); - if (line->len >= 8 && SCMemcmpLowercase("starttls", line->buf, 8) == 0) { + if (tx == NULL) { + DEBUG_VALIDATE_BUG_ON(!no_new_tx); + const bool is_rset = SCMemcmpLowercase("rset", line->buf, 4) == 0; + if (is_rset) + state->bdat_chunk_idx = 0; + state->current_command = is_rset ? SMTP_COMMAND_RSET : SMTP_COMMAND_QUIT; + } else if (line->len >= 8 && SCMemcmpLowercase("starttls", line->buf, 8) == 0) { state->current_command = SMTP_COMMAND_STARTTLS; } else if (line->len >= 4 && SCMemcmpLowercase("data", line->buf, 4) == 0) { state->current_command = SMTP_COMMAND_DATA;