[5418] Improved error handling of invalid subnet prefix lengths

src/lib/dhcpsrv/parsers/dhcp_parsers.cc
    SubnetConfigParser::createSubnet() - explicitly catch
    exceptions thrown by lexical_cast and added logic to
    catch values > 256

src/bin/dhcp4/tests/config_parser_unittest.cc
src/bin/dhcp6/tests/config_parser_unittest.cc
    badSubnetValues() - new test that checks
    several invalid subnet value scenarios

diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc
index e1e291e815b440d54394863adb9026b0b0ecc878..362604bc318991b8f4af3b6cf41e521b28f807b1 100644 (file)
--- a/src/bin/dhcp4/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp4/tests/config_parser_unittest.cc
@@ -1817,23 +1817,73 @@ TEST_F(Dhcp4ParserTest, noPools) {
 }
 
 // Goal of this test is to verify that invalid subnet fails to be parsed.
-TEST_F(Dhcp4ParserTest, badSubnet) {
+TEST_F(Dhcp4ParserTest, badSubnetValues) {
 
-    // Configuration string.
-    string config = "{ " + genIfaceConfig() + "," +
-        "\"rebind-timer\": 2000, "
-        "\"renew-timer\": 1000, "
-        "\"subnet4\": [ { "
-        "    \"pools\": [ ],"
+    // Contains parts needed for a single test scenario.
+    struct Scenario {
+        std::string description_;
+        std::string config_json_;
+        std::string exp_error_msg_;
+    };
+
+    // Vector of scenarios.
+    std::vector<Scenario> scenarios = {
+        {
+        "IP is not an address",
+        "{ \"subnet4\": [ { "
+        "    \"subnet\": \"not an address/24\" } ],"
+        "\"valid-lifetime\": 4000 }",
+        "subnet configuration failed: "
+        "Failed to convert string to address 'notanaddress': Invalid argument"
+        },
+        {
+        "IP is Invalid",
+        "{ \"subnet4\": [ { "
+        "    \"subnet\": \"256.16.1.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }",
+        "subnet configuration failed: "
+        "Failed to convert string to address '256.16.1.0': Invalid argument"
+        },
+        {
+        "Missing prefix",
+        "{ \"subnet4\": [ { "
         "    \"subnet\": \"192.0.2.0\" } ],"
-        "\"valid-lifetime\": 4000 }";
+        "\"valid-lifetime\": 4000 }",
+        "subnet configuration failed: "
+        "Invalid subnet syntax (prefix/len expected):192.0.2.0 (<string>:1:32)"
+        },
+        {
+        "Prefix not an integer (2 slashes)",
+        "{ \"subnet4\": [ { "
+        "    \"subnet\": \"192.0.2.0//24\" } ],"
+        "\"valid-lifetime\": 4000 }",
+        "subnet configuration failed: "
+        "prefix length: '/24' is not an integer (<string>:1:32)"
+        },
+        {
+        "Prefix value is insane",
+        "{ \"subnet4\": [ { "
+        "    \"subnet\": \"192.0.2.0/45938\" } ],"
+        "\"valid-lifetime\": 4000 }",
+        "subnet configuration failed: "
+        "Invalid prefix length specified for subnet: 45938 (<string>:1:32)"
+        }
+    };
 
-    ConstElementPtr json;
-    ASSERT_NO_THROW(json = parseDHCP4(config, true));
-    ConstElementPtr status;
-    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
-    checkResult(status, 1);
-    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
+    // Iterate over the list of scenarios.  Each should fail to parse with
+    // a specific error message.
+    for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) {
+        {
+            SCOPED_TRACE((*scenario).description_);
+            ConstElementPtr config;
+            ASSERT_NO_THROW(config = parseDHCP4((*scenario).config_json_))
+                            << "invalid json, broken test";
+            ConstElementPtr status;
+            EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, config));
+            checkResult(status, 1);
+            EXPECT_EQ(comment_->stringValue(), (*scenario).exp_error_msg_);
+        }
+    }
 }
 
 // Goal of this test is to verify that unknown interface fails

diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc
index 51df3143fd1c78d6e266f5ec0ac67c51c17e2d00..faa6b0961c9b834745fd06d6f10394c912793c76 100644 (file)
--- a/src/bin/dhcp6/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp6/tests/config_parser_unittest.cc
@@ -1533,6 +1533,71 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceAndInterfaceId) {
     EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
+// Goal of this test is to verify that invalid subnet fails to be parsed.
+TEST_F(Dhcp6ParserTest, badSubnetValues) {
+
+    // Contains parts needed for a single test scenario.
+    struct Scenario {
+        std::string description_;
+        std::string config_json_;
+        std::string exp_error_msg_;
+    };
+
+    // Vector of scenarios.
+    std::vector<Scenario> scenarios = {
+        {
+        "IP is not an address",
+        "{ \"subnet6\": [ { "
+        "  \"subnet\": \"not an address/64\" } ]}",
+        "subnet configuration failed: "
+        "Failed to convert string to address 'notanaddress': Invalid argument"
+        },
+        {
+        "IP is Invalid",
+        "{ \"subnet6\": [ { "
+        "  \"subnet\": \"200175:db8::/64\" } ]}",
+        "subnet configuration failed: "
+        "Failed to convert string to address '200175:db8::': Invalid argument"
+        },
+        {
+        "Missing prefix",
+        "{ \"subnet6\": [ { "
+        "  \"subnet\": \"2001:db8::\" } ]}",
+        "subnet configuration failed: "
+        "Invalid subnet syntax (prefix/len expected):2001:db8:: (<string>:1:30)"
+        },
+        {
+        "Prefix not an integer (2 slashes)",
+        "{ \"subnet6\": [ { "
+        "  \"subnet\": \"2001:db8:://64\" } ]}",
+        "subnet configuration failed: "
+        "prefix length: '/64' is not an integer (<string>:1:30)"
+        },
+        {
+        "Prefix value is insane",
+        "{ \"subnet6\": [ { "
+        "  \"subnet\": \"2001:db8::/43225\" } ]}",
+        "subnet configuration failed: "
+        "Invalid prefix length specified for subnet: 43225 (<string>:1:30)"
+        }
+    };
+
+    // Iterate over the list of scenarios.  Each should fail to parse with
+    // a specific error message.
+    for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) {
+        {
+            SCOPED_TRACE((*scenario).description_);
+            ConstElementPtr config;
+            ASSERT_NO_THROW(config = parseDHCP6((*scenario).config_json_))
+                            << "invalid json, broken test";
+            ConstElementPtr status;
+            EXPECT_NO_THROW(status = configureDhcp6Server(srv_, config));
+            checkResult(status, 1);
+            EXPECT_EQ(comment_->stringValue(), (*scenario).exp_error_msg_);
+        }
+    }
+}
+
 // This test checks the configuration of the Rapid Commit option
 // support for the subnet.
 TEST_F(Dhcp6ParserTest, subnetRapidCommit) {

diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
index aec0ed264526ced11365178724d7a2409c9a6cc5..8a2b0ebdb2e6863576d4f5b1bbda7a61cda5a417 100644 (file)
--- a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
+++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
@@ -567,7 +567,26 @@ SubnetConfigParser::createSubnet(ConstElementPtr params) {
     // Try to create the address object. It also validates that
     // the address syntax is ok.
     isc::asiolink::IOAddress addr(subnet_txt.substr(0, pos));
-    uint8_t len = boost::lexical_cast<unsigned int>(subnet_txt.substr(pos + 1));
+
+    // Now parse out the prefix length.
+    unsigned int len;
+    try {
+        len = boost::lexical_cast<unsigned int>(subnet_txt.substr(pos + 1));
+    } catch (const boost::bad_lexical_cast) {
+        ConstElementPtr elem = params->get("subnet");
+        isc_throw(DhcpConfigError, "prefix length: '" <<
+                  subnet_txt.substr(pos+1) << "' is not an integer ("
+                  << elem->getPosition() << ")");
+    }
+
+    // Sanity check the prefix length
+    if ((addr.isV6() && len > 128) ||
+        (addr.isV4() && len > 32)) {
+        ConstElementPtr elem = params->get("subnet");
+        isc_throw(BadValue,
+                  "Invalid prefix length specified for subnet: " << len
+                  << " (" <<  elem->getPosition() << ")");
+    }
 
     // Call the subclass's method to instantiate the subnet
     initSubnet(params, addr, len);