From cbe30f9900fb5bf8ee57f51e92a6dc31c00f970d Mon Sep 17 00:00:00 2001
From: Victor Julien <vjulien@oisf.net>
Date: Sat, 30 Apr 2022 16:54:07 +0200
Subject: [PATCH] ftp-data: fix direction for active mode commands

Set correct direction for PORT mode, where the server connects
to the client.

The direction is not also strictly enforced. No data in the wrong
direction will be accepted to setup the file or to be added to the
file after setup.

This also fixes files getting closed twice.

Adds some general cleanups.

Bug: #3542.
(cherry picked from commit 07bf9214513e54e04508c055bb8ed29aa3bce60f)
---
 src/app-layer-ftp.c | 58 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/src/app-layer-ftp.c b/src/app-layer-ftp.c
index 63d5dc21fd..7bc14a8216 100644
--- a/src/app-layer-ftp.c
+++ b/src/app-layer-ftp.c
@@ -536,6 +536,7 @@ struct FtpTransferCmd {
     uint64_t flow_id;
     uint8_t *file_name;
     uint16_t file_len;
+    uint8_t direction; /**< direction in which the data will flow */
     FtpRequestCommand cmd;
 };
 
@@ -677,8 +678,8 @@ static int FTPParseRequest(Flow *f, void *ftp_state,
                     memcpy(data->file_name, state->current_line + 5, state->current_line_len - 5);
                     data->cmd = state->command;
                     data->flow_id = FlowGetId(f);
-                    int ret = AppLayerExpectationCreate(f,
-                                            state->active ? STREAM_TOSERVER : direction,
+                    data->direction = direction;
+                    int ret = AppLayerExpectationCreate(f, direction,
                                             0, state->dyn_port, ALPROTO_FTPDATA, data);
                     if (ret == -1) {
                         FtpTransferCmdFree(data);
@@ -1096,12 +1097,27 @@ static int FTPDataParse(Flow *f, FtpDataState *ftpdata_state,
     int ret = 0;
     /* we depend on detection engine for file pruning */
     flags |= FILE_USE_DETECT;
-    if (ftpdata_state->files == NULL) {
+    const bool eof = (direction & ftpdata_state->direction) != 0 &&
+        AppLayerParserStateIssetFlag(pstate, APP_LAYER_PARSER_EOF) != 0;
+
+    SCLogDebug("FTP-DATA input_len %u flags %04x dir %d/%s EOF %s", input_len, flags, direction,
+            (direction & STREAM_TOSERVER) ? "toserver" : "toclient", eof ? "true" : "false");
+    if (input_len && ftpdata_state->files == NULL) {
         struct FtpTransferCmd *data = (struct FtpTransferCmd *)FlowGetStorageById(f, AppLayerExpectationGetDataId());
         if (data == NULL) {
             SCReturnInt(-1);
         }
 
+        /* we shouldn't get data in the wrong dir. Don't set things up for this dir */
+        if ((direction & data->direction) == 0) {
+            // TODO set event for data in wrong direction
+            SCLogDebug("input %u not for our direction (%s): %s/%s", input_len,
+                    (direction & STREAM_TOSERVER) ? "toserver" : "toclient",
+                    data->cmd == FTP_COMMAND_STOR ? "STOR" : "RETR",
+                    (data->direction & STREAM_TOSERVER) ? "toserver" : "toclient");
+            SCReturnInt(-1);
+        }
+
         ftpdata_state->files = FileContainerAlloc();
         if (ftpdata_state->files == NULL) {
             FlowFreeStorageById(f, AppLayerExpectationGetDataId());
@@ -1116,10 +1132,14 @@ static int FTPDataParse(Flow *f, FtpDataState *ftpdata_state,
         ftpdata_state->command = data->cmd;
         switch (data->cmd) {
             case FTP_COMMAND_STOR:
-                ftpdata_state->direction = STREAM_TOSERVER;
+                ftpdata_state->direction = data->direction;
+                SCLogDebug("STOR data to %s",
+                        (ftpdata_state->direction & STREAM_TOSERVER) ? "toserver" : "toclient");
                 break;
             case FTP_COMMAND_RETR:
-                ftpdata_state->direction = STREAM_TOCLIENT;
+                ftpdata_state->direction = data->direction;
+                SCLogDebug("RETR data to %s",
+                        (ftpdata_state->direction & STREAM_TOSERVER) ? "toserver" : "toclient");
                 break;
             default:
                 break;
@@ -1136,6 +1156,21 @@ static int FTPDataParse(Flow *f, FtpDataState *ftpdata_state,
         }
         FlowFreeStorageById(f, AppLayerExpectationGetDataId());
     } else {
+        if (ftpdata_state->state == FTPDATA_STATE_FINISHED) {
+            SCLogDebug("state is already finished");
+            DEBUG_VALIDATE_BUG_ON(input_len); // data after state finished is a bug.
+            SCReturnInt(0);
+        }
+        if ((direction & ftpdata_state->direction) == 0) {
+            if (input_len) {
+                // TODO set event for data in wrong direction
+            }
+            SCLogDebug("input %u not for us (%s): %s/%s", input_len,
+                    (direction & STREAM_TOSERVER) ? "toserver" : "toclient",
+                    ftpdata_state->command == FTP_COMMAND_STOR ? "STOR" : "RETR",
+                    (ftpdata_state->direction & STREAM_TOSERVER) ? "toserver" : "toclient");
+            SCReturnInt(0);
+        }
         if (input_len != 0) {
             ret = FileAppendData(ftpdata_state->files, input, input_len);
             if (ret == -2) {
@@ -1147,19 +1182,14 @@ static int FTPDataParse(Flow *f, FtpDataState *ftpdata_state,
                 ret = -2;
                 goto out;
             }
-        } else {
-            ret = FileCloseFile(ftpdata_state->files, NULL, 0, flags);
-            ftpdata_state->state = FTPDATA_STATE_FINISHED;
-            if (ret < 0)
-                goto out;
         }
     }
-
-    if (input_len && AppLayerParserStateIssetFlag(pstate, APP_LAYER_PARSER_EOF)) {
-        ret = FileCloseFile(ftpdata_state->files, (uint8_t *) NULL, 0, flags);
+    DEBUG_VALIDATE_BUG_ON((direction & ftpdata_state->direction) == 0); // should be unreachble
+    if (eof) {
+        ret = FileCloseFile(ftpdata_state->files, NULL, 0, flags);
         ftpdata_state->state = FTPDATA_STATE_FINISHED;
+        SCLogDebug("closed because of eof");
     }
-
 out:
     return ret;
 }
-- 
2.47.2