ftp: do not error the flow on file before port

Ticket: 8659

If a client sends a SOTR/RETR command before doing a PASV/PORT
command, ther server may reply
425 Use PORT or PASV first.\r\n
and allow the client to continue sendinf other commands

So, Suricata should not put the whole flow into error, it just
sets an event, and continues to parse further

rules/ftp-events.rules

@@ -5,3 +5,5 @@
 alert ftp any any -> any any (msg:"SURICATA FTP Request command too long"; flow:to_server; app-layer-event:ftp.request_command_too_long; classtype:protocol-command-decode; sid:2232000; rev:1;)
 alert ftp any any -> any any (msg:"SURICATA FTP Response command too long"; flow:to_client; app-layer-event:ftp.response_command_too_long; classtype:protocol-command-decode; sid:2232001; rev:1;)
 alert ftp any any -> any any (msg:"SURICATA FTP too many transactions"; app-layer-event:ftp.too_many_transactions; classtype:protocol-command-decode; sid:2232002; rev:1;)
+alert ftp any any -> any any (msg:"SURICATA FTP file before port"; app-layer-event:ftp.file_before_port; classtype:protocol-command-decode; sid:2232003; rev:1;)
+alert ftp any any -> any any (msg:"SURICATA FTP file without name"; app-layer-event:ftp.file_without_name; classtype:protocol-command-decode; sid:2232004; rev:1;)

rust/src/ftp/event.rs

@@ -27,6 +27,10 @@ pub enum FtpEvent {
     FtpEventResponseCommandTooLong,
     #[name("too_many_transactions")]
     FtpEventTooManyTransactions,
+    #[name("file_before_port")]
+    FtpEventFileBeforePort,
+    #[name("file_without_name")]
+    FtpEventFileWithoutName,
 }
 
 /// Wrapper around the Rust generic function for get_event_info.

src/app-layer-ftp.c

@@ -513,8 +513,14 @@ static AppLayerResult FTPParseRequest(Flow *f, void *ftp_state, AppLayerParserSt
                 /* Ensure that there is a negotiated dyn port and a file
                  * name -- need more than 5 chars: cmd [4], space, <filename>
                  */
-                if (state->dyn_port == 0 || line.len < 6) {
-                    SCReturnStruct(APP_LAYER_ERROR);
+                if (state->dyn_port == 0) {
+                    SCAppLayerDecoderEventsSetEventRaw(&tx->tx_data.events, FtpEventFileBeforePort);
+                    break;
+                }
+                if (line.len < 6) {
+                    SCAppLayerDecoderEventsSetEventRaw(
+                            &tx->tx_data.events, FtpEventFileWithoutName);
+                    break;
                 }
                 FtpTransferCmd *data = SCFTPTransferCmdNew();
                 if (data == NULL)
@@ -1508,7 +1514,7 @@ static int FTPParserTest11(void)
     r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP,
                                 STREAM_TOSERVER, ftpbuf2,
                                 sizeof(ftpbuf2) - 1);
-    FAIL_IF(r == 0);
+    FAIL_IF(r != 0);
 
     FtpState *ftp_state = f.alstate;
     FAIL_IF_NULL(ftp_state);
@@ -1556,7 +1562,7 @@ static int FTPParserTest12(void)
     r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_FTP,
                                 STREAM_TOSERVER, ftpbuf2,
                                 sizeof(ftpbuf2) - 1);
-    FAIL_IF(r == 0);
+    FAIL_IF(r != 0);
 
     FtpState *ftp_state = f.alstate;
     FAIL_IF_NULL(ftp_state);