From: Bohdan Hryniv -X (bhryniv - SOFTSERVE INC at Cisco) Date: Tue, 13 Jan 2026 21:46:33 +0000 (+0000) Subject: Pull request #5052: appid: prevent oob read in bootp option parsing X-Git-Tag: 3.10.2.0~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=73488807aeee7ae738c7d125822366e6c13fcf78;p=thirdparty%2Fsnort3.git 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 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 4847fa850..40c79465c 100644 --- 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); ioption == 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) ioption == 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 d626715ce..75e68c564 100644 --- 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 index 000000000..be26aa6f6 --- /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 + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "../service_bootp.h" +#include "../service_bootp.cc" +#include "service_plugin_mock.h" + +#include +#include +#include + +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(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 520aee316..05a838102 100644 --- 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