From: Thomas Markwalder Date: Wed, 3 Jul 2019 18:24:04 +0000 (-0400) Subject: [#722] kea-dhcp6 sanity checking of inbound DUID options improved X-Git-Tag: Kea-1.6.0~41^2~31 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=20b6386b52b7f044324bfc2bde02edd5afb6eb65;p=thirdparty%2Fkea.git [#722] kea-dhcp6 sanity checking of inbound DUID options improved src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::sanityCheckDUID() - modified to attempt to construct a DUID instance as final sanity check src/bin/dhcp6/tests/dhcp6_srv_unittest.cc TEST_F(Dhcpv6SrvTest, sanityCheckClientId) TEST_F(Dhcpv6SrvTest, sanityCheckServerId) - revamped tests to check against max and max + 1 src/bin/dhcp6/tests/dhcp6_test_utils.h A little refactoring to ease option creation src/lib/dhcp/duid.cc DUID::DUID(const std::vector& duid) DUID::DUID(const uint8_t* data, size_t len) - updated error log to show actual sizes --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index c07ab6f311..64ac1dbc8f 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1415,6 +1415,14 @@ void Dhcpv6Srv::sanityCheckDUID(const OptionPtr& opt, const std::string& opt_nam isc_throw(RFCViolation, "Received empty or truncated " << opt_name << " option: " << len << " byte(s) only"); } + + // We need to make sure we can construct one, if not we're toast later on. + try { + DuidPtr tmp(new DUID(opt->getData())); + } catch (const std::exception& ex) { + isc_throw(RFCViolation, "Received invalid content for " + << opt_name << ", " << ex.what()); + } } Subnet6Ptr diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 97a124dafc..c01bf663c5 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -1476,54 +1476,75 @@ TEST_F(Dhcpv6SrvTest, sanityCheck) { RFCViolation); } -// This test verifies if the sanityCheck() really checks options content, -// especially truncated client-id. -TEST_F(Dhcpv6SrvTest, truncatedClientId) { +// This test verifies that sanity checking against valid and invalid +// client ids +TEST_F(Dhcpv6SrvTest, sanityCheckClientId) { NakedDhcpv6Srv srv(0); Pkt6Ptr pkt = Pkt6Ptr(new Pkt6(DHCPV6_SOLICIT, 1234)); // Case 1: completely empty (size 0) - pkt->addOption(generateClientId(0)); + pkt->addOption(generateBinaryOption(D6O_CLIENTID, 0)); EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN), RFCViolation); // Case 2: too short (at the very least 3 bytes are needed) pkt->delOption(D6O_CLIENTID); - pkt->addOption(generateClientId(2)); + pkt->addOption(generateBinaryOption(D6O_CLIENTID, 2)); EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN), RFCViolation); // Case 3: the shortest DUID possible (3 bytes) is ok: pkt->delOption(D6O_CLIENTID); - pkt->addOption(generateClientId(3)); + pkt->addOption(generateBinaryOption(D6O_CLIENTID, 3)); EXPECT_NO_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN)); + + // Case 4: longest possible is 128, should be ok + pkt->delOption(D6O_CLIENTID); + pkt->addOption(generateBinaryOption(D6O_CLIENTID, DUID::MAX_DUID_LEN)); + EXPECT_NO_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN)); + + // Case 5: too long + pkt->delOption(D6O_CLIENTID); + pkt->addOption(generateBinaryOption(D6O_CLIENTID, DUID::MAX_DUID_LEN + 1)); + EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN), + RFCViolation); } -// This test verifies if the sanityCheck() really checks options content, -// especially truncated server-id. -TEST_F(Dhcpv6SrvTest, truncatedServerId) { +// This test verifies that sanity checking against valid and invalid +// server ids +TEST_F(Dhcpv6SrvTest, sanityCheckServerId) { NakedDhcpv6Srv srv(0); Pkt6Ptr pkt = Pkt6Ptr(new Pkt6(DHCPV6_SOLICIT, 1234)); // Case 1: completely empty (size 0) - pkt->addOption(generateServerId(0)); + pkt->addOption(generateBinaryOption(D6O_SERVERID, 0)); EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::MANDATORY), RFCViolation); // Case 2: too short (at the very least 3 bytes are needed) pkt->delOption(D6O_SERVERID); - pkt->addOption(generateServerId(2)); + pkt->addOption(generateBinaryOption(D6O_SERVERID, 2)); EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::MANDATORY), RFCViolation); // Case 3: the shortest DUID possible (3 bytes) is ok: pkt->delOption(D6O_SERVERID); - pkt->addOption(generateServerId(3)); + pkt->addOption(generateBinaryOption(D6O_SERVERID, 3)); EXPECT_NO_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::MANDATORY)); -} + // Case 4: longest possible is 128, should be ok + pkt->delOption(D6O_SERVERID); + pkt->addOption(generateBinaryOption(D6O_SERVERID, DUID::MAX_DUID_LEN)); + EXPECT_NO_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::MANDATORY)); + + // Case 5: too long + pkt->delOption(D6O_SERVERID); + pkt->addOption(generateBinaryOption(D6O_SERVERID, DUID::MAX_DUID_LEN + 1)); + EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::MANDATORY), + RFCViolation); +} // Check that the server is testing if server identifier received in the // query, matches server identifier used by the server. diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h index 471a2a97de..8ae6b80029 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.h +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h @@ -310,49 +310,47 @@ public: D6O_INTERFACE_ID, tmp))); } - // Generate client-id option - isc::dhcp::OptionPtr generateClientId(size_t duid_size = 32) { - - if (duid_size == 0) { + /// @brief Generate binary data option + /// + /// Creates an Option in the V6 space with the given type and binary data + /// of the given number of bytes. The data is initialized to the values + /// 100 to 100 + n, where n is the desired number of bytes. + /// + /// @param type option type for the new option + /// @param data_size number of bytes of data to generate + /// + /// @return Pointer to the new option + isc::dhcp::OptionPtr generateBinaryOption(const DHCPv6OptionType type, size_t data_size) { + if (data_size == 0) { return (isc::dhcp::OptionPtr - (new isc::dhcp::Option(isc::dhcp::Option::V6, D6O_CLIENTID))); + (new isc::dhcp::Option(isc::dhcp::Option::V6, type))); } - isc::dhcp::OptionBuffer clnt_duid(duid_size); - for (size_t i = 0; i < duid_size; i++) { - clnt_duid[i] = 100 + i; + isc::dhcp::OptionBuffer data_data(data_size); + for (size_t i = 0; i < data_size; i++) { + data_data[i] = 100 + i; } - duid_ = isc::dhcp::DuidPtr(new isc::dhcp::DUID(clnt_duid)); - return (isc::dhcp::OptionPtr - (new isc::dhcp::Option(isc::dhcp::Option::V6, D6O_CLIENTID, - clnt_duid.begin(), - clnt_duid.begin() + duid_size))); + (new isc::dhcp::Option(isc::dhcp::Option::V6, type, + data_data.begin(), + data_data.begin() + data_size))); + } + + // Generate client-id option + isc::dhcp::OptionPtr generateClientId(size_t duid_size = 32) { + isc::dhcp::OptionPtr opt = generateBinaryOption(D6O_CLIENTID, duid_size); + duid_ = isc::dhcp::DuidPtr(new isc::dhcp::DUID(opt->getData())); + return (opt); } /// Generate server-id option /// @param duid_size size of the duid isc::dhcp::OptionPtr generateServerId(size_t duid_size = 32) { - - if (duid_size == 0) { - return (isc::dhcp::OptionPtr - (new isc::dhcp::Option(isc::dhcp::Option::V6, D6O_SERVERID))); - - } - - isc::dhcp::OptionBuffer clnt_duid(duid_size); - for (size_t i = 0; i < duid_size; i++) { - clnt_duid[i] = 100 + i; - } - - duid_ = isc::dhcp::DuidPtr(new isc::dhcp::DUID(clnt_duid)); - - return (isc::dhcp::OptionPtr - (new isc::dhcp::Option(isc::dhcp::Option::V6, D6O_SERVERID, - clnt_duid.begin(), - clnt_duid.begin() + duid_size))); + isc::dhcp::OptionPtr opt = generateBinaryOption(D6O_SERVERID, duid_size); + duid_ = isc::dhcp::DuidPtr(new isc::dhcp::DUID(opt->getData())); + return (opt); } // Checks if server response (ADVERTISE or REPLY) includes proper diff --git a/src/lib/dhcp/duid.cc b/src/lib/dhcp/duid.cc index 373c26c0c0..bd12e0ec95 100644 --- a/src/lib/dhcp/duid.cc +++ b/src/lib/dhcp/duid.cc @@ -22,7 +22,8 @@ namespace dhcp { DUID::DUID(const std::vector& duid) { if (duid.size() > MAX_DUID_LEN) { - isc_throw(isc::BadValue, "DUID too large"); + isc_throw(isc::BadValue, "DUID is " << duid.size() + << ", exceeds max of " << MAX_DUID_LEN); } if (duid.empty()) { isc_throw(isc::BadValue, "Empty DUIDs are not allowed"); @@ -32,7 +33,8 @@ DUID::DUID(const std::vector& duid) { DUID::DUID(const uint8_t* data, size_t len) { if (len > MAX_DUID_LEN) { - isc_throw(isc::BadValue, "DUID too large"); + isc_throw(isc::BadValue, "DUID is " << len + << ", exceeds max of " << MAX_DUID_LEN); } if (len == 0) { isc_throw(isc::BadValue, "Empty DUIDs/Client-ids not allowed");