[#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.

diff --git a/ChangeLog b/ChangeLog
index 35828f8576dcb2c95808664a838e144a96d87734..ab74e2fe7b3ac4acf752e8cedc322f53a8979511 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+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

diff --git a/src/bin/lfc/tests/lfc_controller_unittests.cc b/src/bin/lfc/tests/lfc_controller_unittests.cc
index b9566e111a6e18baef107a6e2bd43459ae769554..ccd6fc77da8ad5193d719629acb4fbd9a4392d9b 100644 (file)
--- a/src/bin/lfc/tests/lfc_controller_unittests.cc
+++ b/src/bin/lfc/tests/lfc_controller_unittests.cc
@@ -1,4 +1,4 @@
-// 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,,"

diff --git a/src/lib/dhcpsrv/csv_lease_file4.cc b/src/lib/dhcpsrv/csv_lease_file4.cc
index 3d4c707802810a6f400561f86ed811a0d8da3839..6649506579e81faccce343b1398f33d2c9e915d5 100644 (file)
--- a/src/lib/dhcpsrv/csv_lease_file4.cc
+++ b/src/lib/dhcpsrv/csv_lease_file4.cc
@@ -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,

diff --git a/src/lib/dhcpsrv/csv_lease_file4.h b/src/lib/dhcpsrv/csv_lease_file4.h
index 37f2c3978d4b974b83d6c13db070946e5aaf573d..950417c9e1e3a1d6e00eae2dc7cbb3e0cd626115 100644 (file)
--- a/src/lib/dhcpsrv/csv_lease_file4.h
+++ b/src/lib/dhcpsrv/csv_lease_file4.h
@@ -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

diff --git a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
index f85efc5f6bea886492ca38d8f98dc47763fca647..ad65277a7185903734f2aba8092455c243ef06da 100644 (file)
--- a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
@@ -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);
     }
 }
 

diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
index 765a86948e6b8d971be3175dda6105aa1effb64f..eceec558d673477819ccf2d3a46500e800e39875 100644 (file)
--- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
@@ -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));

diff --git a/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc b/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc
index 96b1ba7f949ffbe3692cff86b99ca5ef3fda3949..8e49695122dc22e5b35ebd76bca5796b301d99a5 100644 (file)
--- a/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc
+++ b/src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc
@@ -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);