From: Thomas Markwalder Date: Thu, 9 Feb 2023 15:54:05 +0000 (-0500) Subject: [#2538] Avoid adding options to themselves X-Git-Tag: Kea-2.3.5~48 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3500acb0bf72bba574cf6504d2aab17e0187c951;p=thirdparty%2Fkea.git [#2538] Avoid adding options to themselves src/lib/dhcp/option.cc Option::addOption(OptionPtr opt) - added sanity check to avoid options being added to themselves as Option::addOption() is called all over the place. src/lib/dhcpsrv/cfg_option.cc CfgOption::encapsulateInternal(const OptionPtr& option) - added sanity check to avoid adding options to themselves. Not strictly necessary but more better then relying on lower level defenses. src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc TEST_F(ParseConfigTest, selfEncapsulationTest) - new test src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc TEST_F(HostReservationParserTest, selfEncapsulation) - new test --- diff --git a/ChangeLog b/ChangeLog index 8b630cc53a..f11deb8217 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2096. [bug] [tmark] + Corrected a bug which allowed options to be added to themselves + as suboptions. + (Gitlab #2538) + 2095. [bug] marcin, tmark Added a compile-time check if the PostgreSQL version supports the "tcp-user-timeout" parameter. This parameter is available in diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc index 2ece006fc4..1e923da3cd 100644 --- a/src/lib/dhcp/option.cc +++ b/src/lib/dhcp/option.cc @@ -329,6 +329,12 @@ Option::getHeaderLen() const { } void Option::addOption(OptionPtr opt) { + // Do not allow options to be added to themselves as this + // can lead to infinite recursion. + if (this == opt.get()) { + return; + } + options_.insert(make_pair(opt->getType(), opt)); } diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index de1378586e..ba88b0081a 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -270,6 +270,11 @@ CfgOption::encapsulateInternal(const OptionPtr& option) { // Retrieve all options from the encapsulated option space. OptionContainerPtr encap_options = getAll(encap_space); for (auto const& encap_opt : *encap_options) { + if (option.get() == encap_opt.option_.get()) { + // Avoid recursion by not adding options to themselves. + continue; + } + // Add sub-option if there isn't one added already. if (!option->getOption(encap_opt.option_->getType())) { option->addOption(encap_opt.option_); diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 48665fec70..7fca6b799c 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -3377,4 +3377,43 @@ TEST_F(ParseConfigTest, invalidSubnetPdAllocator6) { // (see CtrlDhcpv4SrvTest.commandSocketBasic in // src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc). + +// Verifies that an option which encapsulates its own option space +// doesn't cause infinite recursion. +TEST_F(ParseConfigTest, selfEncapsulationTest) { + // Verify that the option definition can be retrieved. + OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP6_OPTION_SPACE, 45); + ASSERT_TRUE(def); + + // Configuration string. + std::string config = + "{" + " \"option-data\": [" + "{" + " \"name\": \"client-data\"," + " \"code\": 45," + " \"csv-format\": false," + " \"space\": \"dhcp6\"," + " \"data\": \"0001000B0102020202030303030303\"" + "}" + "]}"; + + // Verify that the configuration string parses. + family_ = AF_INET6; + int rcode = parseConfiguration(config, true, true); + ASSERT_EQ(0, rcode); + + // Verify that the option can be retrieved. + OptionCustomPtr opt = boost::dynamic_pointer_cast + (getOptionPtr(DHCP6_OPTION_SPACE, D6O_CLIENT_DATA)); + ASSERT_TRUE(opt); + + // Verify length is correct and doesn't infinitely recurse. + EXPECT_EQ(19, opt->len()); + + // Check if it can be unparsed. + CfgOptionsTest cfg(CfgMgr::instance().getStagingCfg()); + cfg.runCfgOptionsTest(family_, config); +} + } // Anonymous namespace diff --git a/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc b/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc index a7c1de49e0..637665aa1c 100644 --- a/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -1482,4 +1483,50 @@ TEST_F(HostReservationIdsParserTest, dhcp6EmptyList) { testInvalidConfig(config); } +// Verify that options which encapsulate their own standard +// namespace don't cause recursion. +TEST_F(HostReservationParserTest, selfEncapsulation) { + // Create configuration with a client-data option for a host. + // This option encapsulates dhcp6 option space, and while it + // is not an option users would ever configure, let's make + // sure it won't do bad things if they do. + std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\"," + "\"option-data\": [" + "{" + " \"name\": \"client-data\"," + " \"csv-format\": false," + " \"data\": \"0001000B0102020202030303030303\"" + "}" + ",{ \"code\": 2, \"data\": \"778899\" }" + "]" + "}"; + + ElementPtr config_element = Element::fromJSON(config); + + HostPtr host; + HostReservationParser6 parser; + ASSERT_NO_THROW(host = parser.parse(SubnetID(10), config_element)); + ASSERT_TRUE(host); + + // One host should have been added to the configuration. + CfgHostsPtr cfg_hosts = CfgMgr::instance().getStagingCfg()->getCfgHosts(); + ASSERT_NO_THROW(cfg_hosts->add(host)); + + HostCollection hosts; + ASSERT_NO_THROW(hosts = cfg_hosts->getAll(Host::IDENT_DUID, + &duid_->getDuid()[0], + duid_->getDuid().size())); + ASSERT_EQ(1, hosts.size()); + + // Retrieve and sanity the client data option. + OptionCustomPtr opt = boost::dynamic_pointer_cast< + OptionCustom>(retrieveOption(*hosts[0], DHCP6_OPTION_SPACE, D6O_CLIENT_DATA)); + ASSERT_TRUE(opt); + + // The length should include the explicit data declared plus the length + // and data for option 2 + EXPECT_EQ(26, opt->len()); +} + + } // end of anonymous namespace