mirror of
https://github.com/OISF/suricata.git
synced 2026-10-11 22:09:53 +00:00
ftp: mark truncation on the line, not the connection
The truncation flag lived in FtpState, where the line getter set it and the two callers cleared it once FtpLineState.lf_found said the line had ended. The callers reuse one FtpLineState for every line in a slice, and only the LF branch ever set lf_found, so a complete line left it set for whatever came after. An over-long line behind a complete one in the same slice therefore had its truncation state cleared, its remainder was parsed as ordinary traffic, and a "PASS s3cret" sitting in that remainder became a transaction of its own. The flag could just as easily be left set. The request loop reached its clear only for a line that matched an FTP command, continuing to the next line otherwise, so an over-long run of "Z" left the flag standing and the next command to arrive was discarded as that line's remainder. The response loop's clear sat three ifs deep, inside blocks entered only when the reply parsed, so a line that SCFTPParseResponseLine rejects -- a run of spaces is empty once trimmed -- left it standing too. The getter also raised the flag for a line whose LF it had just consumed. Nothing of that line remained to discard, so the caller had to clear it straight back. Truncation is a property of the line, so FtpLineState now carries it. The getter sets line.truncated for the line it hands back, and the FtpState flag stays set only where truncation genuinely spans slices: the over-long line that arrives without an LF. The callers read line.truncated and no longer touch the shared state, so lf_found goes away entirely. Issue: 8859
This commit is contained in:
committed by
Victor Julien
parent
7564d4d5a9
commit
60dc5d0e75
+11
-13
@@ -281,12 +281,16 @@ static AppLayerResult FTPGetLineForDirection(
|
||||
{
|
||||
SCEnter();
|
||||
|
||||
/* the caller reuses one FtpLineState for every line in a slice */
|
||||
line->truncated = false;
|
||||
|
||||
while (input->len > 0) {
|
||||
const uint8_t *lf_idx = memchr(input->buf + input->consumed, 0x0a, input->len);
|
||||
|
||||
if (lf_idx == NULL) {
|
||||
if (!(*current_line_truncated) && (uint32_t)input->len >= ftp_max_line_len) {
|
||||
*current_line_truncated = true;
|
||||
line->truncated = true;
|
||||
line->buf = input->buf + input->consumed;
|
||||
line->len = ftp_max_line_len;
|
||||
line->delim_len = 0;
|
||||
@@ -313,7 +317,6 @@ static AppLayerResult FTPGetLineForDirection(
|
||||
continue;
|
||||
}
|
||||
|
||||
line->lf_found = true;
|
||||
line->buf = input->buf + o_consumed;
|
||||
line->len = line_len;
|
||||
// There could be one chunk of command data that has LF but post the line limit
|
||||
@@ -321,7 +324,9 @@ static AppLayerResult FTPGetLineForDirection(
|
||||
// lf_idx = 5010
|
||||
// max_line_len = 4096
|
||||
if (line->len >= ftp_max_line_len) {
|
||||
*current_line_truncated = true;
|
||||
/* This LF ends the line, so nothing carries into the next slice;
|
||||
* the line is simply reported clipped to the limit. */
|
||||
line->truncated = true;
|
||||
line->len = ftp_max_line_len;
|
||||
SCReturnStruct(APP_LAYER_OK);
|
||||
}
|
||||
@@ -449,7 +454,7 @@ static AppLayerResult FTPParseRequest(Flow *f, void *ftp_state, AppLayerParserSt
|
||||
}
|
||||
|
||||
FtpInput ftpi = { .buf = input, .len = input_len, .orig_len = input_len, .consumed = 0 };
|
||||
FtpLineState line = { .buf = NULL, .len = 0, .delim_len = 0, .lf_found = false };
|
||||
FtpLineState line = { .buf = NULL, .len = 0, .delim_len = 0, .truncated = false };
|
||||
|
||||
uint8_t direction = STREAM_TOSERVER;
|
||||
AppLayerResult res;
|
||||
@@ -476,11 +481,7 @@ static AppLayerResult FTPParseRequest(Flow *f, void *ftp_state, AppLayerParserSt
|
||||
|
||||
tx->command_descriptor = cmd_descriptor;
|
||||
tx->request_length = CopyCommandLine(&tx->request, &line);
|
||||
tx->request_truncated = state->current_line_truncated_ts;
|
||||
|
||||
if (line.lf_found) {
|
||||
state->current_line_truncated_ts = false;
|
||||
}
|
||||
tx->request_truncated = line.truncated;
|
||||
if (tx->request_truncated) {
|
||||
SCAppLayerDecoderEventsSetEventRaw(&tx->tx_data.events, FtpEventRequestCommandTooLong);
|
||||
}
|
||||
@@ -687,7 +688,7 @@ static AppLayerResult FTPParseResponse(Flow *f, void *ftp_state, AppLayerParserS
|
||||
SCReturnStruct(APP_LAYER_OK);
|
||||
}
|
||||
FtpInput ftpi = { .buf = input, .len = input_len, .orig_len = input_len, .consumed = 0 };
|
||||
FtpLineState line = { .buf = NULL, .len = 0, .delim_len = 0, .lf_found = false };
|
||||
FtpLineState line = { .buf = NULL, .len = 0, .delim_len = 0, .truncated = false };
|
||||
|
||||
FTPTransaction *lasttx = TAILQ_FIRST(&state->tx_list);
|
||||
AppLayerResult res;
|
||||
@@ -769,14 +770,11 @@ static AppLayerResult FTPParseResponse(Flow *f, void *ftp_state, AppLayerParserS
|
||||
if (likely(response)) {
|
||||
FTPResponseWrapper *wrapper = FTPResponseWrapperAlloc(response);
|
||||
if (likely(wrapper)) {
|
||||
response->truncated = state->current_line_truncated_tc;
|
||||
response->truncated = line.truncated;
|
||||
if (response->truncated) {
|
||||
SCAppLayerDecoderEventsSetEventRaw(
|
||||
&tx->tx_data.events, FtpEventResponseCommandTooLong);
|
||||
}
|
||||
if (line.lf_found) {
|
||||
state->current_line_truncated_tc = false;
|
||||
}
|
||||
TAILQ_INSERT_TAIL(&tx->response_list, wrapper, next);
|
||||
} else {
|
||||
SCFTPFreeResponseLine(response);
|
||||
|
||||
+3
-1
@@ -39,7 +39,9 @@ typedef struct FtpLineState_ {
|
||||
const uint8_t *buf;
|
||||
uint32_t len;
|
||||
uint8_t delim_len;
|
||||
bool lf_found;
|
||||
/** the line was longer than the configured maximum and buf holds only
|
||||
* what fit; set by the line getter for the line it hands back */
|
||||
bool truncated;
|
||||
} FtpLineState;
|
||||
|
||||
typedef struct FTPResponseWrapper_ {
|
||||
|
||||
Reference in New Issue
Block a user