[2723] The code now gracefully refuses too short DUIDs and client-ids

src/lib/dhcp/duid.cc

@@ -28,15 +28,20 @@ namespace dhcp {
 DUID::DUID(const std::vector<uint8_t>& duid) {
     if (duid.size() > MAX_DUID_LEN) {
         isc_throw(OutOfRange, "DUID too large");
-    } else {
-        duid_ = duid;
     }
+    if (duid.empty()) {
+        isc_throw(OutOfRange, "Empty DUIDs are not allowed");
+    }
+    duid_ = duid;
 }
 
 DUID::DUID(const uint8_t* data, size_t len) {
     if (len > MAX_DUID_LEN) {
         isc_throw(OutOfRange, "DUID too large");
     }
+    if (len == 0) {
+        isc_throw(OutOfRange, "Empty DUIDs/Client-ids not allowed");
+    }
 
     duid_ = std::vector<uint8_t>(data, data + len);
 }
@@ -83,11 +88,19 @@ bool DUID::operator!=(const DUID& other) const {
 // Constructor based on vector<uint8_t>
 ClientId::ClientId(const std::vector<uint8_t>& clientid)
     : DUID(clientid) {
+    if (clientid.size() < MIN_CLIENT_ID_LEN) {
+        isc_throw(OutOfRange, "client-id is too short (" << clientid.size()
+                  << "), at least 2 is required");
+    }
 }
 
 // Constructor based on C-style data
 ClientId::ClientId(const uint8_t *clientid, size_t len)
     : DUID(clientid, len) {
+    if (len < MIN_CLIENT_ID_LEN) {
+        isc_throw(OutOfRange, "client-id is too short (" << len
+                  << "), at least 2 is required");
+    }
 }
 
 // Returns a copy of client-id data

src/lib/dhcp/duid.h

@@ -35,6 +35,11 @@ class DUID {
     /// As defined in RFC3315, section 9.1
     static const size_t MAX_DUID_LEN = 128;
 
+    /// @brief minimum duid size
+    /// There's no explicit minimal DUID size specified in RFC3315,
+    /// so let's use absolute minimum
+    static const size_t MIN_DUID_LEN = 1;
+
     /// @brief specifies DUID type
     typedef enum {
         DUID_UNKNOWN = 0, ///< invalid/unknown type
@@ -88,6 +93,13 @@ typedef boost::shared_ptr<DUID> DuidPtr;
 /// a client-id
 class ClientId : public DUID {
 public:
+
+    /// @brief Minimum size of a client ID
+    ///
+    /// Excerpt from RFC2132, section 9.14.
+    /// The code for this option is 61, and its minimum length is 2.
+    static const size_t MIN_CLIENT_ID_LEN = 2;
+
     /// @brief Maximum size of a client ID
     ///
     /// This is the same as the maximum size of the underlying DUID.

src/lib/dhcp/tests/duid_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2013  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -37,6 +37,15 @@ using boost::scoped_ptr;
 
 namespace {
 
+// This is a workaround for strange linking problems with gtest:
+// libdhcp___unittests-duid_unittest.o: In function `Compare<long unsigned int, long unsigned int>':
+// ~/gtest-1.6.0/include/gtest/gtest.h:1353: undefined reference to `isc::dhcp::ClientId::MAX_CLIENT_ID_LE'N
+// collect2: ld returned 1 exit status
+
+const size_t MAX_DUID_LEN = DUID::MAX_DUID_LEN;
+const size_t MAX_CLIENT_ID_LEN = DUID::MAX_DUID_LEN;
+
+
 // This test verifies if the constructors are working as expected
 // and process passed parameters.
 TEST(DuidTest, constructor) {
@@ -61,21 +70,20 @@ TEST(DuidTest, constructor) {
 // This test verifies if DUID size restrictions are implemented
 // properly.
 TEST(DuidTest, size) {
-    const int MAX_DUID_SIZE = 128;
-    uint8_t data[MAX_DUID_SIZE + 1];
+    uint8_t data[MAX_DUID_LEN + 1];
     vector<uint8_t> data2;
-    for (uint8_t i = 0; i < MAX_DUID_SIZE + 1; ++i) {
+    for (uint8_t i = 0; i < MAX_DUID_LEN + 1; ++i) {
         data[i] = i;
-        if (i < MAX_DUID_SIZE)
+        if (i < MAX_DUID_LEN)
             data2.push_back(i);
     }
-    ASSERT_EQ(data2.size(), MAX_DUID_SIZE);
+    ASSERT_EQ(data2.size(), MAX_DUID_LEN);
 
-    scoped_ptr<DUID> duidmaxsize1(new DUID(data, MAX_DUID_SIZE));
+    scoped_ptr<DUID> duidmaxsize1(new DUID(data, MAX_DUID_LEN));
     scoped_ptr<DUID> duidmaxsize2(new DUID(data2));
 
     EXPECT_THROW(
-        scoped_ptr<DUID> toolarge1(new DUID(data, MAX_DUID_SIZE + 1)),
+        scoped_ptr<DUID> toolarge1(new DUID(data, MAX_DUID_LEN + 1)),
         OutOfRange);
 
     // that's one too much
@@ -84,6 +92,16 @@ TEST(DuidTest, size) {
     EXPECT_THROW(
         scoped_ptr<DUID> toolarge2(new DUID(data2)),
         OutOfRange);
+
+    // empty duids are not allowed
+    vector<uint8_t> empty;
+    EXPECT_THROW(
+        scoped_ptr<DUID> emptyDuid(new DUID(empty)),
+        OutOfRange);
+
+    EXPECT_THROW(
+        scoped_ptr<DUID> emptyDuid2(new DUID(data, 0)),
+        OutOfRange);
 }
 
 // This test verifies if the implementation supports all defined
@@ -157,6 +175,51 @@ TEST(ClientIdTest, constructor) {
     EXPECT_TRUE(data2 == vecdata);
 }
 
+// Check that client-id sizes are reasonable
+TEST(ClientIdTest, size) {
+    uint8_t data[MAX_CLIENT_ID_LEN + 1];
+    vector<uint8_t> data2;
+    for (uint8_t i = 0; i < MAX_CLIENT_ID_LEN + 1; ++i) {
+        data[i] = i;
+        if (i < MAX_CLIENT_ID_LEN)
+            data2.push_back(i);
+    }
+    ASSERT_EQ(data2.size(), MAX_CLIENT_ID_LEN);
+
+    scoped_ptr<ClientId> duidmaxsize1(new ClientId(data, MAX_CLIENT_ID_LEN));
+    scoped_ptr<ClientId> duidmaxsize2(new ClientId(data2));
+
+    EXPECT_THROW(
+        scoped_ptr<ClientId> toolarge1(new ClientId(data, MAX_CLIENT_ID_LEN + 1)),
+        OutOfRange);
+
+    // that's one too much
+    data2.push_back(128);
+
+    EXPECT_THROW(
+        scoped_ptr<ClientId> toolarge2(new ClientId(data2)),
+        OutOfRange);
+
+    // empty client-ids are not allowed
+    vector<uint8_t> empty;
+    EXPECT_THROW(
+        scoped_ptr<ClientId> empty_client_id1(new ClientId(empty)),
+        OutOfRange);
+
+    EXPECT_THROW(
+        scoped_ptr<ClientId> empty_client_id2(new ClientId(data, 0)),
+        OutOfRange);
+
+    // client-id must be at least 2 bytes long
+    vector<uint8_t> shorty(1,17); // just a single byte with value 17
+    EXPECT_THROW(
+        scoped_ptr<ClientId> too_short_client_id1(new ClientId(shorty)),
+        OutOfRange);
+    EXPECT_THROW(
+        scoped_ptr<ClientId> too_short_client_id1(new ClientId(data, 1)),
+        OutOfRange);
+}
+
 // This test checks if the comparison operators are sane.
 TEST(ClientIdTest, operators) {
     uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};

src/lib/dhcpsrv/alloc_engine.cc

@@ -177,6 +177,14 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
             isc_throw(InvalidOperation, "No allocator selected");
         }
 
+        if (!subnet) {
+            isc_throw(InvalidOperation, "Subnet is required for allocation");
+        }
+
+        if (!duid) {
+            isc_throw(InvalidOperation, "DUID is mandatory for allocation");
+        }
+
         // check if there's existing lease for that subnet/duid/iaid combination.
         Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(*duid, iaid, subnet->getID());
         if (existing) {
@@ -284,8 +292,16 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
             isc_throw(InvalidOperation, "No allocator selected");
         }
 
+        if (!subnet) {
+            isc_throw(InvalidOperation, "Can't allocate IPv4 address without subnet");
+        }
+
+        if (!hwaddr) {
+            isc_throw(InvalidOperation, "HWAddr must be defined");
+        }
+
         // Check if there's existing lease for that subnet/clientid/hwaddr combination.
-        Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(hwaddr->hwaddr_, subnet->getID());
+        Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet->getID());
         if (existing) {
             // We have a lease already. This is a returning client, probably after
             // its reboot.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -160,7 +160,15 @@ public:
         EXPECT_EQ(subnet_->getT2(), lease->t2_);
         EXPECT_TRUE(false == lease->fqdn_fwd_);
         EXPECT_TRUE(false == lease->fqdn_rev_);
-        EXPECT_TRUE(*lease->client_id_ == *clientid_);
+        if (lease->client_id_ && !clientid_) {
+            ADD_FAILURE() << "Lease4 has a client-id, while it should have none.";
+        }
+        if (!lease->client_id_ && clientid_) {
+            ADD_FAILURE() << "Lease4 has no client-id, but it was expected to have one.";
+        }
+        if (lease->client_id_ && clientid_) {
+            EXPECT_TRUE(*lease->client_id_ == *clientid_);
+        }
         EXPECT_TRUE(lease->hwaddr_ == hwaddr_->hwaddr_);
         // @todo: check cltt
      }
@@ -329,6 +337,24 @@ TEST_F(AllocEngine6Test, allocBogusHint6) {
     detailCompareLease(lease, from_mgr);
 }
 
+// This test checks that NULL values are handled properly
+TEST_F(AllocEngine6Test, allocateAddress6Nulls) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    // Allocations without subnet are not allowed
+    Lease6Ptr lease = engine->allocateAddress6(Subnet6Ptr(), duid_, iaid_,
+                                               IOAddress("::"), false);
+    ASSERT_FALSE(lease);
+
+    // Allocations without DUID are not allowed either
+    lease = engine->allocateAddress6(subnet_, DuidPtr(), iaid_,
+                                     IOAddress("::"), false);
+    ASSERT_FALSE(lease);
+}
+
+
 // This test verifies that the allocator picks addresses that belong to the
 // pool
 TEST_F(AllocEngine6Test, IterativeAllocator) {
@@ -702,6 +728,42 @@ TEST_F(AllocEngine4Test, allocBogusHint4) {
 }
 
 
+// This test checks that NULL values are handled properly
+TEST_F(AllocEngine4Test, allocateAddress4Nulls) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    // Allocations without subnet are not allowed
+    Lease4Ptr lease = engine->allocateAddress4(SubnetPtr(), clientid_, hwaddr_,
+                                               IOAddress("0.0.0.0"), false);
+    EXPECT_FALSE(lease);
+
+    // Allocations without HW address are not allowed
+    lease = engine->allocateAddress4(subnet_, clientid_, HWAddrPtr(),
+                                     IOAddress("0.0.0.0"), false);
+    EXPECT_FALSE(lease);
+
+    // Allocations without client-id are allowed
+    clientid_ = ClientIdPtr();
+    lease = engine->allocateAddress4(subnet_, ClientIdPtr(), hwaddr_,
+                                     IOAddress("0.0.0.0"), false);
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+
+
 // This test verifies that the allocator picks addresses that belong to the
 // pool
 TEST_F(AllocEngine4Test, IterativeAllocator) {

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -317,8 +317,7 @@ public:
 
         } else if (address == straddress4_[7]) {
             lease->hwaddr_ = vector<uint8_t>();             // Empty
-            lease->client_id_ = ClientIdPtr(
-                new ClientId(vector<uint8_t>()));           // Empty
+            lease->client_id_ = ClientIdPtr();              // Empty
             lease->valid_lft_ = 7975;
             lease->cltt_ = 213876;
             lease->subnet_id_ = 19;
@@ -1048,9 +1047,10 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientId) {
     ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[3], *returned.begin());
 
-    // Check that an empty vector is valid
-    EXPECT_TRUE(leases[7]->client_id_->getClientId().empty());
-    returned = lmptr_->getLease4(leases[7]->hwaddr_);
+    // Check that client-id is NULL
+    EXPECT_FALSE(leases[7]->client_id_);
+    HWAddr tmp(leases[7]->hwaddr_, HTYPE_ETHER);
+    returned = lmptr_->getLease4(tmp);
     ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[7], *returned.begin());
 
@@ -1075,7 +1075,11 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientIdSize) {
     // ClientId::MAX_CLIENT_ID_LEN is used in an EXPECT_EQ.
     int client_id_max = ClientId::MAX_CLIENT_ID_LEN;
     EXPECT_EQ(128, client_id_max);
-    for (uint8_t i = 0; i <= client_id_max; i += 16) {
+
+    int client_id_min = ClientId::MIN_CLIENT_ID_LEN;
+    EXPECT_EQ(2, client_id_min); // See RFC2132, section 9.14
+
+    for (uint8_t i = client_id_min; i <= client_id_max; i += 16) {
         vector<uint8_t> clientid_vec(i, i);
         leases[1]->client_id_.reset(new ClientId(clientid_vec));
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
@@ -1184,7 +1188,11 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSize) {
     // For speed, go from 0 to 128 is steps of 16.
     int duid_max = DUID::MAX_DUID_LEN;
     EXPECT_EQ(128, duid_max);
-    for (uint8_t i = 0; i <= duid_max; i += 16) {
+
+    int duid_min = DUID::MIN_DUID_LEN;
+    EXPECT_EQ(1, duid_min);
+
+    for (uint8_t i = duid_min; i <= duid_max; i += 16) {
         vector<uint8_t> duid_vec(i, i);
         leases[1]->duid_.reset(new DUID(duid_vec));
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
@@ -1249,7 +1257,11 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSubnetIdSize) {
     // For speed, go from 0 to 128 is steps of 16.
     int duid_max = DUID::MAX_DUID_LEN;
     EXPECT_EQ(128, duid_max);
-    for (uint8_t i = 0; i <= duid_max; i += 16) {
+
+    int duid_min = DUID::MIN_DUID_LEN;
+    EXPECT_EQ(1, duid_min);
+
+    for (uint8_t i = duid_min; i <= duid_max; i += 16) {
         vector<uint8_t> duid_vec(i, i);
         leases[1]->duid_.reset(new DUID(duid_vec));
         EXPECT_TRUE(lmptr_->addLease(leases[1]));