Pull request #4977: appid: ftp parsing bounds check

Merge in SNORT/snort3 from ~OSTEPANO/snort3:ftp_overflow to master

Squashed commit of the following:

commit 725fa41fe90f105ce77b67ba120b8bd3778018c3
Author: Oleksandr Stepanov <ostepano@cisco.com>
Date:   Wed Nov 5 06:48:08 2025 -0500

    appid: ftp parsing bounds check

diff --git a/src/network_inspectors/appid/service_plugins/service_ftp.cc b/src/network_inspectors/appid/service_plugins/service_ftp.cc
index 0dd9b6e4d690bcf5f8e9990d2d4b2f6d64d90684..bd34492d4f55e60abe28f9ef8995e19e7fc4c622 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/service_ftp.cc
+++ b/src/network_inspectors/appid/service_plugins/service_ftp.cc
@@ -317,7 +317,7 @@ static int CheckVendorVersion(const uint8_t* data, uint16_t init_offset,
 static FtpEolReturn ftp_parse_response(const uint8_t* data, uint16_t& offset,
     uint16_t size, ServiceFTPData& fd, FTPReplyState rstate)
 {
-    for (; offset < size; ++offset)
+    while (offset < size)
     {
         if (data[offset] == 0x0D)
         {
@@ -338,6 +338,13 @@ static FtpEolReturn ftp_parse_response(const uint8_t* data, uint16_t& offset,
             fd.rstate = rstate;
             return FTP_FOUND_EOL;
         }
+
+        if (offset == UINT16_MAX)
+        {
+            return FTP_NOT_FOUND_EOL;
+        }
+
+        ++offset;
     }
     return FTP_NOT_FOUND_EOL;
 }
@@ -389,13 +396,18 @@ static int ftp_validate_reply(const uint8_t* data, uint16_t& offset, uint16_t si
     FtpEolReturn parse_ret;
     FTPReplyState tmp_state;
 
-    for (; offset < size; ++offset)
+    while (offset < size)
     {
         /* Trim any blank lines (be a little tolerant) */
-        for (; offset < size; ++offset)
+        while (offset < size)
         {
             if (data[offset] != 0x0D and data[offset] != 0x0A)
                 break;
+            if (offset == UINT16_MAX)
+            {
+                return 0;
+            }
+            ++offset;
         }
 
         switch (fd.rstate)
@@ -535,6 +547,8 @@ static int ftp_validate_reply(const uint8_t* data, uint16_t& offset, uint16_t si
                 parse_ret = ftp_parse_response(data, offset, size, fd, FTP_REPLY_LONG);
                 if (parse_ret == FTP_INCORRECT_EOL)
                     return -1;
+                if (offset == UINT16_MAX)
+                    return 0;
                 if (++offset >= size)
                 {
                     offset = size;
@@ -605,7 +619,7 @@ static int ftp_validate_reply(const uint8_t* data, uint16_t& offset, uint16_t si
         }
         if (fd.rstate == FTP_REPLY_BEGIN)
         {
-            for (; offset < size; ++offset)
+            while (offset < size)
             {
                 if (data[offset] == 0x0D)
                 {
@@ -616,9 +630,20 @@ static int ftp_validate_reply(const uint8_t* data, uint16_t& offset, uint16_t si
                 }
                 else if (!isspace(data[offset]))
                     break;
+
+                if (offset == UINT16_MAX)
+                {
+                    return 0;
+                }
+                ++offset;
             }
             return fd.code;
         }
+        if (offset == UINT16_MAX)
+        {
+            return 0;
+        }
+        ++offset;
     }
     return 0;
 }

diff --git a/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt b/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt
index 6c78f2a3b917f37f1bdcd07159641b4e2c0c87e2..8fe78eee0955e964f0ffbfd3f43604c03edc96ce 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt
+++ b/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt
@@ -7,3 +7,4 @@ add_cpputest( service_snmp_test )
 add_cpputest( service_rtmp_test )
 add_cpputest( service_netbios_test )
 add_cpputest( service_nntp_test )
+add_cpputest( service_ftp_test )

diff --git a/src/network_inspectors/appid/service_plugins/test/service_ftp_test.cc b/src/network_inspectors/appid/service_plugins/test/service_ftp_test.cc
new file mode 100644 (file)
index 0000000..8d77774
--- /dev/null
+++ b/src/network_inspectors/appid/service_plugins/test/service_ftp_test.cc
@@ -0,0 +1,113 @@
+//--------------------------------------------------------------------------
+// Copyright (C) 2022-2025 Cisco and/or its affiliates. All rights reserved.
+//
+// This program is free software; you can redistribute it and/or modify it
+// under the terms of the GNU General Public License Version 2 as published
+// by the Free Software Foundation.  You may not use, modify or distribute
+// this program under any other version of the GNU General Public License.
+//
+// This program is distributed in the hope that it will be useful, but
+// WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program; if not, write to the Free Software Foundation, Inc.,
+// 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+//--------------------------------------------------------------------------
+//
+// service_ftp_test.cc author Oleksandr Stepanov <ostepano@cisco.com>
+
+#define FTP_UNIT_TEST
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "../service_ftp.h"
+#include "../service_ftp.cc"
+#include "service_plugin_mock.h"
+
+#include <CppUTest/CommandLineTestRunner.h>
+#include <CppUTest/TestHarness.h>
+#include <CppUTestExt/MockSupport.h>
+
+const uint8_t* service_strstr(const uint8_t* p, unsigned,
+    const uint8_t*, unsigned)
+{
+    return nullptr;
+}
+
+AppIdModule::AppIdModule()
+    : Module("a", "b") { }
+AppIdModule::~AppIdModule() = default;
+
+AppIdConfig test_app_config;
+
+static ServiceFTPData mock_service_data;
+static ServiceDiscovery mock_service_discovery;
+static FtpServiceDetector test_detector(&mock_service_discovery);
+
+AppIdInspector::AppIdInspector(AppIdModule&) : config(&test_app_config), ctxt(test_app_config) { }
+
+AppIdFlowData* AppIdDetector::data_get(const AppIdSession&)
+{
+    return &mock_service_data;
+}
+
+TEST_GROUP(ftp_parsing_tests)
+{
+    void setup() override
+    {
+        
+    }
+    void teardown() override
+    {
+        
+    }
+};
+
+TEST(ftp_parsing_tests, ftp_parse_invalid_offset_reply)
+{
+    uint8_t* data = new uint8_t[65535];
+    uint16_t offset = 65530;
+    ServiceFTPData fd;
+    fd.rstate = FTP_REPLY_MULTI;
+
+    memset(data, 0, 65535);
+    data[65534] = 0x0D;
+    data[65533] = 0x0D;
+
+    auto res = ftp_validate_reply(data, offset, 65535, fd);
+    CHECK_EQUAL(0, res);
+    delete[] data;
+}
+
+TEST(ftp_parsing_tests, ftp_parse_invalid_offset_response)
+{
+    uint8_t* data = new uint8_t[65535];
+    uint16_t offset = 65533;
+    ServiceFTPData fd;
+    fd.rstate = FTP_REPLY_MULTI;
+
+    memset(data, 0, 65535);
+    data[65534] = 0x0D;
+    data[65533] = 0x0D;
+    data[65532] = 0x0D;
+
+    auto res = ftp_parse_response(data, offset, 65535, fd, FTP_REPLY_MULTI);
+    CHECK_EQUAL(FTP_PARTIAL_EOL, res);
+
+    offset = 65534;
+    memset(data, 0, 65535);
+    res = ftp_parse_response(data, offset, 65535, fd, FTP_REPLY_MULTI);
+    CHECK_EQUAL(FTP_NOT_FOUND_EOL, res);
+
+    delete[] data;
+}
+
+int main(int argc, char** argv)
+{
+    int return_value = CommandLineTestRunner::RunAllTests(argc, argv);
+    return return_value;
+}
\ No newline at end of file

diff --git a/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h b/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h
index 5fae71be905d576c7b91014c8d0f49e5141e3a98..55ce81082d8860ba5c39456ca7c9b5f9aa90f707 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h
+++ b/src/network_inspectors/appid/service_plugins/test/service_plugin_mock.h
@@ -98,7 +98,9 @@ void ClientDiscovery::reload() {}
 
 int AppIdDetector::initialize(AppIdInspector&){return 0;}
 int AppIdDetector::data_add(AppIdSession&, AppIdFlowData*){return 0;}
+#ifndef FTP_UNIT_TEST
 AppIdFlowData* AppIdDetector::data_get(const AppIdSession&) {return nullptr;}
+#endif
 void AppIdDetector::add_user(AppIdSession&, const char*, AppId, bool, AppidChangeBits&){}
 void AppIdDetector::add_payload(AppIdSession&, AppId){}
 void AppIdDetector::add_app(const snort::Packet&, AppIdSession&, AppidSessionDirection, AppId, AppId, const char*, AppidChangeBits&){}
@@ -234,6 +236,26 @@ AppIdHttpSession* AppIdSession::get_http_session(uint32_t) const { return nullpt
 AppIdHttpSession* AppIdSession::create_http_session(int64_t stream_id) { return nullptr; }
 void AppIdHttpSession::set_field(HttpFieldIds id, const std::string* str, AppidChangeBits&) { }
 
+void AppIdModule::reset_stats() {}
+void AppIdModule::sum_stats(bool) { }
+AppIdInspector::~AppIdInspector() = default;
+SfIpRet snort::SfIp::set(void const*, int) { return SFIP_SUCCESS; }
+SfIpRet snort::SfIp::set(void const*) { return SFIP_SUCCESS; }
+SfIpRet snort::SfIp::pton(const int, const char* ) { return SFIP_SUCCESS; }
+void AppIdInspector::eval(snort::Packet*) { }
+void AppIdInspector::show(const snort::SnortConfig*) const { }
+void AppIdInspector::tinit() { }
+void AppIdInspector::tterm() { }
+void AppIdInspector::tear_down(snort::SnortConfig*) { }
+snort::ProfileStats* AppIdModule::get_profile(
+        unsigned, const char*&, const char*&) const
+{
+    return nullptr;
+}
+bool AppIdInspector::configure(snort::SnortConfig*) { return true; }
+PegCount snort::Module::get_global_count(char const*) const { return 0; }
+snort::Module::Module(char const*, char const*) {}
+
 void snort::DataBus::publish(unsigned, unsigned, snort::DataEvent&, snort::Flow*) { }
 void snort::DataBus::publish(unsigned, unsigned, const uint8_t*, unsigned, snort::Flow*) { }
 void snort::DataBus::publish(unsigned, unsigned, snort::Packet*, snort::Flow*) { }