BIND 10 trac2723, updated. 4317f74e10dbaa6a03a2bba9993fa2c79aa241bb [2723] ChangeLog updated.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Feb 27 11:14:45 UTC 2013
The branch, trac2723 has been updated
via 4317f74e10dbaa6a03a2bba9993fa2c79aa241bb (commit)
via b312e9ab66e413ee0708fc2db23d27e7f4b7b251 (commit)
from 28e4d9aaa305d597c533f43c8eaaae37567d6896 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 4317f74e10dbaa6a03a2bba9993fa2c79aa241bb
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Wed Feb 27 12:14:27 2013 +0100
[2723] ChangeLog updated.
commit b312e9ab66e413ee0708fc2db23d27e7f4b7b251
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Wed Feb 27 12:13:04 2013 +0100
[2723] The code now gracefully refuses too short DUIDs and client-ids
-----------------------------------------------------------------------
Summary of changes:
ChangeLog | 5 ++
src/lib/dhcp/duid.cc | 17 ++++-
src/lib/dhcp/duid.h | 12 ++++
src/lib/dhcp/tests/duid_unittest.cc | 79 ++++++++++++++++++---
src/lib/dhcpsrv/alloc_engine.cc | 18 ++++-
src/lib/dhcpsrv/tests/alloc_engine_unittest.cc | 64 ++++++++++++++++-
src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc | 28 +++++---
7 files changed, 203 insertions(+), 20 deletions(-)
-----------------------------------------------------------------------
diff --git a/ChangeLog b/ChangeLog
index fa50a5d..48ddecd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+5XX. [bug] tomek
+ All DHCP components now gracefully refuse to handle too short
+ DUIDs and client-id.
+ (Trac #2723, git TBD)
+
581. [func]* y-aharen
Added statistics items in b10-auth based on
http://bind10.isc.org/wiki/StatisticsItems. Qtype counters are
diff --git a/src/lib/dhcp/duid.cc b/src/lib/dhcp/duid.cc
index f1c8866..4f5113a 100644
--- a/src/lib/dhcp/duid.cc
+++ b/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
diff --git a/src/lib/dhcp/duid.h b/src/lib/dhcp/duid.h
index 688885b..efbc6e1 100644
--- a/src/lib/dhcp/duid.h
+++ b/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.
diff --git a/src/lib/dhcp/tests/duid_unittest.cc b/src/lib/dhcp/tests/duid_unittest.cc
index de20e51..aea15d1 100644
--- a/src/lib/dhcp/tests/duid_unittest.cc
+++ b/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};
diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index bcb7851..9c5bdeb 100644
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/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.
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
index ddc0f62..25224e6 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
+++ b/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) {
diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
index 1fc4f0b..9b8ec63 100644
--- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
+++ b/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]));
More information about the bind10-changes
mailing list