From: Thomas Markwalder Date: Tue, 29 May 2018 20:00:06 +0000 (-0400) Subject: [5418] Improved error handling of invalid subnet prefix lengths X-Git-Tag: trac5631_base~5^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5ba16fabc6e81e0f9d7755cceb5189c7140c9864;p=thirdparty%2Fkea.git [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 e1e291e815..362604bc31 100644 --- 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 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 (: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 (: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 (: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, "")); + // 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 51df3143fd..faa6b0961c 100644 --- 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, "")); } +// 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 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:: (:1:30)" + }, + { + "Prefix not an integer (2 slashes)", + "{ \"subnet6\": [ { " + " \"subnet\": \"2001:db8:://64\" } ]}", + "subnet configuration failed: " + "prefix length: '/64' is not an integer (:1:30)" + }, + { + "Prefix value is insane", + "{ \"subnet6\": [ { " + " \"subnet\": \"2001:db8::/43225\" } ]}", + "subnet configuration failed: " + "Invalid prefix length specified for subnet: 43225 (: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 aec0ed2645..8a2b0ebdb2 100644 --- 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(subnet_txt.substr(pos + 1)); + + // Now parse out the prefix length. + unsigned int len; + try { + len = boost::lexical_cast(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);