[#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<uint8_t>& 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 c07ab6f311d97fb3eb054bd868e2ee4b1aaa0167..64ac1dbc8f93154add4e54f95478f4969ed3fc1d 100644 (file)
--- 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 97a124dafc1cbfcd643d18eb638617c798442c62..c01bf663c57d945d88c1fe08eb625352d5b47fbb 100644 (file)
--- 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 471a2a97de7231a5732d4186539052a5f98e63ce..8ae6b80029a40775092f8792d5e88662801e8fca 100644 (file)
--- 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 373c26c0c0782fd42d2aed48dd7f71c80ca2f656..bd12e0ec9584e17b57b3b4ff124d37074e8171bf 100644 (file)
--- a/src/lib/dhcp/duid.cc
+++ b/src/lib/dhcp/duid.cc
@@ -22,7 +22,8 @@ namespace dhcp {
 
 DUID::DUID(const std::vector<uint8_t>& 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<uint8_t>& 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");