Commit 67f3a951 authored by Thomas Markwalder's avatar Thomas Markwalder

[#805,!6-p] CSVLeaseFile4 ensures either hwaddr or client id for non-declined leases

Declined leases are expected to have neither hardware address nor client
id.  All others have must have at least one of them.

src/lib/dhcpsrv/csv_lease_file4.cc
    CSVLeaseFile4::append() - throws if a lease has no hardware address,
    no client id and is not in STATE_DECLINED

    CSVLeaseFile4::next(Lease4Ptr& lease) - discards rows if they have
    neither hardware addr nor client id and are not in STATE_DECLINED

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc
src/bin/lfc/tests/lfc_controller_unittests.cc
    Updated tests.
parent f551f62e
1654. [security] tmark
kea-dhcp4 Memfile logic now ensures during reading and writing
that leases which are not in the declined state, have either
a hardware address, client id, or both.
CVE:2019-6474
(Gitlab #805,!6-p, git #TBD)
1653. [security] tmark
Added a new parameter, "max-row-errors", to Memfile lease database
configuration for kea-dhcp4 and kea-dhcp6. This parameter can be
......@@ -188,12 +195,6 @@ Kea 1.6.0-beta2 released on July 24, 2019
Add support for associating subnets with the server tags in the
mysql_cb hooks library.
(Gitlab #717,!417, git e121ec4e0a04bc5bebdbfecf9cc1606b50e71263)
=======
16xx. [bug] tmark
Prevent DHCP servers from asserting when malformed hostname
or FQDN options are received.
(Gitlab #730,!2 git TBD)
>>>>>>> [#730,!2] Reworded ChangeLog entry
1618. [func] marcin
Add support for associating the shared networks with the server
......
// Copyright (C) 2015-2018 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2015-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
......@@ -411,8 +411,9 @@ TEST_F(LFCControllerTest, launch4) {
string b_3 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
"100,150,7,0,0,,1,\n";
// This one should be invalid, no hardware address and state is not declined
string c_1 = "192.0.2.3,,a:11:01:04,"
// This one should be invalid, no hardware address or client id
// and state is not declined
string c_1 = "192.0.2.3,,,"
"200,200,8,1,1,host.example.com,0,\n";
string d_1 = "192.0.2.5,16:17:18:19:1a:bc,,"
......
......@@ -36,22 +36,19 @@ CSVLeaseFile4::append(const Lease4& lease) {
CSVRow row(getColumnCount());
row.writeAt(getColumnIndex("address"), lease.addr_.toText());
std::string hwaddr_text;
if (!lease.hwaddr_) {
if (((!lease.hwaddr_) || lease.hwaddr_->hwaddr_.empty()) &&
((!lease.client_id_) || (lease.client_id_->getClientId().empty())) &&
(lease.state_ != Lease::STATE_DECLINED)) {
// Bump the error counter
++write_errs_;
isc_throw(BadValue, "Lease4 must have hardware address specified.");
} else {
hwaddr_text = lease.hwaddr_->toText(false);
if (hwaddr_text.empty() && lease.state_ != Lease::STATE_DECLINED) {
// Bump the error counter
++write_errs_;
isc_throw(BadValue, "Lease4 must have non empty hardware address.");
}
isc_throw(BadValue, "Lease4: " << lease.addr_.toText() << ", state: "
<< lease.state_ << " has neither hardware address or client id");
}
row.writeAt(getColumnIndex("hwaddr"), hwaddr_text);
row.writeAt(getColumnIndex("hwaddr"),
(!lease.hwaddr_ ? "" : lease.hwaddr_->toText(false)));
// Client id may be unset (NULL).
if (lease.client_id_) {
row.writeAt(getColumnIndex("client_id"), lease.client_id_->toText());
......@@ -99,6 +96,9 @@ CSVLeaseFile4::next(Lease4Ptr& lease) {
return (true);
}
// Get the lease address.
IOAddress addr(readAddress(row));
// Get client id. It is possible that the client id is empty and the
// returned pointer is NULL. This is ok, but if the client id is NULL,
// we need to be careful to not use the NULL pointer.
......@@ -113,15 +113,17 @@ CSVLeaseFile4::next(Lease4Ptr& lease) {
// that.
HWAddr hwaddr = readHWAddr(row);
uint32_t state = readState(row);
if (hwaddr.hwaddr_.empty() && state != Lease::STATE_DECLINED) {
isc_throw(isc::BadValue, "A blank hardware address is only"
" valid for declined leases");
if ((hwaddr.hwaddr_.empty()) && (client_id_vec.empty()) &&
(state != Lease::STATE_DECLINED)) {
isc_throw(BadValue, "Lease4: " << addr.toText() << ", state: "
<< state << "has neither hardware address or client id");
}
// Get the user context (can be NULL).
ConstElementPtr ctx = readContext(row);
lease.reset(new Lease4(readAddress(row),
lease.reset(new Lease4(addr,
HWAddrPtr(new HWAddr(hwaddr)),
client_id_vec.empty() ? NULL : &client_id_vec[0],
client_id_len,
......
......@@ -58,6 +58,8 @@ public:
/// error.
///
/// @param lease Structure representing a DHCPv4 lease.
/// @throw BadValue if the lease has no hardware address, no client id and
/// is not in STATE_DECLINED.
void append(const Lease4& lease);
/// @brief Reads next lease from the CSV file.
......@@ -66,6 +68,9 @@ public:
/// message using @c CSVFile::setReadMsg and returns false. The error
/// string may be read using @c CSVFile::getReadMsg.
///
/// Treats rows that no hardware address, no client id and state is not
/// STATE_DECLINED as an error.
///
/// This function is exception safe.
///
/// @param [out] lease Pointer to the lease read from CSV file or
......
......@@ -105,9 +105,11 @@ CSVLeaseFile4Test::writeSampleFile() const {
"fqdn_fwd,fqdn_rev,hostname,state,user_context\n"
"192.0.2.1,06:07:08:09:0a:bc,,200,200,8,1,1,"
"host.example.com,0,\n"
"192.0.2.1,,a:11:01:04,200,200,8,1,1,host.example.com,0,\n"
"192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,100,7,"
"0,0,,1,{ \"foobar\": true }\n");
"192.0.2.2,,,200,200,8,1,1,host.example.com,0,\n"
"192.0.2.3,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,100,7,"
"0,0,,1,{ \"foobar\": true }\n"
"192.0.2.4,,11:22:33:44:55:66,200,200,8,1,1,host.example.com,0,\n"
"192.0.2.5,,,200,200,8,1,1,,1,\n");
}
// This test checks the capability to read and parse leases from the file.
......@@ -148,11 +150,12 @@ TEST_F(CSVLeaseFile4Test, parse) {
EXPECT_FALSE(lease->getContext());
}
// Second lease is malformed - HW address is empty when state
// Second lease is malformed - has no HW address or client id and state
// is not declined.
{
SCOPED_TRACE("Second lease malformed");
EXPECT_FALSE(lf.next(lease));
EXPECT_FALSE(lease);
checkStats(lf, 2, 1, 1, 0, 0, 0);
}
......@@ -165,7 +168,7 @@ TEST_F(CSVLeaseFile4Test, parse) {
checkStats(lf, 3, 2, 1, 0, 0, 0);
// Verify that the third lease is correct.
EXPECT_EQ("192.0.3.15", lease->addr_.toText());
EXPECT_EQ("192.0.2.3", lease->addr_.toText());
HWAddr hwaddr3(*lease->hwaddr_);
EXPECT_EQ("dd:de:ba:0d:1b:2e:3e:4f", hwaddr3.toText(false));
ASSERT_TRUE(lease->client_id_);
......@@ -181,21 +184,49 @@ TEST_F(CSVLeaseFile4Test, parse) {
EXPECT_EQ("{ \"foobar\": true }", lease->getContext()->str());
}
// Fourth lease has no hardware address but has client id
{
SCOPED_TRACE("Fourth lease valid");
EXPECT_TRUE(lf.next(lease));
ASSERT_TRUE(lease);
checkStats(lf, 4, 3, 1, 0, 0, 0);
EXPECT_EQ("192.0.2.4", lease->addr_.toText());
ASSERT_TRUE(lease->hwaddr_);
EXPECT_TRUE(lease->hwaddr_->hwaddr_.empty());
ASSERT_TRUE(lease->client_id_);
EXPECT_EQ("11:22:33:44:55:66", lease->client_id_->toText());
}
// Fifth lease has no hardware address or client id but is declined
{
SCOPED_TRACE("Fifth lease valid");
EXPECT_TRUE(lf.next(lease));
ASSERT_TRUE(lease);
checkStats(lf, 5, 4, 1, 0, 0, 0);
EXPECT_EQ("192.0.2.5", lease->addr_.toText());
ASSERT_TRUE(lease->hwaddr_);
EXPECT_TRUE(lease->hwaddr_->hwaddr_.empty());
ASSERT_FALSE(lease->client_id_);
EXPECT_EQ(lease->state_, Lease::STATE_DECLINED);
}
// There are no more leases. Reading should cause no error, but the returned
// lease pointer should be NULL.
{
SCOPED_TRACE("Fifth read empty");
SCOPED_TRACE("Sixth read empty");
EXPECT_TRUE(lf.next(lease));
EXPECT_FALSE(lease);
checkStats(lf, 4, 2, 1, 0, 0, 0);
checkStats(lf, 6, 4, 1, 0, 0, 0);
}
// We should be able to do it again.
{
SCOPED_TRACE("Sixth read empty");
SCOPED_TRACE("Seventh read empty");
EXPECT_TRUE(lf.next(lease));
EXPECT_FALSE(lease);
checkStats(lf, 5, 2, 1, 0, 0, 0);
checkStats(lf, 7, 4, 1, 0, 0, 0);
}
}
......
......@@ -175,7 +175,8 @@ GenericLeaseMgrTest::initializeLease4(std::string address) {
} else if (address == straddress4_[7]) {
lease->hwaddr_.reset(new HWAddr(vector<uint8_t>(), HTYPE_ETHER)); // Empty
lease->client_id_ = ClientIdPtr(); // Empty
lease->client_id_ = ClientIdPtr(
new ClientId(vector<uint8_t>(8, 0x77)));
lease->valid_lft_ = 7975;
lease->cltt_ = 213876;
lease->subnet_id_ = 19;
......@@ -566,6 +567,7 @@ GenericLeaseMgrTest::testGetLease4HWAddr2() {
// Check that an empty vector is valid
EXPECT_TRUE(leases[7]->hwaddr_->hwaddr_.empty());
EXPECT_FALSE(leases[7]->client_id_->getClientId().empty());
returned = lmptr_->getLease4(*leases[7]->hwaddr_);
ASSERT_EQ(1, returned.size());
detailCompareLease(leases[7], *returned.begin());
......@@ -1159,13 +1161,6 @@ GenericLeaseMgrTest::testGetLease4ClientId2() {
ASSERT_EQ(1, returned.size());
detailCompareLease(leases[3], *returned.begin());
// Check that client-id is NULL
EXPECT_FALSE(leases[7]->client_id_);
HWAddr tmp(*leases[7]->hwaddr_);
returned = lmptr_->getLease4(tmp);
ASSERT_EQ(1, returned.size());
detailCompareLease(leases[7], *returned.begin());
// Try to get something with invalid client ID
const uint8_t invalid_data[] = {0, 0, 0};
ClientId invalid(invalid_data, sizeof(invalid_data));
......
......@@ -347,12 +347,12 @@ TEST_F(LeaseFileLoaderTest, loadWrite4) {
std::string b_2 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
"100,135,7,0,0,,1,\n";
std::string c_1 = "192.0.2.3,,a:11:01:04,"
std::string c_1 = "192.0.2.3,,,"
"200,200,8,1,1,host.example.com,0,\n";
// Create lease file with leases for 192.0.2.1, 192.0.3.15. The lease
// entry for the 192.0.2.3 is invalid (lacks HW address) and should
// be discarded.
// entry for the 192.0.2.3 is invalid (lacks HW address and client id)
// and should be discarded.
test_str = v4_hdr_ + a_1 + b_1 + c_1 + b_2 + a_2;
io_.writeFile(test_str);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment