From 354074bac68b4d6356f2fcd9fd60a322268a51ce Mon Sep 17 00:00:00 2001 From: Jeff Lucovsky Date: Tue, 8 Oct 2019 20:14:23 -0400 Subject: [PATCH] ftp: Handle malformed RETR/STOR Ensure that RETR (STOR) have a filename -- otherwise, treat the command string as malformed. Added unittests for each command and verified that SEGV's occur without parser change and no longer occur with the parser change. --- src/app-layer-ftp.c | 168 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 2 deletions(-) diff --git a/src/app-layer-ftp.c b/src/app-layer-ftp.c index a172350ef6..9d168cd751 100644 --- a/src/app-layer-ftp.c +++ b/src/app-layer-ftp.c @@ -624,8 +624,10 @@ static int FTPParseRequest(Flow *f, void *ftp_state, // fallthrough case FTP_COMMAND_STOR: { - /* No dyn port negotiated so get out */ - if (state->dyn_port == 0) { + /* Ensure that there is a negotiated dyn port and a file + * name -- need more than 5 chars: cmd [4], space, + */ + if (state->dyn_port == 0 || state->current_line_len < 6) { SCReturnInt(-1); } struct FtpTransferCmd *data = FTPCalloc(1, sizeof(struct FtpTransferCmd)); @@ -1748,6 +1750,166 @@ static int FTPParserTest10(void) goto end; } +end: + if (alp_tctx != NULL) + AppLayerParserThreadCtxFree(alp_tctx); + StreamTcpFreeConfig(TRUE); + FLOW_DESTROY(&f); + return result; +} + +/** \test Supply RETR without a filename */ +static int FTPParserTest11(void) +{ + int result = 1; + Flow f; + uint8_t ftpbuf1[] = "PORT 192,168,1,1,0,80\r\n"; + uint8_t ftpbuf2[] = "RETR\r\n"; + uint8_t ftpbuf3[] = "227 OK\r\n"; + TcpSession ssn; + + AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc(); + + memset(&f, 0, sizeof(f)); + memset(&ssn, 0, sizeof(ssn)); + + FLOW_INITIALIZE(&f); + f.protoctx = (void *)&ssn; + f.proto = IPPROTO_TCP; + f.alproto = ALPROTO_FTP; + + StreamTcpInitConfig(TRUE); + + FLOWLOCK_WRLOCK(&f); + int r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, + STREAM_TOSERVER | STREAM_START, ftpbuf1, + sizeof(ftpbuf1) - 1); + if (r != 0) { + result = 0; + FLOWLOCK_UNLOCK(&f); + goto end; + } + FLOWLOCK_UNLOCK(&f); + + /* Response */ + FLOWLOCK_WRLOCK(&f); + r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, + STREAM_TOCLIENT, + ftpbuf3, + sizeof(ftpbuf3) - 1); + if (r != 0) { + result = 0; + FLOWLOCK_UNLOCK(&f); + goto end; + } + FLOWLOCK_UNLOCK(&f); + + FLOWLOCK_WRLOCK(&f); + r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, + STREAM_TOSERVER, ftpbuf2, + sizeof(ftpbuf2) - 1); + if (r == 0) { + SCLogDebug("parse should've failed"); + result = 0; + FLOWLOCK_UNLOCK(&f); + goto end; + } + FLOWLOCK_UNLOCK(&f); + + FtpState *ftp_state = f.alstate; + if (ftp_state == NULL) { + SCLogDebug("no ftp state: "); + result = 0; + goto end; + } + + if (ftp_state->command != FTP_COMMAND_RETR) { + SCLogDebug("expected command %" PRIu32 ", got %" PRIu32 ": ", + FTP_COMMAND_RETR, ftp_state->command); + result = 0; + goto end; + } + +end: + if (alp_tctx != NULL) + AppLayerParserThreadCtxFree(alp_tctx); + StreamTcpFreeConfig(TRUE); + FLOW_DESTROY(&f); + return result; +} + +/** \test Supply STOR without a filename */ +static int FTPParserTest12(void) +{ + int result = 1; + Flow f; + uint8_t ftpbuf1[] = "PORT 192,168,1,1,0,80\r\n"; + uint8_t ftpbuf2[] = "STOR\r\n"; + uint8_t ftpbuf3[] = "227 OK\r\n"; + TcpSession ssn; + + AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc(); + + memset(&f, 0, sizeof(f)); + memset(&ssn, 0, sizeof(ssn)); + + FLOW_INITIALIZE(&f); + f.protoctx = (void *)&ssn; + f.proto = IPPROTO_TCP; + f.alproto = ALPROTO_FTP; + + StreamTcpInitConfig(TRUE); + + FLOWLOCK_WRLOCK(&f); + int r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, + STREAM_TOSERVER | STREAM_START, ftpbuf1, + sizeof(ftpbuf1) - 1); + if (r != 0) { + result = 0; + FLOWLOCK_UNLOCK(&f); + goto end; + } + FLOWLOCK_UNLOCK(&f); + + /* Response */ + FLOWLOCK_WRLOCK(&f); + r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, + STREAM_TOCLIENT, + ftpbuf3, + sizeof(ftpbuf3) - 1); + if (r != 0) { + result = 0; + FLOWLOCK_UNLOCK(&f); + goto end; + } + FLOWLOCK_UNLOCK(&f); + + FLOWLOCK_WRLOCK(&f); + r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP, + STREAM_TOSERVER, ftpbuf2, + sizeof(ftpbuf2) - 1); + if (r == 0) { + SCLogDebug("parse should've failed"); + result = 0; + FLOWLOCK_UNLOCK(&f); + goto end; + } + FLOWLOCK_UNLOCK(&f); + + FtpState *ftp_state = f.alstate; + if (ftp_state == NULL) { + SCLogDebug("no ftp state: "); + result = 0; + goto end; + } + + if (ftp_state->command != FTP_COMMAND_STOR) { + SCLogDebug("expected command %" PRIu32 ", got %" PRIu32 ": ", + FTP_COMMAND_STOR, ftp_state->command); + result = 0; + goto end; + } + end: if (alp_tctx != NULL) AppLayerParserThreadCtxFree(alp_tctx); @@ -1765,6 +1927,8 @@ void FTPParserRegisterTests(void) UtRegisterTest("FTPParserTest06", FTPParserTest06); UtRegisterTest("FTPParserTest07", FTPParserTest07); UtRegisterTest("FTPParserTest10", FTPParserTest10); + UtRegisterTest("FTPParserTest11", FTPParserTest11); + UtRegisterTest("FTPParserTest12", FTPParserTest12); #endif /* UNITTESTS */ }