[5646] Dedicated checks for truncated client-id, server-id added

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index f5ef0d8af596bfdb59b2a838c6ee8cb99f435dfa..5fd0b1b71a7a4123559ef02817ceb3e3a4b1f5cf 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -1260,18 +1260,23 @@ Dhcpv6Srv::sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
                        RequirementLevel serverid) {
     OptionCollection client_ids = pkt->getOptions(D6O_CLIENTID);
     switch (clientid) {
-    case MANDATORY:
+    case MANDATORY: {
         if (client_ids.size() != 1) {
             isc_throw(RFCViolation, "Exactly 1 client-id option expected in "
                       << pkt->getName() << ", but " << client_ids.size()
                       << " received");
         }
+        sanityCheckDUID(client_ids.begin()->second, "client-id");
         break;
+    }
     case OPTIONAL:
         if (client_ids.size() > 1) {
             isc_throw(RFCViolation, "Too many (" << client_ids.size()
                       << ") client-id options received in " << pkt->getName());
         }
+        if (!client_ids.empty()) {
+            sanityCheckDUID(client_ids.begin()->second, "client-id");
+        }
         break;
 
     case FORBIDDEN:
@@ -1294,6 +1299,7 @@ Dhcpv6Srv::sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
                       << server_ids.size() << "), exactly 1 expected in message "
                       << pkt->getName());
         }
+        sanityCheckDUID(server_ids.begin()->second, "server-id");
         break;
 
     case OPTIONAL:
@@ -1301,6 +1307,23 @@ Dhcpv6Srv::sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
             isc_throw(RFCViolation, "Too many (" << server_ids.size()
                       << ") server-id options received in " << pkt->getName());
         }
+        if (!server_ids.empty()) {
+            sanityCheckDUID(server_ids.begin()->second, "server-id");
+        }
+    }
+}
+
+void Dhcpv6Srv::sanityCheckDUID(const OptionPtr& opt, const std::string& opt_name) {
+    if (!opt) {
+        isc_throw(RFCViolation, "Unable to find expected option " << opt_name);
+    }
+
+    // The client-id or server-id has to have at least 3 bytes of useful data:
+    // two for duid type and one more for actual duid value.
+    uint16_t len = opt->len() - opt->getHeaderLen();
+    if (len < 3) {
+        isc_throw(RFCViolation, "Received empty or truncated " << opt_name << " option: "
+                  << len << " byte(s) only");
     }
 }
 

diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h
index 6debce96ac7e5a9f56335f8e5d5ef2995a1b06ad..c8a1ec93d8334247bd2b4cfdbfd3ffc1f8de210a 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.h
+++ b/src/bin/dhcp6/dhcp6_srv.h
@@ -225,6 +225,13 @@ protected:
     void sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
                      RequirementLevel serverid);
 
+    /// @brief verifies if received DUID option (client-id or server-id) is sane
+    ///
+    /// @param opt option to be checked
+    /// @param opt_name text name to be printed
+    /// @throw RFCViolation if any issues are detected
+    void sanityCheckDUID(const OptionPtr& opt, const std::string& opt_name);
+
     /// @brief Processes incoming Solicit and returns response.
     ///
     /// Processes received Solicit message and verifies that its sender

diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
index 393d1bd4d7648116e6218cce407ffabb0b2b4a85..8a8d1708b07b9c89b3960a720d1788211c8b0d1a 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
@@ -1167,6 +1167,56 @@ TEST_F(Dhcpv6SrvTest, sanityCheck) {
     EXPECT_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY),
                  RFCViolation);
 }
+
+// This test verifies if the sanityCheck() really checks options content,
+// especially truncated client-id.
+TEST_F(Dhcpv6SrvTest, truncatedClientId) {
+    NakedDhcpv6Srv srv(0);
+
+    Pkt6Ptr pkt = Pkt6Ptr(new Pkt6(DHCPV6_SOLICIT, 1234));
+
+    // Case 1: completely empty (size 0)
+    pkt->addOption(generateClientId(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));
+    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));
+    EXPECT_NO_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN));
+}
+
+// This test verifies if the sanityCheck() really checks options content,
+// especially truncated server-id.
+TEST_F(Dhcpv6SrvTest, truncatedServerId) {
+    NakedDhcpv6Srv srv(0);
+
+    Pkt6Ptr pkt = Pkt6Ptr(new Pkt6(DHCPV6_SOLICIT, 1234));
+
+    // Case 1: completely empty (size 0)
+    pkt->addOption(generateServerId(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));
+    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));
+    EXPECT_NO_THROW(srv.sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::MANDATORY));
+}
+
+
 // Check that the server is testing if server identifier received in the
 // query, matches server identifier used by the server.
 TEST_F(Dhcpv6SrvTest, testServerID) {

diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h
index da51036bb88f13b58eb1d52d97487a4aaf4804c6..71a23d8db26626bd7af43dce539da281386b0d70 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_test_utils.h
+++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h
@@ -312,6 +312,12 @@ public:
     // Generate client-id option
     isc::dhcp::OptionPtr generateClientId(size_t duid_size = 32) {
 
+        if (duid_size == 0) {
+            return (isc::dhcp::OptionPtr
+                    (new isc::dhcp::Option(isc::dhcp::Option::V6, D6O_CLIENTID)));
+
+        }
+
         isc::dhcp::OptionBuffer clnt_duid(duid_size);
         for (size_t i = 0; i < duid_size; i++) {
             clnt_duid[i] = 100 + i;
@@ -325,6 +331,29 @@ public:
                                        clnt_duid.begin() + duid_size)));
     }
 
+    /// 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)));
+    }
+
     // Checks if server response (ADVERTISE or REPLY) includes proper
     // server-id.
     void checkServerId(const isc::dhcp::Pkt6Ptr& rsp,