Pull request #5052: appid: prevent oob read in bootp option parsing

Merge in SNORT/snort3 from ~BHRYNIV/snort3:fix_bootp_oob to master

Squashed commit of the following:

commit fdd6efba573953f666c8cb7de141c5df4d8e7086
Author: Bohdan Hryniv <bhryniv@cisco>
Date:   Fri Dec 12 05:45:41 2025 -0500

    appid: prevent oob read in bootp option parsing

diff --git a/src/network_inspectors/appid/service_plugins/service_bootp.cc b/src/network_inspectors/appid/service_plugins/service_bootp.cc
index 4847fa850804cadc33afd60abe1e68a5b95bcdde..40c79465cf297d82b03c95ef042f58255c00a588 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/service_bootp.cc
+++ b/src/network_inspectors/appid/service_plugins/service_bootp.cc
@@ -154,11 +154,9 @@ int BootpServiceDetector::validate(AppIdDiscoveryArgs& args)
                 int option53 = 0;
                 for (i=sizeof(ServiceBOOTPHeader)+sizeof(uint32_t); i<size; )
                 {
-                    op = (const ServiceDHCPOption*)&data[i];
-                    if (op->option == 0xff)
+                    if (data[i] == 0xff)
                     {
                         const eth::EtherHdr* eh = layer::get_eth_layer(pkt);
-
                         if (!eh)
                             goto fail;
 
@@ -168,10 +166,13 @@ int BootpServiceDetector::validate(AppIdDiscoveryArgs& args)
                         }
                         goto inprocess;
                     }
+                    if (i + sizeof(ServiceDHCPOption) > size)
+                        goto not_compatible;
+                    op = (const ServiceDHCPOption*)(&data[i]);
                     i += sizeof(ServiceDHCPOption);
-                    if (i >= size)
+                    if (i + op->len > size)
                         goto not_compatible;
-                    if (op->option == 53 && op->len == 1 && i + 1 < size && data[i] == 3)
+                    if (op->option == 53 && op->len == 1 && data[i] == 3)
                     {
                         option53 = 1;
                     }
@@ -223,11 +224,9 @@ int BootpServiceDetector::validate(AppIdDiscoveryArgs& args)
                 i<size;
                 )
             {
-                op = (const ServiceDHCPOption*)&data[i];
-                if (op->option == 0xff)
+                if (data[i] == 0xff)
                 {
                     const eth::EtherHdr* eh = layer::get_eth_layer(pkt);
-
                     if (!eh)
                         goto fail;
 
@@ -237,6 +236,9 @@ int BootpServiceDetector::validate(AppIdDiscoveryArgs& args)
                             router);
                     goto success;
                 }
+                if (i + sizeof(ServiceDHCPOption) > size)
+                    goto fail;
+                op = (const ServiceDHCPOption*)(&data[i]);
                 i += sizeof(ServiceDHCPOption);
                 if (i + op->len > size)
                     goto fail;

diff --git a/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt b/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt
index d626715ce5282c44f37817ad47bf8a2a0951db80..75e68c564cd4675e5111ca3a4c30a2f9f1e107dc 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt
+++ b/src/network_inspectors/appid/service_plugins/test/CMakeLists.txt
@@ -9,3 +9,4 @@ add_cpputest( service_netbios_test )
 add_cpputest( service_nntp_test )
 add_cpputest( service_ftp_test )
 add_cpputest( service_tftp_test )
+add_cpputest( service_bootp_test )

diff --git a/src/network_inspectors/appid/service_plugins/test/service_bootp_test.cc b/src/network_inspectors/appid/service_plugins/test/service_bootp_test.cc
new file mode 100644 (file)
index 0000000..be26aa6
--- /dev/null
+++ b/src/network_inspectors/appid/service_plugins/test/service_bootp_test.cc
@@ -0,0 +1,137 @@
+//--------------------------------------------------------------------------
+// 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_bootp_test.cc author Bohdan Hryniv <bhryniv@cisco.com>
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "../service_bootp.h"
+#include "../service_bootp.cc"
+#include "service_plugin_mock.h"
+
+#include <CppUTest/CommandLineTestRunner.h>
+#include <CppUTest/TestHarness.h>
+#include <CppUTestExt/MockSupport.h>
+
+namespace snort
+{
+namespace layer
+{
+static eth::EtherHdr s_mock_eth_hdr = {};
+static const eth::EtherHdr* s_mock_eth_hdr_ptr = &s_mock_eth_hdr;
+const eth::EtherHdr* get_eth_layer(const Packet*) { return s_mock_eth_hdr_ptr; }
+}
+}
+
+AppIdModule::AppIdModule() : Module("a", "b") { }
+AppIdModule::~AppIdModule() = default;
+
+static AppIdConfig test_config;
+static ServiceDiscovery mock_sd;
+static BootpServiceDetector detector(&mock_sd);
+static snort::Packet mock_pkt(false);
+static snort::SfIp mock_ip;
+static AppIdModule mock_module;
+
+AppIdInspector::AppIdInspector(AppIdModule&) : config(&test_config), ctxt(test_config) { }
+
+static AppIdInspector test_inspector(mock_module);
+static OdpContext test_odp(test_config, nullptr);
+static AppIdSession test_asd(IpProtocol::UDP, &mock_ip, 67, test_inspector, test_odp, 0, 0);
+
+static void build_bootp_header(uint8_t* buf, uint8_t op)
+{
+    memset(buf, 0, sizeof(ServiceBOOTPHeader));
+    ServiceBOOTPHeader* bh = reinterpret_cast<ServiceBOOTPHeader*>(buf);
+    bh->op = op;
+    bh->htype = 0x01;
+    bh->hlen = 0x06;
+}
+
+static void add_magic_cookie(uint8_t* buf)
+{
+    buf[0] = 0x63;
+    buf[1] = 0x82;
+    buf[2] = 0x53;
+    buf[3] = 0x63;
+}
+
+TEST_GROUP(bootp_parsing_tests)
+{
+    void setup() override
+    {
+    }
+    void teardown() override
+    {
+    }
+};
+
+TEST(bootp_parsing_tests, dhcp_request_truncated_option_header)
+{
+    uint8_t pkt[sizeof(ServiceBOOTPHeader) + 4 + 1];
+    build_bootp_header(pkt, 0x01);
+    add_magic_cookie(pkt + sizeof(ServiceBOOTPHeader));
+    pkt[sizeof(ServiceBOOTPHeader) + 4] = 0x35;
+
+    AppidChangeBits change_bits;
+    AppIdDiscoveryArgs args(pkt, sizeof(pkt), APP_ID_FROM_INITIATOR,
+        test_asd, &mock_pkt, change_bits);
+
+    int ret = detector.validate(args);
+    CHECK_EQUAL(APPID_NOT_COMPATIBLE, ret);
+}
+
+TEST(bootp_parsing_tests, dhcp_reply_truncated_option_header)
+{
+    uint8_t pkt[sizeof(ServiceBOOTPHeader) + 4 + 1];
+    build_bootp_header(pkt, 0x02);
+    add_magic_cookie(pkt + sizeof(ServiceBOOTPHeader));
+    pkt[sizeof(ServiceBOOTPHeader) + 4] = 0x35;
+
+    AppidChangeBits change_bits;
+    AppIdDiscoveryArgs args(pkt, sizeof(pkt), APP_ID_FROM_RESPONDER,
+        test_asd, &mock_pkt, change_bits);
+
+    int ret = detector.validate(args);
+    CHECK_EQUAL(APPID_NOMATCH, ret);
+}
+
+TEST(bootp_parsing_tests, dhcp_reply_multi_option_then_truncated)
+{
+    uint8_t pkt[sizeof(ServiceBOOTPHeader) + 4 + 4];
+    build_bootp_header(pkt, 0x02);
+    add_magic_cookie(pkt + sizeof(ServiceBOOTPHeader));
+    pkt[sizeof(ServiceBOOTPHeader) + 4] = 53;
+    pkt[sizeof(ServiceBOOTPHeader) + 5] = 1;
+    pkt[sizeof(ServiceBOOTPHeader) + 6] = 5;
+    pkt[sizeof(ServiceBOOTPHeader) + 7] = 0x01;
+
+    AppidChangeBits change_bits;
+    AppIdDiscoveryArgs args(pkt, sizeof(pkt), APP_ID_FROM_RESPONDER,
+        test_asd, &mock_pkt, change_bits);
+
+    int ret = detector.validate(args);
+    CHECK_EQUAL(APPID_NOMATCH, ret);
+}
+
+int main(int argc, char** argv)
+{
+    return CommandLineTestRunner::RunAllTests(argc, argv);
+}

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 520aee3164ba627ae3097a54f0cf4735ed7f3e59..05a838102dae09332b27edc3c23396f0b056184e 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
@@ -260,7 +260,8 @@ 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*) { }
 
-snort::Packet* snort::DetectionEngine::get_current_packet() { return nullptr; }
+static snort::Packet s_mock_packet(false);
+snort::Packet* snort::DetectionEngine::get_current_packet() { return &s_mock_packet; }
 
 unsigned AppIdInspector::get_pub_id() { return 0; }
 #endif