From ce4cbac1efba5425a1fe0fcca1bb57f43ea5d991 Mon Sep 17 00:00:00 2001
From: Arne Welzel <arne.welzel@corelight.com>
Date: Tue, 29 Aug 2023 17:52:12 +0200
Subject: [PATCH] ftp: Do not base seq on number of pending commands

Previously, seq was computed as the result of |pending_commands|+1. This
opened the possibility to override queued commands, as well as logging
the same pending ftp reply multiple times.

For example, when commands 1, 2, 3 are pending, command 1 may be dequeued,
but the incoming command then receives seq 3 and overrides the already
pending command 3. The second scenario happens when ftp_reply() selected
command 3 as pending for logging, but is then followed by many ftp_request()
events. This resulted in command 3's response being logged for every
following ftp_request() over and over again.

Avoid both scenarios by tracking the command sequence as an absolute counter.
---
 scripts/base/protocols/ftp/info.zeek                      | 3 +++
 scripts/base/protocols/ftp/main.zeek                      | 4 ++--
 scripts/base/protocols/ftp/utils-commands.zeek            | 4 ++--
 testing/btest/Baseline/coverage.record-fields/out.default | 1 +
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/scripts/base/protocols/ftp/info.zeek b/scripts/base/protocols/ftp/info.zeek
index 733c01729c..d0122819fe 100644
--- a/scripts/base/protocols/ftp/info.zeek
+++ b/scripts/base/protocols/ftp/info.zeek
@@ -64,6 +64,9 @@ export {
 		## to are tracked here.
 		pending_commands:   PendingCmds;
 
+		## Sequence number of previous command.
+		command_seq:        count &default=0;
+
 		## Indicates if the session is in active or passive mode.
 		passive:            bool &default=F;
 
diff --git a/scripts/base/protocols/ftp/main.zeek b/scripts/base/protocols/ftp/main.zeek
index 89c06765c8..85b04038be 100644
--- a/scripts/base/protocols/ftp/main.zeek
+++ b/scripts/base/protocols/ftp/main.zeek
@@ -165,7 +165,7 @@ function set_ftp_session(c: connection)
 		Conn::register_removal_hook(c, finalize_ftp);
 
 		# Add a shim command so the server can respond with some init response.
-		add_pending_cmd(c$ftp$pending_commands, "<init>", "");
+		add_pending_cmd(c$ftp$pending_commands, ++c$ftp$command_seq, "<init>", "");
 		}
 	}
 
@@ -270,7 +270,7 @@ event ftp_request(c: connection, command: string, arg: string) &priority=5
 
 	# Queue up the new command and argument
 	if ( |c$ftp$pending_commands| < max_pending_commands )
-		add_pending_cmd(c$ftp$pending_commands, command, arg);
+		add_pending_cmd(c$ftp$pending_commands, ++c$ftp$command_seq, command, arg);
 	else
 		Reporter::conn_weird("FTP_too_many_pending_commands", c,
 				     cat(|c$ftp$pending_commands|), "FTP");
diff --git a/scripts/base/protocols/ftp/utils-commands.zeek b/scripts/base/protocols/ftp/utils-commands.zeek
index d14d8da97c..6d0c193d38 100644
--- a/scripts/base/protocols/ftp/utils-commands.zeek
+++ b/scripts/base/protocols/ftp/utils-commands.zeek
@@ -78,9 +78,9 @@ export {
 	};
 }
 
-function add_pending_cmd(pc: PendingCmds, cmd: string, arg: string): CmdArg
+function add_pending_cmd(pc: PendingCmds, seq: count, cmd: string, arg: string): CmdArg
 	{
-	local ca = [$cmd = cmd, $arg = arg, $seq=|pc|+1, $ts=network_time()];
+	local ca = [$cmd = cmd, $arg = arg, $seq=seq, $ts=network_time()];
 	pc[ca$seq] = ca;
 
 	return ca;
diff --git a/testing/btest/Baseline/coverage.record-fields/out.default b/testing/btest/Baseline/coverage.record-fields/out.default
index d4f04e20bd..a28140ebf4 100644
--- a/testing/btest/Baseline/coverage.record-fields/out.default
+++ b/testing/btest/Baseline/coverage.record-fields/out.default
@@ -162,6 +162,7 @@ connection {
               * ts: time, log=F, optional=F
              }
         * command: string, log=T, optional=T
+        * command_seq: count, log=F, optional=T
         * cwd: string, log=F, optional=T
         * data_channel: record FTP::ExpectedDataChannel, log=T, optional=T
             FTP::ExpectedDataChannel {