From 9bafeaebedc2055d0863987ababae21755621111 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 23 Apr 2019 10:26:46 -0400 Subject: [PATCH] [564-patch-1.5.0] Patch for 564 --- ChangeLog | 6 + src/bin/dhcp4/tests/Makefile.am | 1 + src/bin/dhcp4/tests/vendor_opts_unittest.cc | 178 ++++++++++++++++++ src/lib/dhcp/libdhcp++.cc | 8 +- src/lib/dhcp/option.cc | 7 +- src/lib/dhcp/tests/libdhcp++_unittest.cc | 91 +++++++++ src/lib/dhcp/tests/option_unittest.cc | 10 +- src/lib/dhcpsrv/parsers/dhcp_parsers.cc | 26 ++- src/lib/dhcpsrv/parsers/option_data_parser.cc | 36 +++- .../dhcpsrv/tests/dhcp_parsers_unittest.cc | 132 +++++++++++++ 10 files changed, 471 insertions(+), 24 deletions(-) create mode 100644 src/bin/dhcp4/tests/vendor_opts_unittest.cc diff --git a/ChangeLog b/ChangeLog index db4af4a83f..7cb559527a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +1560. [bug] fdupont + -- Applied via patch --- + kea-dhcp4 now permits option code values of 0 and 255 for + defined in option spaces other than the "dhcp4" space. + (Gitlab #564,!300, git 7a0a0b84d91893f08c0ee6f236daa05bede65166) + Kea 1.5.0 released on Dec 14, 2018 1506. [build] marcin diff --git a/src/bin/dhcp4/tests/Makefile.am b/src/bin/dhcp4/tests/Makefile.am index 9e0221d3b5..3038cf7d81 100644 --- a/src/bin/dhcp4/tests/Makefile.am +++ b/src/bin/dhcp4/tests/Makefile.am @@ -103,6 +103,7 @@ dhcp4_unittests_SOURCES += simple_parser4_unittest.cc dhcp4_unittests_SOURCES += get_config_unittest.cc get_config_unittest.h dhcp4_unittests_SOURCES += shared_network_unittest.cc dhcp4_unittests_SOURCES += host_unittest.cc +dhcp4_unittests_SOURCES += vendor_opts_unittest.cc nodist_dhcp4_unittests_SOURCES = marker_file.h test_libraries.h diff --git a/src/bin/dhcp4/tests/vendor_opts_unittest.cc b/src/bin/dhcp4/tests/vendor_opts_unittest.cc new file mode 100644 index 0000000000..6201a763ed --- /dev/null +++ b/src/bin/dhcp4/tests/vendor_opts_unittest.cc @@ -0,0 +1,178 @@ +// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +// This file is dedicated to testing vendor options. There are several +// vendor options in DHCPv4: +// +// vivso (125) - vendor independent vendor specific option. This is by far the +// most popular +// vendor specific (43) - this is probably the second most popular. +// Unfortunately, its definition is blurry, so there are many +// similar, but not exact implementations that do things in +// different ways. +// vivco (124) - vendor indepdent vendor class option. +// class identifier (60) - not exactly vendor specific. It's a string, but the +// content of that string identifies what kind of vendor device +// this is. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +using namespace isc::asiolink; +using namespace isc::dhcp; +using namespace isc::config; +using namespace isc::dhcp::test; + +/// @brief Class dedicated to testing vendor options in DHCPv4 +/// +/// For the time being it does not provide any additional functionality, but it +/// groups all vendor related tests under a single name. There were too many +/// tests in Dhcpv4SrvTest class anyway. +class VendorOptsTest : public Dhcpv4SrvTest { + +}; + +/// Checks that it's possible to define and use a suboption 0. +TEST_F(VendorOptsTest, vendorOpsSubOption0) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + NakedDhcpv4Srv srv(0); + + // Zero Touch provisioning + string config = + "{" + " \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + " }," + " \"option-def\": [" + " {" + " \"name\": \"vendor-encapsulated-options\"," + " \"code\": 43," + " \"type\": \"empty\"," + " \"encapsulate\": \"ZTP\"" + " }," + " {" + " \"name\": \"config-file-name\"," + " \"code\": 1," + " \"space\": \"ZTP\"," + " \"type\": \"string\"" + " }," + " {" + " \"name\": \"image-file-name\"," + " \"code\": 0," + " \"space\": \"ZTP\"," + " \"type\": \"string\"" + " }," + " {" + " \"name\": \"image-file-type\"," + " \"code\": 2," + " \"space\": \"ZTP\"," + " \"type\": \"string\"" + " }," + " {" + " \"name\": \"transfer-mode\"," + " \"code\": 3," + " \"space\": \"ZTP\"," + " \"type\": \"string\"" + " }," + " {" + " \"name\": \"all-image-file-name\"," + " \"code\": 4," + " \"space\": \"ZTP\"," + " \"type\": \"string\"" + " }," + " {" + " \"name\": \"http-port\"," + " \"code\": 5," + " \"space\": \"ZTP\"," + " \"type\": \"string\"" + " }" + " ]," + " \"option-data\": [" + " {" + " \"name\": \"vendor-encapsulated-options\"" + " }," + " {" + " \"name\": \"image-file-name\"," + " \"data\": \"/dist/images/jinstall-ex.tgz\"," + " \"space\": \"ZTP\"" + " }" + " ]," + "\"subnet4\": [ { " + " \"pools\": [ { \"pool\": \"192.0.2.1 - 192.0.2.100\" } ]," + " \"subnet\": \"192.0.2.0/24\"" + " } ]" + "}"; + + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP4(config, true)); + ConstElementPtr status; + + // Configure the server and make sure the config is accepted + EXPECT_NO_THROW(status = configureDhcp4Server(srv, json)); + ASSERT_TRUE(status); + comment_ = parseAnswer(rcode_, status); + ASSERT_EQ(0, rcode_); + + CfgMgr::instance().commit(); + + // Create a packet with enough to select the subnet and go through + // the DISCOVER processing + Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 1234)); + query->setRemoteAddr(IOAddress("192.0.2.1")); + OptionPtr clientid = generateClientId(); + query->addOption(clientid); + query->setIface("eth1"); + + // Create and add a PRL option to the query + OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4, + DHO_DHCP_PARAMETER_REQUEST_LIST)); + ASSERT_TRUE(prl); + prl->addValue(DHO_VENDOR_ENCAPSULATED_OPTIONS); + prl->addValue(DHO_VENDOR_CLASS_IDENTIFIER); + query->addOption(prl); + + srv.classifyPacket(query); + ASSERT_NO_THROW(srv.deferredUnpack(query)); + + // Pass it to the server and get an offer + Pkt4Ptr offer = srv.processDiscover(query); + + // Check if we get response at all + checkResponse(offer, DHCPOFFER, 1234); + + // Processing should add a vendor-encapsulated-options (code 43) + OptionPtr opt = offer->getOption(DHO_VENDOR_ENCAPSULATED_OPTIONS); + ASSERT_TRUE(opt); + const OptionCollection& opts = opt->getOptions(); + ASSERT_EQ(1, opts.size()); + OptionPtr sopt = opts.begin()->second; + ASSERT_TRUE(sopt); + EXPECT_EQ(0, sopt->getType()); + + // Check suboption 0 content. + OptionStringPtr sopt0 = + boost::dynamic_pointer_cast(sopt); + ASSERT_TRUE(sopt0); + EXPECT_EQ("/dist/images/jinstall-ex.tgz", sopt0->getValue()); +} diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index ced705dd29..6e3e274132 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -472,6 +472,9 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, size_t offset = 0; size_t last_offset = 0; + // Special case when option_space is dhcp4. + bool space_is_dhcp4 = (option_space == DHCP4_OPTION_SPACE); + // Get the list of standard option definitions. const OptionDefContainerPtr& option_defs = LibDHCP::getOptionDefs(option_space); // Runtime option definitions for non standard option space and if @@ -493,7 +496,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, uint8_t opt_type = buf[offset++]; // DHO_END is a special, one octet long option - if (opt_type == DHO_END) { + if (space_is_dhcp4 && (opt_type == DHO_END)) { // just return. Don't need to add DHO_END option // Don't return offset because it makes this condition // and partial parsing impossible to recognize. @@ -502,8 +505,9 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, // DHO_PAD is just a padding after DHO_END. Let's continue parsing // in case we receive a message without DHO_END. - if (opt_type == DHO_PAD) + if (space_is_dhcp4 && (opt_type == DHO_PAD)) { continue; + } if (offset + 1 > buf.size()) { // We peeked at the option header of the next option, but diff --git a/src/lib/dhcp/option.cc b/src/lib/dhcp/option.cc index 5d7320780b..b6904289eb 100644 --- a/src/lib/dhcp/option.cc +++ b/src/lib/dhcp/option.cc @@ -35,12 +35,7 @@ Option::factory(Option::Universe u, Option::Option(Universe u, uint16_t type) :universe_(u), type_(type) { - - // END option (type 255 is forbidden as well) - if ((u == V4) && ((type == 0) || (type > 254))) { - isc_throw(BadValue, "Can't create V4 option of type " - << type << ", V4 options are in range 1..254"); - } + check(); } Option::Option(Universe u, uint16_t type, const OptionBuffer& data) diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 89d5e65092..866b91823d 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -2156,4 +2156,95 @@ TEST_F(LibDhcpTest, sw46options) { EXPECT_EQ("type=00093, len=00004: 8 (uint8) len=6,psid=63 (psid)", portparam->toText()); } +// Verifies that options 0 (PAD) and 255 (END) are handled as PAD and END +// in and only in the dhcp4 space. +TEST_F(LibDhcpTest, unpackPadEnd) { + // Create option definition for the container. + OptionDefinitionPtr opt_def(new OptionDefinition("container", 200, + "empty", "my-space")); + // Create option definition for option 0. + OptionDefinitionPtr opt_def0(new OptionDefinition("zero", 0, "uint8")); + + // Create option definition for option 255. + OptionDefinitionPtr opt_def255(new OptionDefinition("max", 255, "uint8")); + + // Create option definition for another option. + OptionDefinitionPtr opt_def2(new OptionDefinition("my-option", 1, + "string")); + + // Register created option definitions as runtime option definitions. + OptionDefSpaceContainer defs; + ASSERT_NO_THROW(defs.addItem(opt_def, DHCP4_OPTION_SPACE)); + ASSERT_NO_THROW(defs.addItem(opt_def0, "my-space")); + ASSERT_NO_THROW(defs.addItem(opt_def255, "my-space")); + ASSERT_NO_THROW(defs.addItem(opt_def2, "my-space")); + LibDHCP::setRuntimeOptionDefs(defs); + LibDHCP::commitRuntimeOptionDefs(); + + // Create the buffer holding the structure of options. + const uint8_t raw_data[] = { + // Add a PAD + 0x00, // option code = 0 (PAD) + // Container option starts here. + 0xc8, // option code = 200 (container) + 0x0b, // option length = 11 + // Suboption 0. + 0x00, 0x01, 0x00, // code = 0, length = 1, content = 0 + // Suboption 255. + 0xff, 0x01, 0xff, // code = 255, length = 1, content = 255 + // Suboption 1. + 0x01, 0x03, 0x66, 0x6f, 0x6f, // code = 1, length = 2, content = "foo" + // END + 0xff, + // Extra bytes at tail. + 0x01, 0x02, 0x03, 0x04 + }; + size_t raw_data_len = sizeof(raw_data) / sizeof(uint8_t); + OptionBuffer buf(raw_data, raw_data + raw_data_len); + + // Parse options. + OptionCollection options; + list deferred; + size_t offset = 0; + ASSERT_NO_THROW(offset = LibDHCP::unpackOptions4(buf, DHCP4_OPTION_SPACE, + options, deferred)); + + // Returned offset should point to the END. + EXPECT_EQ(0xff, raw_data[offset]); + + // There should be one top level option. + ASSERT_EQ(1, options.size()); + + // Get it. + OptionPtr option = options.begin()->second; + ASSERT_TRUE(option); + EXPECT_EQ(200, option->getType()); + + // There should be 3 suboptions. + EXPECT_EQ(3, option->getOptions().size()); + + // Get suboption 0. + boost::shared_ptr > sub0 = + boost::dynamic_pointer_cast > + (option->getOption(0)); + ASSERT_TRUE(sub0); + EXPECT_EQ(0, sub0->getType()); + EXPECT_EQ(0, sub0->getValue()); + + // Get suboption 255. + boost::shared_ptr > sub255 = + boost::dynamic_pointer_cast > + (option->getOption(255)); + ASSERT_TRUE(sub255); + EXPECT_EQ(255, sub255->getType()); + EXPECT_EQ(255, sub255->getValue()); + + // Get suboption 1. + boost::shared_ptr sub = + boost::dynamic_pointer_cast(option->getOption(1)); + ASSERT_TRUE(sub); + EXPECT_EQ(1, sub->getType()); + EXPECT_EQ("foo", sub->getValue()); +} + } // end of anonymous space diff --git a/src/lib/dhcp/tests/option_unittest.cc b/src/lib/dhcp/tests/option_unittest.cc index 0e282072ce..4b140270ba 100644 --- a/src/lib/dhcp/tests/option_unittest.cc +++ b/src/lib/dhcp/tests/option_unittest.cc @@ -70,13 +70,11 @@ TEST_F(OptionTest, v4_basic) { EXPECT_NO_THROW(opt.reset()); // V4 options have type 0...255 - EXPECT_THROW(opt.reset(new Option(Option::V4, 256)), BadValue); + EXPECT_THROW(opt.reset(new Option(Option::V4, 256)), OutOfRange); - // 0 is a special PAD option - EXPECT_THROW(opt.reset(new Option(Option::V4, 0)), BadValue); - - // 255 is a special END option - EXPECT_THROW(opt.reset(new Option(Option::V4, 255)), BadValue); + // 0 (PAD) and 255 (END) are no longer forbidden + EXPECT_NO_THROW(opt.reset(new Option(Option::V4, 0))); + EXPECT_NO_THROW(opt.reset(new Option(Option::V4, 255))); } const uint8_t dummyPayload[] = diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc index 28f8c57958..d40ac052fe 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc @@ -1,8 +1,9 @@ // Copyright (C) 2013-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include +// License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include #include +#include #include #include #include @@ -130,6 +131,29 @@ OptionDefParser::parse(ConstElementPtr option_def) { << getPosition("space", option_def) << ")"); } + // Protect against definition of options 0 (PAD) or 255 (END) + // in (and only in) the dhcp4 space. + if (space == DHCP4_OPTION_SPACE) { + if (code == DHO_PAD) { + isc_throw(DhcpConfigError, "invalid option code '0': " + << "reserved for PAD (" + << getPosition("code", option_def) << ")"); + } else if (code == DHO_END) { + isc_throw(DhcpConfigError, "invalid option code '255': " + << "reserved for END (" + << getPosition("code", option_def) << ")"); + } + } + + // For dhcp6 space the value 0 is reserved. + if (space == DHCP6_OPTION_SPACE) { + if (code == 0) { + isc_throw(DhcpConfigError, "invalid option code '0': " + << "reserved value (" + << getPosition("code", option_def) << ")"); + } + } + // Create option definition. OptionDefinitionPtr def; // We need to check if user has set encapsulated option space diff --git a/src/lib/dhcpsrv/parsers/option_data_parser.cc b/src/lib/dhcpsrv/parsers/option_data_parser.cc index 97c34e0970..1764114fbb 100644 --- a/src/lib/dhcpsrv/parsers/option_data_parser.cc +++ b/src/lib/dhcpsrv/parsers/option_data_parser.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -59,14 +60,10 @@ OptionDataParser::extractCode(ConstElementPtr parent) const { return (OptionalValue()); } - if (code == 0) { - isc_throw(DhcpConfigError, "option code must not be zero " - "(" << getPosition("code", parent) << ")"); - - } else if (address_family_ == AF_INET && - code > std::numeric_limits::max()) { + if (address_family_ == AF_INET && + code > std::numeric_limits::max()) { isc_throw(DhcpConfigError, "invalid option code '" << code - << "', it must not be greater than '" + << "', it must not be greater than '" << static_cast(std::numeric_limits::max()) << "' (" << getPosition("code", parent) << ")"); @@ -74,7 +71,7 @@ OptionDataParser::extractCode(ConstElementPtr parent) const { } else if (address_family_ == AF_INET6 && code > std::numeric_limits::max()) { isc_throw(DhcpConfigError, "invalid option code '" << code - << "', it must not exceed '" + << "', it must not exceed '" << std::numeric_limits::max() << "' (" << getPosition("code", parent) << ")"); @@ -312,7 +309,6 @@ OptionDataParser::createOption(ConstElementPtr option_data) { } } - OptionPtr option; OptionDescriptor desc(false); if (!def) { @@ -360,6 +356,28 @@ OptionDataParser::createOption(ConstElementPtr option_data) { } } + // Check PAD and END in (and only in) dhcp4 space. + if (space_param == DHCP4_OPTION_SPACE) { + if (desc.option_->getType() == DHO_PAD) { + isc_throw(DhcpConfigError, "invalid option code '0': " + << "reserved for PAD (" + << option_data->getPosition() << ")"); + } else if (desc.option_->getType() == DHO_END) { + isc_throw(DhcpConfigError, "invalid option code '255': " + << "reserved for END (" + << option_data->getPosition() << ")"); + } + } + + // For dhcp6 space the value 0 is reserved. + if (space_param == DHCP6_OPTION_SPACE) { + if (desc.option_->getType() == 0) { + isc_throw(DhcpConfigError, "invalid option code '0': " + << "reserved value (" + << option_data->getPosition() << ")"); + } + } + // Add user context if (user_context) { desc.setContext(user_context); diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index c750b6866f..fc0d0a6679 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -677,6 +677,60 @@ TEST_F(ParseConfigTest, defaultSpaceOptionDefTest) { cfg.runCfgOptionsTest(family_, config); } +/// @brief Check parsing of option definitions using invalid code fails. +TEST_F(ParseConfigTest, badCodeOptionDefTest) { + + { + SCOPED_TRACE("conflict with PAD"); + family_ = AF_INET; // Switch to DHCPv4. + + std::string config = + "{ \"option-def\": [ {" + " \"name\": \"zero\"," + " \"code\": 0," + " \"type\": \"ip-address\"," + " \"space\": \"dhcp4\"" + " } ]" + "}"; + + int rcode = parseConfiguration(config, false); + ASSERT_NE(0, rcode); + } + + { + SCOPED_TRACE("conflict with END"); + family_ = AF_INET; // Switch to DHCPv4. + + std::string config = + "{ \"option-def\": [ {" + " \"name\": \"max\"," + " \"code\": 255," + " \"type\": \"ip-address\"," + " \"space\": \"dhcp4\"" + " } ]" + "}"; + + int rcode = parseConfiguration(config, false); + ASSERT_NE(0, rcode); + } + + { + SCOPED_TRACE("conflict with reserved"); + + std::string config = + "{ \"option-def\": [ {" + " \"name\": \"zero\"," + " \"code\": 0," + " \"type\": \"ipv6-address\"," + " \"space\": \"dhcp6\"" + " } ]" + "}"; + + int rcode = parseConfiguration(config, false); + ASSERT_NE(0, rcode); + } +} + /// @brief Check parsing of option definitions using invalid space fails. TEST_F(ParseConfigTest, badSpaceOptionDefTest) { @@ -779,6 +833,84 @@ TEST_F(ParseConfigTest, minimalOptionDataTest) { cfg.runCfgOptionsTest(family_, expected); } +/// @brief Check parsing of options with code 0. +TEST_F(ParseConfigTest, optionDataTest0) { + + // Configuration string. + std::string config = + "{ \"option-def\": [ {" + " \"name\": \"foo\"," + " \"code\": 0," + " \"type\": \"ipv4-address\"," + " \"space\": \"isc\"" + " } ], " + " \"option-data\": [ {" + " \"name\": \"foo\"," + " \"space\": \"isc\"," + " \"code\": 0," + " \"data\": \"192.0.2.0\"," + " \"csv-format\": true," + " \"always-send\": false" + " } ]" + "}"; + + // Verify that the configuration string parses. + int rcode = parseConfiguration(config); + ASSERT_EQ(0, rcode); + + // Verify that the option can be retrieved. + OptionPtr opt_ptr = getOptionPtr("isc", 0); + ASSERT_TRUE(opt_ptr); + + // Verify that the option data is correct. + std::string val = "type=00000, len=00004: 192.0.2.0 (ipv4-address)"; + + EXPECT_EQ(val, opt_ptr->toText()); + + // Check if it can be unparsed. + CfgOptionsTest cfg(CfgMgr::instance().getStagingCfg()); + cfg.runCfgOptionsTest(family_, config); +} + +/// @brief Check parsing of options with code 255. +TEST_F(ParseConfigTest, optionDataTest255) { + + // Configuration string. + std::string config = + "{ \"option-def\": [ {" + " \"name\": \"foo\"," + " \"code\": 255," + " \"type\": \"ipv4-address\"," + " \"space\": \"isc\"" + " } ], " + " \"option-data\": [ {" + " \"name\": \"foo\"," + " \"space\": \"isc\"," + " \"code\": 255," + " \"data\": \"192.0.2.0\"," + " \"csv-format\": true," + " \"always-send\": false" + " } ]" + "}"; + + // Verify that the configuration string parses. + int rcode = parseConfiguration(config); + ASSERT_EQ(0, rcode); + + // Verify that the option can be retrieved. + OptionPtr opt_ptr = getOptionPtr("isc", 255); + ASSERT_TRUE(opt_ptr); + + // Verify that the option data is correct. + std::string val = "type=00255, len=00004: 192.0.2.0 (ipv4-address)"; + + EXPECT_EQ(val, opt_ptr->toText()); + + // Check if it can be unparsed. + CfgOptionsTest cfg(CfgMgr::instance().getStagingCfg()); + cfg.runCfgOptionsTest(family_, config); +} + /// @brief Check parsing of unknown options fails. TEST_F(ParseConfigTest, unknownOptionDataTest) { -- 2.47.2