[#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 e2795d9593ca163947b3528fba474b7c6e980273..1c40b9cee1b245940fe40899c4c1938c2b3c96d8 100644 (file)
--- 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 c469d97efaa6687cb06a7202f6b2c199dc1f2d0f..31ccefcbf64771b056263ba5d1fa2a5fb4de7833 100644 (file)
--- 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 d73b4efa16f9998cc5c60b8eedcdc0955b18b911..e7018dd79098a689745d848296b9e55846f511e0 100644 (file)
--- 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 0c9cac91201c800e1b0534213cf01c145246a073..c41623ddb2187a472065825c8f29c773588d0e97 100644 (file)
--- 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 0d275ec450c086707c926e523f5a772eadf2d433..d6a7af859430135a4d9bcc6fbcc38ef19f376221 100644 (file)
--- 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<uint8_t> 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<uint16_t> 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<OptionInt<int32_t>>(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<uint8_t> 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<OptionInt<uint16_t>>(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 4053c1e2f59f1ae31a4a856906da0609d23180bc..f0fa1602ee2f40b096a7d58bb3e67b89aeae1873 100644 (file)
--- 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