[#722,!3] Applies the fix for #722 to 1.5.0

kea-dhcp6 sanity checking now catches over-sized DUIDs

modified:
    src/bin/dhcp6/dhcp6_srv.cc
    src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
    src/bin/dhcp6/tests/dhcp6_test_utils.h
    src/lib/dhcp/duid.cc
    src/lib/dhcp/tests/duid_unittest.cc

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 440b26d6201eba79dba40dae21cdae3d9e4b3339..fd1d7fa92d91ec3529c1b3d710e0170bc7bd92cd 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -1357,6 +1357,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 2a2f505fb68a56d9e20c5f03d69e210780d9a4cd..f479fba21fe06bb863e45ce8caee2058e8d7996a 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
@@ -1168,54 +1168,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 71a23d8db26626bd7af43dce539da281386b0d70..4b13266983591602cfa20b67c7c3fbb7f0bf9640 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_test_utils.h
+++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h
@@ -309,49 +309,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..011325785d273d77ad018ba2b7f95008f5edf520 100644 (file)
--- a/src/lib/dhcp/duid.cc
+++ b/src/lib/dhcp/duid.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -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 size is " << duid.size()
+                  << " bytes, exceeds maximum 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 size is " << len
+                  << " bytes, exceeds maximum of " << MAX_DUID_LEN);
     }
     if (len == 0) {
         isc_throw(isc::BadValue, "Empty DUIDs/Client-ids not allowed");
@@ -44,7 +46,6 @@ DUID::DUID(const uint8_t* data, size_t len) {
 const std::vector<uint8_t>& DUID::getDuid() const {
     return (duid_);
 }
-
 DUID::DUIDType DUID::getType() const {
     if (duid_.size() < 2) {
         return (DUID_UNKNOWN);

diff --git a/src/lib/dhcp/tests/duid_unittest.cc b/src/lib/dhcp/tests/duid_unittest.cc
index 108bbe4d1e71df3e87e8dbc4820f6fdfbfa3a9cd..d9bb7e1014c9cdb81040f795ab07930bd732b533 100644 (file)
--- a/src/lib/dhcp/tests/duid_unittest.cc
+++ b/src/lib/dhcp/tests/duid_unittest.cc
@@ -62,6 +62,10 @@ TEST(DuidTest, constructor) {
 // This test verifies if DUID size restrictions are implemented
 // properly.
 TEST(DuidTest, size) {
+
+    // Ensure that our size constant is RFC-compliant.
+    ASSERT_EQ(128, MAX_DUID_LEN);
+
     uint8_t data[MAX_DUID_LEN + 1];
     vector<uint8_t> data2;
     for (uint8_t i = 0; i < MAX_DUID_LEN + 1; ++i) {
@@ -220,6 +224,9 @@ TEST(ClientIdTest, constructor) {
 
 // Check that client-id sizes are reasonable
 TEST(ClientIdTest, size) {
+    // Ensure that our size constant is RFC-compliant.
+    ASSERT_EQ(128, MAX_CLIENT_ID_LEN);
+
     uint8_t data[MAX_CLIENT_ID_LEN + 1];
     vector<uint8_t> data2;
     for (uint8_t i = 0; i < MAX_CLIENT_ID_LEN + 1; ++i) {