From: Thomas Markwalder Date: Mon, 21 Oct 2019 14:38:53 +0000 (-0400) Subject: [#900,!561] kea-dhcp4/6 now quietly drop empty or all-null string options X-Git-Tag: Kea-1.7.1~54 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a917e4ae7e36e1273d3be5c370a1d3d705ded22b;p=thirdparty%2Fkea.git [#900,!561] kea-dhcp4/6 now quietly drop empty or all-null string options src/lib/dhcp/option.h class SkipThisOptionError - new exception type src/lib/dhcp/libdhcp++.cc LibDHCP::unpackOptions4() LibDHCP::unpackOptions6() - explicitly catches and handles SkipThisOptionError expceptions src/lib/dhcp/option_definition.cc OptionDefinition::optionFactory() - now rethrows SkipThisOptionError src/lib/dhcp/option_int.h OptionInt::unpack() - altered ambiguous exception text src/lib/dhcp/option_int_array.h OptionIntArray::unpack() - altered ambiguous exception text src/lib/dhcp/option_string.cc OptionString::unpack() - now throws SkipThisOptionError if option, once trimmed, is empty src/lib/dhcp/tests/option_string_unittest.cc Updated tests src/lib/dhcp/tests/pkt4_unittest.cc TEST_F(Pkt4Test, testSkipThisOptionError) - new test src/lib/dhcp/tests/pkt6_unittest.cc TEST_F(Pkt6Test, testSkipThisOptionError) - new test src/lib/dhcpsrv/tests/cfg_option_unittest.cc Updated expected exception text src/lib/testutils/gtest_utils.h Added two macros to emit exception info on throws. #define EXPECT_NO_THROW_LOG(statement) #define ASSERT_NO_THROW_LOG(statement) --- diff --git a/ChangeLog b/ChangeLog index 8afab76547..88d43231eb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +1673. [bug] tmark + Fixed a bug introduced in Kea 1.6.0 (see #539) that caused + kea-dhcp4 and kea-dhcp6 to discard inbound packets containing + string options that consist solely of nulls. The servers + will now quiely omit empty or all-null string options from + inbound packets. + (Gitlab #900, !561 git TBD) + 1672. [build] fdupont Deprecated bind1st and bind2nd templates were replaced with lambda expressions or plain bind templates. diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index a55313e618..44f06ed436 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -448,16 +448,24 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, buf.begin() + offset, buf.begin() + offset + opt_len)); } else { - // The option definition has been found. Use it to create - // the option instance from the provided buffer chunk. - const OptionDefinitionPtr& def = *(range.first); - assert(def); - opt = def->optionFactory(Option::V6, opt_type, - buf.begin() + offset, - buf.begin() + offset + opt_len); + try { + // The option definition has been found. Use it to create + // the option instance from the provided buffer chunk. + const OptionDefinitionPtr& def = *(range.first); + assert(def); + opt = def->optionFactory(Option::V6, opt_type, + buf.begin() + offset, + buf.begin() + offset + opt_len); + } catch (const SkipThisOptionError& ex) { + opt.reset(); + } } + // add option to options - options.insert(std::make_pair(opt_type, opt)); + if (opt) { + options.insert(std::make_pair(opt_type, opt)); + } + offset += opt_len; } @@ -583,16 +591,24 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, buf.begin() + offset + opt_len)); opt->setEncapsulatedSpace(DHCP4_OPTION_SPACE); } else { - // The option definition has been found. Use it to create - // the option instance from the provided buffer chunk. - const OptionDefinitionPtr& def = *(range.first); - assert(def); - opt = def->optionFactory(Option::V4, opt_type, - buf.begin() + offset, - buf.begin() + offset + opt_len); + try { + // The option definition has been found. Use it to create + // the option instance from the provided buffer chunk. + const OptionDefinitionPtr& def = *(range.first); + assert(def); + opt = def->optionFactory(Option::V4, opt_type, + buf.begin() + offset, + buf.begin() + offset + opt_len); + } catch (const SkipThisOptionError& ex) { + opt.reset(); + } + } + + // If we have the option, insert it + if (opt) { + options.insert(std::make_pair(opt_type, opt)); } - options.insert(std::make_pair(opt_type, opt)); offset += opt_len; } last_offset = offset; diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h index fad71e5108..1d484f46e1 100644 --- a/src/lib/dhcp/option.h +++ b/src/lib/dhcp/option.h @@ -55,6 +55,13 @@ public: isc::Exception(file, line, what) { }; }; +class SkipThisOptionError : public Exception { +public: + SkipThisOptionError (const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { }; +}; + + class Option { public: /// length of the usual DHCPv4 option header (there are exceptions) diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index c886e71222..1297e4a608 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/src/lib/dhcp/option_definition.cc @@ -264,6 +264,9 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type, ; } return (OptionPtr(new OptionCustom(*this, u, begin, end))); + } catch (const SkipThisOptionError& ex) { + // We need to throw this one as is. + throw ex; } catch (const SkipRemainingOptionsError& ex) { // We need to throw this one as is. throw ex; diff --git a/src/lib/dhcp/option_int.h b/src/lib/dhcp/option_int.h index 05224307f1..dc2994661c 100644 --- a/src/lib/dhcp/option_int.h +++ b/src/lib/dhcp/option_int.h @@ -146,7 +146,7 @@ public: /// because it is checked in a constructor. virtual void unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { if (distance(begin, end) < sizeof(T)) { - isc_throw(OutOfRange, "Option " << getType() << " truncated"); + isc_throw(OutOfRange, "OptionInt " << getType() << " truncated"); } // @todo consider what to do if buffer is longer than data type. diff --git a/src/lib/dhcp/option_int_array.h b/src/lib/dhcp/option_int_array.h index ef379bb5c0..c72b1338ed 100644 --- a/src/lib/dhcp/option_int_array.h +++ b/src/lib/dhcp/option_int_array.h @@ -186,7 +186,7 @@ public: isc_throw(OutOfRange, "option " << getType() << " empty"); } if (distance(begin, end) % sizeof(T) != 0) { - isc_throw(OutOfRange, "option " << getType() << " truncated"); + isc_throw(OutOfRange, "OptionIntArray " << getType() << " truncated"); } // @todo consider what to do if buffer is longer than data type. diff --git a/src/lib/dhcp/option_string.cc b/src/lib/dhcp/option_string.cc index 68b80a40f0..64d4499350 100644 --- a/src/lib/dhcp/option_string.cc +++ b/src/lib/dhcp/option_string.cc @@ -88,7 +88,7 @@ OptionString::unpack(OptionBufferConstIter begin, // Trim off trailing null(s) end = util::str::seekTrimmed(begin, end, 0x0); if (std::distance(begin, end) == 0) { - isc_throw(isc::OutOfRange, "failed to parse an option '" + isc_throw(isc::dhcp::SkipThisOptionError, "failed to parse an option '" << getType() << "' holding string value" << "' was empty or contained only NULLs"); } diff --git a/src/lib/dhcp/tests/option_string_unittest.cc b/src/lib/dhcp/tests/option_string_unittest.cc index ae9346e1fc..0020a7878a 100644 --- a/src/lib/dhcp/tests/option_string_unittest.cc +++ b/src/lib/dhcp/tests/option_string_unittest.cc @@ -71,14 +71,14 @@ TEST_F(OptionStringTest, constructorFromBuffer) { // an exception. EXPECT_THROW( OptionString(Option::V4, 234, buf_.begin(), buf_.begin()), - isc::OutOfRange + isc::dhcp::SkipThisOptionError ); // NULLs should result in an exception. std::vectornulls = { 0, 0, 0 }; EXPECT_THROW( OptionString(Option::V4, 234, nulls.begin(), nulls.begin()), - isc::OutOfRange + isc::dhcp::SkipThisOptionError ); // Declare option as a scoped pointer here so as its scope is @@ -211,7 +211,7 @@ TEST_F(OptionStringTest, unpackNullsHandling) { // Only nulls should throw. OptionBuffer buffer = { 0, 0 }; - ASSERT_THROW(optv4.unpack(buffer.begin(), buffer.end()), isc::OutOfRange); + ASSERT_THROW(optv4.unpack(buffer.begin(), buffer.end()), isc::dhcp::SkipThisOptionError); // One trailing null should trim off. buffer = {'o', 'n', 'e', 0 }; diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index 89c6142089..f33d1c001d 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -1180,7 +1181,7 @@ TEST_F(Pkt4Test, getType) { TEST_F(Pkt4Test, truncatedVendorLength) { // Build a good discover packet - Pkt4Ptr pkt = test::PktCaptures::discoverWithValidVIVSO(); + Pkt4Ptr pkt = dhcp::test::PktCaptures::discoverWithValidVIVSO(); // Unpacking should not throw ASSERT_NO_THROW(pkt->unpack()); @@ -1195,7 +1196,7 @@ TEST_F(Pkt4Test, truncatedVendorLength) { EXPECT_EQ(133+2, vivso->len()); // data + opt code + len // Build a bad discover packet - pkt = test::PktCaptures::discoverWithTruncatedVIVSO(); + pkt = dhcp::test::PktCaptures::discoverWithTruncatedVIVSO(); // Unpack should throw Skip exception ASSERT_THROW(pkt->unpack(), SkipRemainingOptionsError); @@ -1299,4 +1300,58 @@ TEST_F(Pkt4Test, nullTerminatedOptions) { EXPECT_EQ(0, memcmp(&packed[base_size], &packed_opts[0], packed_opts.size())); } +// Checks that unpacking correctly handles SkipThisOptionError by +// omitting the offending option from the unpacked options. +TEST_F(Pkt4Test, testSkipThisOptionError) { + vector orig = generateTestPacket2(); + + orig.push_back(0x63); + orig.push_back(0x82); + orig.push_back(0x53); + orig.push_back(0x63); + + orig.push_back(53); // Message Type + orig.push_back(1); // length=1 + orig.push_back(2); // type=2 + + orig.push_back(14); // merit-dump + orig.push_back(3); // length=3 + orig.push_back(0x61); // data="abc" + orig.push_back(0x62); + orig.push_back(0x63); + + orig.push_back(12); // Hostname + orig.push_back(3); // length=3 + orig.push_back(0); // data= all nulls + orig.push_back(0); + orig.push_back(0); + + orig.push_back(17); // root-path + orig.push_back(3); // length=3 + orig.push_back(0x64); // data="def" + orig.push_back(0x65); + orig.push_back(0x66); + + // Unpacking should not throw. + Pkt4Ptr pkt(new Pkt4(&orig[0], orig.size())); + ASSERT_NO_THROW_LOG(pkt->unpack()); + + // We should have option 14 = "abc". + OptionPtr opt; + OptionStringPtr opstr; + ASSERT_TRUE(opt = pkt->getOption(14)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("abc", opstr->getValue()); + + // We should not have option 12. + EXPECT_FALSE(opt = pkt->getOption(12)); + + // We should have option 17 = "def". + ASSERT_TRUE(opt = pkt->getOption(17)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("def", opstr->getValue()); +} + } // end of anonymous namespace diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc index 042ffe8916..fefcf0b2b2 100644 --- a/src/lib/dhcp/tests/pkt6_unittest.cc +++ b/src/lib/dhcp/tests/pkt6_unittest.cc @@ -13,12 +13,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -1581,7 +1583,7 @@ TEST_F(Pkt6Test, getMACFromRemoteIdRelayOption) { // (Relay Supplied Options option). This test checks whether it was parsed // properly. See captureRelayed2xRSOO() description for details. TEST_F(Pkt6Test, rsoo) { - Pkt6Ptr msg = test::PktCaptures::captureRelayed2xRSOO(); + Pkt6Ptr msg = dhcp::test::PktCaptures::captureRelayed2xRSOO(); EXPECT_NO_THROW(msg->unpack()); @@ -1728,7 +1730,7 @@ TEST_F(Pkt6Test, getLabelEmptyClientId) { TEST_F(Pkt6Test, truncatedVendorLength) { // Build a good Solicit packet - Pkt6Ptr pkt = test::PktCaptures::captureSolicitWithVIVSO(); + Pkt6Ptr pkt = dhcp::test::PktCaptures::captureSolicitWithVIVSO(); // Unpacking should not throw ASSERT_NO_THROW(pkt->unpack()); @@ -1743,7 +1745,7 @@ TEST_F(Pkt6Test, truncatedVendorLength) { EXPECT_EQ(8, vivso->len()); // data + opt code + len // Build a bad Solicit packet - pkt = test::PktCaptures::captureSolicitWithTruncatedVIVSO(); + pkt = dhcp::test::PktCaptures::captureSolicitWithTruncatedVIVSO(); // Unpack should throw Skip exception ASSERT_THROW(pkt->unpack(), SkipRemainingOptionsError); @@ -1754,4 +1756,60 @@ TEST_F(Pkt6Test, truncatedVendorLength) { ASSERT_FALSE(x); } +// Checks that unpacking correctly handles SkipThisOptionError by +// omitting the offending option from the unpacked options. +TEST_F(Pkt6Test, testSkipThisOptionError) { + // Get a packet. We're really interested in its on-wire + // representation only. + Pkt6Ptr donor(capture1()); + + // That's our original content. It should be sane. + OptionBuffer orig = donor->data_; + + orig.push_back(0); + orig.push_back(41); // new-posix-timezone + orig.push_back(0); + orig.push_back(3); // length=3 + orig.push_back(0x61); // data="abc" + orig.push_back(0x62); + orig.push_back(0x63); + + orig.push_back(0); + orig.push_back(59); // bootfile-url + orig.push_back(0); + orig.push_back(3); // length=3 + orig.push_back(0); // data= all nulls + orig.push_back(0); + orig.push_back(0); + + orig.push_back(0); + orig.push_back(42); // new-tzdb-timezone + orig.push_back(0); + orig.push_back(3); // length=3 + orig.push_back(0x64); // data="def" + orig.push_back(0x65); + orig.push_back(0x66); + + // Unpacking should not throw. + Pkt6Ptr pkt(new Pkt6(&orig[0], orig.size())); + ASSERT_NO_THROW_LOG(pkt->unpack()); + + // We should have option 41 = "abc". + OptionPtr opt; + OptionStringPtr opstr; + ASSERT_TRUE(opt = pkt->getOption(41)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("abc", opstr->getValue()); + + // We should not have option 59. + EXPECT_FALSE(opt = pkt->getOption(59)); + + // We should have option 42 = "def". + ASSERT_TRUE(opt = pkt->getOption(42)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("def", opstr->getValue()); +} + } diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 5c67bd9427..f2f8dc72be 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -552,16 +553,9 @@ TEST_F(CfgOptionTest, mergeInvalid) { // When we attempt to merge, it should fail, recognizing that // option 2, which has a formatted value, has no definition. - try { - this_cfg.merge(defs, other_cfg); - GTEST_FAIL() << "merge should have thrown"; - } catch (const InvalidOperation& ex) { - std::string exp_msg = "option: isc.2 has a formatted value: " - "'one,two,three' but no option definition"; - EXPECT_EQ(std::string(exp_msg), std::string(ex.what())); - } catch (const std::exception& ex) { - GTEST_FAIL() << "wrong exception thrown:" << ex.what(); - } + ASSERT_THROW_MSG(this_cfg.merge(defs, other_cfg), InvalidOperation, + "option: isc.2 has a formatted value: " + "'one,two,three' but no option definition"); // Now let's add an option definition that will force data truncated // error for option 1. @@ -569,17 +563,10 @@ TEST_F(CfgOptionTest, mergeInvalid) { // When we attempt to merge, it should fail because option 1's data // is not valid per its definition. - try { - this_cfg.merge(defs, other_cfg); - GTEST_FAIL() << "merge should have thrown"; - } catch (const InvalidOperation& ex) { - std::string exp_msg = "could not create option: isc.1" - " from data specified, reason:" - " Option 1 truncated"; - EXPECT_EQ(std::string(exp_msg), std::string(ex.what())); - } catch (const std::exception& ex) { - GTEST_FAIL() << "wrong exception thrown:" << ex.what(); - } + EXPECT_THROW_MSG(this_cfg.merge(defs, other_cfg), InvalidOperation, + "could not create option: isc.1" + " from data specified, reason:" + " OptionInt 1 truncated"); } // This test verifies the all of the valid option cases diff --git a/src/lib/testutils/gtest_utils.h b/src/lib/testutils/gtest_utils.h index 7f7bfb4839..13c1c0d122 100644 --- a/src/lib/testutils/gtest_utils.h +++ b/src/lib/testutils/gtest_utils.h @@ -4,8 +4,8 @@ // 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/. -#ifndef GTEST_UTIL_H -#define GTEST_UTIL_H +#ifndef GTEST_UTILS_H +#define GTEST_UTILS_H #include @@ -54,7 +54,37 @@ namespace test { } \ } \ +/// @brief Adds a non-fatal failure with exception info, if the given +/// expression throws +/// +/// @param statement - statement block to execute +#define EXPECT_NO_THROW_LOG(statement) \ +{ \ + try { \ + statement; \ + } catch (const std::exception& ex) { \ + ADD_FAILURE() << #statement << "threw: " << ex.what(); \ + } catch (...) { \ + ADD_FAILURE() << #statement << "threw non-std::exception"; \ + } \ +} \ + +/// @brief Generates a fatal failure with exception info, if the given +/// expression throws +/// +/// @param statement - statement block to execute +#define ASSERT_NO_THROW_LOG(statement) \ +{ \ + try { \ + statement; \ + } catch (const std::exception& ex) { \ + GTEST_FAIL() << #statement << "threw: " << ex.what(); \ + } catch (...) { \ + GTEST_FAIL() << #statement << "threw non-std::exception"; \ + } \ +} \ + }; // end of isc::test namespace }; // end of isc namespace -#endif // GTEST_UTIL_H +#endif // GTEST_UTILS_H