From 67f3a9515b1195e5487ed3b8d33653a6f7aac8de Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 12 Aug 2019 15:46:36 -0400 Subject: [PATCH] [#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. --- ChangeLog | 13 ++--- src/bin/lfc/tests/lfc_controller_unittests.cc | 7 +-- src/lib/dhcpsrv/csv_lease_file4.cc | 34 +++++++------ src/lib/dhcpsrv/csv_lease_file4.h | 5 ++ .../dhcpsrv/tests/csv_lease_file4_unittest.cc | 49 +++++++++++++++---- .../tests/generic_lease_mgr_unittest.cc | 11 ++--- .../tests/lease_file_loader_unittest.cc | 6 +-- 7 files changed, 80 insertions(+), 45 deletions(-) diff --git a/ChangeLog b/ChangeLog index 35828f8576..ab74e2fe7b 100644 --- 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 b9566e111a..ccd6fc77da 100644 --- 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 3d4c707802..6649506579 100644 --- 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 37f2c3978d..950417c9e1 100644 --- 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 f85efc5f6b..ad65277a71 100644 --- 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 765a86948e..eceec558d6 100644 --- 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(), HTYPE_ETHER)); // Empty - lease->client_id_ = ClientIdPtr(); // Empty + lease->client_id_ = ClientIdPtr( + new ClientId(vector(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 96b1ba7f94..8e49695122 100644 --- 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); -- 2.47.2