From b06c134b5d5b422c12bda9d0b2a1ab100b8c21f4 Mon Sep 17 00:00:00 2001 From: Jeff Lucovsky Date: Fri, 14 Aug 2026 08:37:30 -0400 Subject: [PATCH] ftp: strip trailing whitespace before allocating CopyCommandLine allocated a buffer for the whole request line, stripped the trailing whitespace and returned the shorter length. That shorter length is what the caller stores in tx->request_length and what FTPTransactionFree later hands to FTPFree, so one byte for every stripped whitespace character is never subtracted from ftp_memuse. The memory itself is released -- SCFree ignores the size it is given -- but the counter keeps climbing for the life of the process, and a client that pads its commands with spaces can push it up with every command it sends. Once the counter reaches app-layer.protocols.ftp.memcap, FTPCheckMemcap refuses any allocation that would cross it, and request buffers are the first to be refused: the transaction is still created, but tx->request stays NULL, so command_data is missing from the log and rules that inspect it stop matching. Whether a new flow can still allocate its state depends on how much room is left below the cap. With no memcap configured, the symptom is an ftp.memuse counter that is permanently wrong. Strip the whitespace before allocating, so the size accounted for is the size returned to the caller: - CopyCommandLine shortens line->len first, then allocates and copies that length. - A line that is nothing but whitespace returns without allocating. It used to get a buffer that was stored on the transaction and freed with a length of zero, leaving the whole allocation on the counter. - When the allocation fails, line->len has already been shortened, so port_line, the transfer file name and the too-long-line check see the same length there that they see when it succeeds. - FTPParserTest13 covers the accounting and the stored request; FTPParserTest14 covers the line the parser uses afterwards. Issue: 8848 --- src/app-layer-ftp.c | 126 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 111 insertions(+), 15 deletions(-) diff --git a/src/app-layer-ftp.c b/src/app-layer-ftp.c index c9af1b8a4a..cd59f348e8 100644 --- a/src/app-layer-ftp.c +++ b/src/app-layer-ftp.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2007-2025 Open Information Security Foundation +/* Copyright (C) 2007-2026 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -391,23 +391,25 @@ static void FtpTransferCmdFree(void *data) static uint32_t CopyCommandLine(uint8_t **dest, FtpLineState *line) { - if (likely(line->len)) { - uint8_t *where = FTPCalloc(line->len + 1, sizeof(char)); - if (unlikely(where == NULL)) { - return 0; - } - memcpy(where, line->buf, line->len); + /* Strip trailing whitespace before allocating, so the length accounted by + * FTPCalloc matches the length the caller later hands to FTPFree. */ + while (line->len && isspace((unsigned char)line->buf[line->len - 1])) { + line->len--; + } - /* Remove trailing newlines/carriage returns */ - while (line->len && isspace((unsigned char)where[line->len - 1])) { - line->len--; - } + if (unlikely(line->len == 0)) { + return 0; + } - where[line->len] = '\0'; - *dest = where; + uint8_t *where = FTPCalloc(line->len + 1, sizeof(char)); + if (unlikely(where == NULL)) { + return 0; } - /* either 0 or actual */ - return line->len ? line->len + 1 : 0; + memcpy(where, line->buf, line->len); + where[line->len] = '\0'; + *dest = where; + + return line->len + 1; } #include "util-print.h" @@ -1657,6 +1659,98 @@ static int FTPParserTest12(void) StreamTcpFreeConfig(true); PASS; } + +/** \test A command padded with trailing whitespace must leave the memuse + * counter where it found it, and the padding must not be kept. */ +static int FTPParserTest13(void) +{ + Flow f; + uint8_t ftpbuf[] = "USER anonymous \r\n"; + const char expected[] = "USER anonymous"; + TcpSession ssn; + + AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc(); + + memset(&f, 0, sizeof(f)); + memset(&ssn, 0, sizeof(ssn)); + + f.protoctx = (void *)&ssn; + f.proto = IPPROTO_TCP; + f.alproto = ALPROTO_FTP; + + StreamTcpInitConfig(true); + + const uint64_t memuse = SC_ATOMIC_GET(ftp_memuse); + + int r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, STREAM_TOSERVER | STREAM_EOF, + ftpbuf, sizeof(ftpbuf) - 1); + FAIL_IF(r != 0); + + FtpState *ftp_state = f.alstate; + FAIL_IF_NULL(ftp_state); + FAIL_IF(ftp_state->command != FTP_COMMAND_USER); + + /* the request is what the transaction is freed with, so it has to be the + * stripped line: the padding is neither stored nor accounted */ + FTPTransaction *tx = TAILQ_FIRST(&ftp_state->tx_list); + FAIL_IF_NULL(tx); + FAIL_IF_NULL(tx->request); + FAIL_IF(tx->request_length != sizeof(expected)); + FAIL_IF(memcmp(tx->request, expected, sizeof(expected)) != 0); + + FLOW_DESTROY(&f); + FAIL_IF(SC_ATOMIC_GET(ftp_memuse) != memuse); + + AppLayerParserThreadCtxFree(alp_tctx); + StreamTcpFreeConfig(true); + PASS; +} + +/** \test The padding is stripped from the line itself, not just from the copy + * kept on the transaction: what follows the copy has to see the + * shortened line. */ +static int FTPParserTest14(void) +{ + Flow f; + uint8_t ftpbuf1[] = "PORT 192,168,1,1,0,80 \r\n"; + uint8_t ftpbuf2[] = "227 OK\r\n"; + const char expected[] = "PORT 192,168,1,1,0,80"; + TcpSession ssn; + + AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc(); + + memset(&f, 0, sizeof(f)); + memset(&ssn, 0, sizeof(ssn)); + + f.protoctx = (void *)&ssn; + f.proto = IPPROTO_TCP; + f.alproto = ALPROTO_FTP; + + StreamTcpInitConfig(true); + + int r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, STREAM_TOSERVER | STREAM_START, + ftpbuf1, sizeof(ftpbuf1) - 1); + FAIL_IF(r != 0); + + FtpState *ftp_state = f.alstate; + FAIL_IF_NULL(ftp_state); + FAIL_IF(ftp_state->command != FTP_COMMAND_PORT); + + /* the port line is taken from the line, so the padding must be gone */ + FAIL_IF(ftp_state->port_line_len != sizeof(expected) - 1); + FAIL_IF(memcmp(ftp_state->port_line, expected, sizeof(expected) - 1) != 0); + + r = AppLayerParserParse( + NULL, alp_tctx, &f, ALPROTO_FTP, STREAM_TOCLIENT, ftpbuf2, sizeof(ftpbuf2) - 1); + FAIL_IF(r != 0); + + FAIL_IF(ftp_state->dyn_port != 80); + + FLOW_DESTROY(&f); + AppLayerParserThreadCtxFree(alp_tctx); + StreamTcpFreeConfig(true); + PASS; +} #endif /* UNITTESTS */ void FTPParserRegisterTests(void) @@ -1665,5 +1759,7 @@ void FTPParserRegisterTests(void) UtRegisterTest("FTPParserTest01", FTPParserTest01); UtRegisterTest("FTPParserTest11", FTPParserTest11); UtRegisterTest("FTPParserTest12", FTPParserTest12); + UtRegisterTest("FTPParserTest13", FTPParserTest13); + UtRegisterTest("FTPParserTest14", FTPParserTest14); #endif /* UNITTESTS */ }