From: Thomas Markwalder Date: Mon, 5 Aug 2024 19:53:14 +0000 (+0000) Subject: [#3492] Extend lenient parser check to FQDN types X-Git-Tag: Kea-2.7.2~87 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=da923051fa0fb9ce64d95686bf5fa4d292d779ef;p=thirdparty%2Fkea.git [#3492] Extend lenient parser check to FQDN types /src/lib/dhcp/libdhcp++.cc LibDHCP::unpackOptions4() LibDHCP::unpackOptions6() - split out throw so we can emit a more helpful log on parser errors /src/lib/dhcp/option.h Added OptionParseError exception /src/lib/dhcp/option_custom.cc OptionCustom::bufferLength() - add throw of SkipThisOptionError if lenient parsing enabled for OPT_FQDN_TYPE errors /src/lib/dhcp/option_definition.cc OptionDefinition::factoryFqdnList() - add throw of SkipThisOptionError if lenient parsing enabled /src/lib/dhcp/tests/libdhcp++_unittest.cc TEST_F(LibDhcpTest, unpackOptions4LenientFqdn) TEST_F(LibDhcpTest, unpackOptions6LenientFqdn) - new tests /src/lib/dhcp/tests/option_custom_unittest.cc TEST_F(OptionCustomTest, fqdnData) - check lenient parsing behavior --- diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index e2795d9593..1c40b9cee1 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -442,6 +442,12 @@ LibDHCP::unpackOptions6(const OptionBuffer& buf, const string& option_space, buf.begin() + offset + opt_len); } catch (const SkipThisOptionError&) { opt.reset(); + } catch (const SkipRemainingOptionsError&) { + throw; + } catch (const std::exception& ex) { + isc_throw(OptionParseError, "opt_type: " << (uint16_t)(opt_type) + << ", opt_len " << (uint16_t)(opt_len) + << " error: " << ex.what()); } } @@ -673,6 +679,12 @@ LibDHCP::unpackOptions4(const OptionBuffer& buf, const string& option_space, opt = def->optionFactory(Option::V4, opt_type, obuf); } catch (const SkipThisOptionError&) { opt.reset(); + } catch (const SkipRemainingOptionsError&) { + throw; + } catch (const std::exception& ex) { + isc_throw(OptionParseError, "opt_type: " << (uint16_t)(opt_type) + << ", opt_len: " << (uint16_t)(opt_len) + << " error: " << ex.what()); } } diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h index c469d97efa..31ccefcbf6 100644 --- a/src/lib/dhcp/option.h +++ b/src/lib/dhcp/option.h @@ -70,6 +70,11 @@ public: isc::Exception(file, line, what) { }; }; +class OptionParseError : public Exception { +public: + OptionParseError (const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { }; +}; class Option { public: diff --git a/src/lib/dhcp/option_custom.cc b/src/lib/dhcp/option_custom.cc index d73b4efa16..e7018dd790 100644 --- a/src/lib/dhcp/option_custom.cc +++ b/src/lib/dhcp/option_custom.cc @@ -226,12 +226,21 @@ OptionCustom::bufferLength(const OptionDataType data_type, bool in_array, // to obtain the length of the data is to read the FQDN. The // utility function will return the size of the buffer on success. if (data_type == OPT_FQDN_TYPE) { - std::string fqdn = - OptionDataTypeUtil::readFqdn(OptionBuffer(begin, end)); - // The size of the buffer holding an FQDN is always - // 1 byte larger than the size of the string - // representation of this FQDN. - data_size = fqdn.size() + 1; + try { + std::string fqdn = + OptionDataTypeUtil::readFqdn(OptionBuffer(begin, end)); + // The size of the buffer holding an FQDN is always + // 1 byte larger than the size of the string + // representation of this FQDN. + data_size = fqdn.size() + 1; + } catch (const std::exception& ex) { + if (Option::lenient_parsing_) { + isc_throw(SkipThisOptionError, "failed to read " + "partial domain-name from wire format"); + } + + throw; + } } else if (!definition_.getArrayType() && ((data_type == OPT_BINARY_TYPE) || (data_type == OPT_STRING_TYPE))) { diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index 0c9cac9120..c41623ddb2 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/src/lib/dhcp/option_definition.cc @@ -856,6 +856,11 @@ OptionDefinition::factoryFqdnList(Option::Universe u, out_buf.insert(out_buf.end(), label, label + read_len); } } catch (const isc::Exception& ex) { + if (Option::lenient_parsing_) { + isc_throw(SkipThisOptionError, + "invalid FQDN list content: " << ex.what()); + } + isc_throw(InvalidOptionValue, ex.what()); } } diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 0d275ec450..d6a7af8594 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -61,6 +61,7 @@ public: /// Removes runtime option definitions. LibDhcpTest() { LibDHCP::clearRuntimeOptionDefs(); + Option::lenient_parsing_ = false; } /// @brief Destructor. @@ -68,6 +69,7 @@ public: /// Removes runtime option definitions. virtual ~LibDhcpTest() { LibDHCP::clearRuntimeOptionDefs(); + Option::lenient_parsing_ = false; } /// @brief Generic factory function to create any option. @@ -3648,4 +3650,84 @@ TEST_F(LibDhcpTest, sw46options) { EXPECT_EQ("type=00093, len=00004: 8 (uint8) len=6,psid=63 (psid)", portparam->toText()); } +// Verifies that V4 lenient parsing skips ill-formed OPT_TYPE_FQDN +// and OPT_TYPE_FQDN lists. +TEST_F(LibDhcpTest, unpackOptions4LenientFqdn) { + std::vector bin = { + DHO_V4_LOST, // invalid single FQDN + 9, 3, 109, 121, 100, 111, 109, 97, 105, 110, + DHO_DOMAIN_SEARCH, // invalid FQDN list + 2, 2, 56, + DHO_TIME_OFFSET, // Valid int option + 4,0,0,0,77 + }; + + // List of parsed options will be stored here. + isc::dhcp::OptionCollection options; + + OptionBuffer buf(bin); + std::list deferred_options; + + // Parse with lenient parsing disabled. + ASSERT_FALSE(Option::lenient_parsing_); + ASSERT_THROW(LibDHCP::unpackOptions4(buf, DHCP4_OPTION_SPACE, options, + deferred_options), OptionParseError); + // There should be no parsed options. + EXPECT_EQ(0, options.size()); + + // Now parse with lenient parsing enabled. + Option::lenient_parsing_ = true; + ASSERT_NO_THROW(LibDHCP::unpackOptions4(buf, DHCP4_OPTION_SPACE, options, + deferred_options)); + + // We should have skipped DHO_V4_LOST and DHO_DOMAIN_SEARCH + // and parsed DHO_TIME_OFFSET. + EXPECT_EQ(1, options.size()); + auto opt = options.find(DHO_TIME_OFFSET); + ASSERT_FALSE(opt == options.end()); + + auto offset = boost::dynamic_pointer_cast>(opt->second); + ASSERT_TRUE(offset); + EXPECT_EQ(77, offset->getValue()); + Option::lenient_parsing_ = false; +} + +// Verifies that V6 lenient parsing skips ill-formed OPT_TYPE_FQDN +// and OPT_TYPE_FQDN lists. +TEST_F(LibDhcpTest, unpackOptions6LenientFqdn) { + std::vector bin = { + 0, D6O_V6_LOST, // Invalid single FQDN. + 0, 9, 3, 109, 121, 100, 111, 109, 97, 105, 110, + 0, D6O_SIP_SERVERS_DNS, // Invalid FQDN list + 0, 2, 2, 56, + 0, D6O_ELAPSED_TIME, // Valid uint16_t + 0, 2, 0, 77 + }; + + // List of parsed options will be stored here. + isc::dhcp::OptionCollection options; + + OptionBuffer buf(bin); + + // Parse with lenient parsing disabled. + ASSERT_FALSE(Option::lenient_parsing_); + EXPECT_THROW(LibDHCP::unpackOptions6(buf, DHCP6_OPTION_SPACE, options), OptionParseError); + EXPECT_EQ(0, options.size()); + + // There should be no parsed options. + Option::lenient_parsing_ = true; + ASSERT_NO_THROW(LibDHCP::unpackOptions6(buf, DHCP6_OPTION_SPACE, options)); + + // Now parse with lenient parsing enabled. + EXPECT_EQ(1, options.size()); + auto opt = options.find(D6O_ELAPSED_TIME); + ASSERT_FALSE(opt == options.end()); + + // We should have skipped D6O_V6_LOST and D6O_SIP_SERVERS_DNS + // and parsed D6O_ELAPSED_TIME. + auto elapsed = boost::dynamic_pointer_cast>(opt->second); + ASSERT_TRUE(elapsed); + EXPECT_EQ(77, elapsed->getValue()); +} + } // namespace diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc index 4053c1e2f5..f0fa1602ee 100644 --- a/src/lib/dhcp/tests/option_custom_unittest.cc +++ b/src/lib/dhcp/tests/option_custom_unittest.cc @@ -438,12 +438,24 @@ TEST_F(OptionCustomTest, fqdnData) { // This option should have one suboption. EXPECT_TRUE(hasV6Suboption(option.get())); + ASSERT_FALSE(Option::lenient_parsing_); // Check that the option with truncated data can't be created. EXPECT_THROW( option.reset(new OptionCustom(opt_def, Option::V6, buf.begin(), buf.begin() + 4)), isc::dhcp::BadDataTypeCast ); + + // Check lenient parsing mitigates a mal-formed option by throwing + // SkipThisOptionError. + Option::lenient_parsing_ = true; + EXPECT_THROW( + option.reset(new OptionCustom(opt_def, Option::V6, + buf.begin(), buf.begin() + 4)), + isc::dhcp::SkipThisOptionError + ); + + Option::lenient_parsing_ = false; } // The purpose of this test is to verify that the option definition comprising