Skip to content

Commit

Permalink
smtp: avoid creating empty transaction
Browse files Browse the repository at this point in the history
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

(cherry picked from commit 61f2e4e)
  • Loading branch information
catenacyber authored and victorjulien committed Feb 6, 2024
1 parent f9de1cc commit cd035d5
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 26 deletions.
34 changes: 9 additions & 25 deletions src/app-layer-smtp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
7 changes: 6 additions & 1 deletion src/app-layer-smtp.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down

0 comments on commit cd035d5

Please sign in to comment.