[#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 8b630cc53a85fcab2cbfe4ea11aa54b96e69d2bf..f11deb82175125e0d65c721298939597c804fe56 100644 (file)
--- 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 2ece006fc4ad450e9bc31707d61e0b4e40a89edb..1e923da3cd1a3d54bd844f7640d63918c6a00093 100644 (file)
--- 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 de1378586e0e412cabd574887de4879b1ad69124..ba88b0081aef050c8d92a9142d589e97dcc85094 100644 (file)
--- 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 48665fec709b9b0624f19ffd556e2e18c5a8c6d5..7fca6b799c382950b5cc9ce9e6f59325650440c1 100644 (file)
--- 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<OptionCustom>
+                          (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 a7c1de49e0e90583b5e453ece21a80add27c5268..637665aa1c0ec20bf0cc630560e42440bbca1a9b 100644 (file)
--- 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 <cc/data.h>
 #include <dhcp/duid.h>
 #include <dhcp/hwaddr.h>
+#include <dhcp/option_custom.h>
 #include <dhcp/option_int.h>
 #include <dhcp/option4_addrlst.h>
 #include <dhcp/option6_addrlst.h>
@@ -1482,4 +1483,50 @@ TEST_F(HostReservationIdsParserTest, dhcp6EmptyList) {
     testInvalidConfig<HostReservationIdsParser6>(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