From e6eced975fe659bf835d0faf1a2558ff21e26d9d Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 11 Dec 2018 15:22:16 +0100 Subject: [PATCH 1/2] [#337,!167] Fixed errors for high lease expiration times in lease_cmds. --- src/hooks/dhcp/lease_cmds/lease_parser.cc | 16 ++- .../lease_cmds/tests/lease_cmds_unittest.cc | 128 +++++++++++++----- 2 files changed, 109 insertions(+), 35 deletions(-) diff --git a/src/hooks/dhcp/lease_cmds/lease_parser.cc b/src/hooks/dhcp/lease_cmds/lease_parser.cc index db4143d589..e7dd3cbdee 100644 --- a/src/hooks/dhcp/lease_cmds/lease_parser.cc +++ b/src/hooks/dhcp/lease_cmds/lease_parser.cc @@ -95,8 +95,12 @@ Lease4Parser::parse(ConstSrvConfigPtr& cfg, /// any leases. time_t cltt; if (lease_info->contains("expire")) { - int64_t tmp = getUint32(lease_info, "expire"); - cltt = static_cast(tmp - valid_lft); + int64_t expire_time = getInteger(lease_info, "expire"); + if (expire_time <= 0) { + isc_throw(BadValue , "expiration time must be positive for address " + << addr); + } + cltt = static_cast(expire_time - valid_lft); } else { cltt = time(NULL); } @@ -282,8 +286,12 @@ Lease6Parser::parse(ConstSrvConfigPtr& cfg, /// discarding any leases. time_t cltt; if (lease_info->contains("expire")) { - int64_t tmp = getUint32(lease_info, "expire"); - cltt = static_cast(tmp - valid_lft); + int64_t expire_time = getInteger(lease_info, "expire"); + if (expire_time <= 0) { + isc_throw(BadValue , "expiration time must be positive for address " + << addr); + } + cltt = static_cast(expire_time - valid_lft); } else { cltt = time(NULL); } diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc index 0e297a5ea1..3fbb1e22e2 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc @@ -30,6 +30,12 @@ using namespace isc::test; namespace { +/// @brief High valid lifetime used for leases in the tests below. +constexpr uint32_t HIGH_VALID_LIFETIME = 0xFFFFFFFE; + +/// @brief December 11th 2030 date used in the unit tests for cltt. +constexpr time_t DEC_2030_TIME = 1923222072; + /// @brief Test fixture for testing loading and unloading the flex-id library class LibLoadTest : public ::testing::Test { public: @@ -291,7 +297,7 @@ public: /// @brief Creates an IPv4 lease /// - /// Lease parameters: valid lifetime = 3600, cltt = 12345678, fqdn-fwd = false, + /// Lease parameters: valid lifetime = 0xFFFFFFFE, cltt = 1923222072, fqdn-fwd = false, /// fqdn-rev = true, hostname = myhost.example.com /// /// @param ip_address IP address for the lease. @@ -315,8 +321,10 @@ public: // Set other parameters. For historical reasons, address 0 is not used. lease->hwaddr_.reset(new HWAddr(vector(6, hw_address_pattern), HTYPE_ETHER)); lease->client_id_ = ClientIdPtr(new ClientId(vector(8, client_id_pattern))); - lease->valid_lft_ = 3600; - lease->cltt_ = 12345678; + // Purposely using high cltt and valid lifetime to test that + // expiration time is cast properly. + lease->valid_lft_ = HIGH_VALID_LIFETIME; // Very high valid lifetime + lease->cltt_ = DEC_2030_TIME; // December 11th 2030 lease->subnet_id_ = subnet_id; lease->fqdn_fwd_ = false; lease->fqdn_rev_ = true; @@ -327,9 +335,9 @@ public: /// @brief Creates an IPv6 lease /// - /// Lease parameters: cltt = 12345678, fqdn-fwd = false, fqdn-rev = true, + /// Lease parameters: cltt = 1923222072, fqdn-fwd = false, fqdn-rev = true, /// hostname = myhost.example.com, preferred lifetime = 1800, - /// valid lifetime = 3600 + /// valid lifetime = 0xFFFFFFFE /// /// @param ip_address IP address for the lease. /// @param subnet_id subnet identifier @@ -346,8 +354,10 @@ public: lease->iaid_ = 42; lease->duid_ = DuidPtr(new DUID(vector(8, duid_pattern))); lease->preferred_lft_ = 1800; - lease->valid_lft_ = 3600; - lease->cltt_ = 12345678; + // Purposely using high cltt and valid lifetime to test that + // expiration time is cast properly. + lease->valid_lft_ = HIGH_VALID_LIFETIME; // Very high valid lifetime + lease->cltt_ = DEC_2030_TIME; // December 11th 2030 lease->subnet_id_ = subnet_id; lease->fqdn_fwd_ = false; lease->fqdn_rev_ = true; @@ -398,19 +408,25 @@ public: } // Check that other parameters are there. - EXPECT_TRUE(l->contains("valid-lft")); - EXPECT_TRUE(l->contains("cltt")); - EXPECT_TRUE(l->contains("subnet-id")); - EXPECT_TRUE(l->contains("state")); - EXPECT_TRUE(l->contains("fqdn-fwd")); - EXPECT_TRUE(l->contains("fqdn-rev")); - EXPECT_TRUE(l->contains("hostname")); - EXPECT_TRUE(l->contains("state")); + ASSERT_TRUE(l->contains("valid-lft")); + ASSERT_TRUE(l->contains("cltt")); + ASSERT_TRUE(l->contains("subnet-id")); + ASSERT_TRUE(l->contains("state")); + ASSERT_TRUE(l->contains("fqdn-fwd")); + ASSERT_TRUE(l->contains("fqdn-rev")); + ASSERT_TRUE(l->contains("hostname")); + ASSERT_TRUE(l->contains("state")); // Check that there are no v6 specific fields - EXPECT_FALSE(l->contains("prefix")); - EXPECT_FALSE(l->contains("duid")); - EXPECT_FALSE(l->contains("preferred-lft")); + ASSERT_FALSE(l->contains("prefix")); + ASSERT_FALSE(l->contains("duid")); + ASSERT_FALSE(l->contains("preferred-lft")); + + // Assuming that these values were used to create the lease. + // If we ever want to test different values they will need to + // be added as parameters to this function. + EXPECT_EQ(HIGH_VALID_LIFETIME, l->get("valid-lft")->intValue()); + EXPECT_EQ(DEC_2030_TIME, l->get("cltt")->intValue()); } /// @brief Checks if specified response contains IPv6 lease @@ -462,17 +478,23 @@ public: } // Check that there are expected fields - EXPECT_TRUE(l->contains("preferred-lft")); - EXPECT_TRUE(l->contains("valid-lft")); - EXPECT_TRUE(l->contains("cltt")); - EXPECT_TRUE(l->contains("subnet-id")); - EXPECT_TRUE(l->contains("fqdn-fwd")); - EXPECT_TRUE(l->contains("fqdn-rev")); - EXPECT_TRUE(l->contains("hostname")); - EXPECT_TRUE(l->contains("state")); + ASSERT_TRUE(l->contains("preferred-lft")); + ASSERT_TRUE(l->contains("valid-lft")); + ASSERT_TRUE(l->contains("cltt")); + ASSERT_TRUE(l->contains("subnet-id")); + ASSERT_TRUE(l->contains("fqdn-fwd")); + ASSERT_TRUE(l->contains("fqdn-rev")); + ASSERT_TRUE(l->contains("hostname")); + ASSERT_TRUE(l->contains("state")); // Check that there are no v4 specific fields. - EXPECT_FALSE(l->contains("client-id")); + ASSERT_FALSE(l->contains("client-id")); + + // Assuming that these values were used to create the lease. + // If we ever want to test different values they will need to + // be added as parameters to this function. + EXPECT_EQ(HIGH_VALID_LIFETIME, l->get("valid-lft")->intValue()); + EXPECT_EQ(DEC_2030_TIME, l->get("cltt")->intValue()); } }; @@ -751,6 +773,33 @@ TEST_F(LeaseCmdsTest, Lease4AddSubnetIdMissingBadAddr) { ASSERT_FALSE(l); } +// Check that the lease with negative expiration time is rejected. +TEST_F(LeaseCmdsTest, Lease4AddNegativeExpireTime) { + + // Initialize lease manager (false = v4, false = don't add leases) + initLeaseMgr(false, false); + + // Check that the lease manager pointer is there. + ASSERT_TRUE(lmptr_); + + // Add a lease with negative expiration time. + string txt = + "{\n" + " \"command\": \"lease4-add\",\n" + " \"arguments\": {" + " \"ip-address\": \"192.0.2.202\",\n" + " \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"expire\": -6218189367\n" + " }\n" + "}"; + string exp_rsp = "expiration time must be positive for address 192.0.2.202"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + + // Now check that the lease was not added. + Lease4Ptr l = lmptr_->getLease4(IOAddress("192.0.2.202")); + ASSERT_FALSE(l); +} + // Check that a well formed lease4 with tons of parameters can be added. TEST_F(LeaseCmdsTest, Lease4AddFull) { @@ -770,7 +819,7 @@ TEST_F(LeaseCmdsTest, Lease4AddFull) { " \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n" " \"client-id\": \"01:02:03:04:05:06:07:08\",\n" " \"valid-lft\": 1000,\n" - " \"expire\": 12345678,\n" + " \"expire\": 6218189367,\n" " \"fqdn-fwd\": true,\n" " \"fqdn-rev\": true,\n" " \"hostname\": \"urania.example.org\",\n" @@ -788,7 +837,7 @@ TEST_F(LeaseCmdsTest, Lease4AddFull) { EXPECT_EQ("1a:1b:1c:1d:1e:1f", l->hwaddr_->toText(false)); ASSERT_TRUE(l->client_id_); EXPECT_EQ("01:02:03:04:05:06:07:08", l->client_id_->toText()); - EXPECT_EQ(l->cltt_, 12344678); // expire (12345678) - valid_lft(1000) + EXPECT_EQ(6218189367 - 1000, l->cltt_); // expire (6218189367) - valid_lft(1000) EXPECT_EQ(true, l->fqdn_fwd_); EXPECT_EQ(true, l->fqdn_rev_); EXPECT_EQ("urania.example.org", l->hostname_); @@ -1011,6 +1060,23 @@ TEST_F(LeaseCmdsTest, Lease6AddBadParams) { exp_rsp = "Duplicated comment entry '\"direct\"' in user context " "'{ \"comment\": \"in user context\" }'"; testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + + // Negative expiration time. + txt = + "{\n" + " \"command\": \"lease6-add\",\n" + " \"arguments\": {" + " \"subnet-id\": 66,\n" + " \"ip-address\": \"2001:db8:1::1\",\n" + " \"duid\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"iaid\": 1234\n," + " \"user-context\": { \"comment\": \"in user context\" },\n" + " \"expire\": -6218189367\n" + " }\n" + "}"; + exp_rsp = "expiration time must be positive for address 2001:db8:1::1"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + } // Check that a simple, well formed lease6 can be added. @@ -1157,7 +1223,7 @@ TEST_F(LeaseCmdsTest, Lease6AddFullAddr) { " \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n" " \"preferred-lft\": 500,\n" " \"valid-lft\": 1000,\n" - " \"expire\": 12345678,\n" + " \"expire\": 6218189367,\n" " \"fqdn-fwd\": true,\n" " \"fqdn-rev\": true,\n" " \"hostname\": \"urania.example.org\",\n" @@ -1176,7 +1242,7 @@ TEST_F(LeaseCmdsTest, Lease6AddFullAddr) { EXPECT_EQ("1a:1b:1c:1d:1e:1f", l->hwaddr_->toText(false)); ASSERT_TRUE(l->duid_); EXPECT_EQ("01:02:03:04:05:06:07:08", l->duid_->toText()); - EXPECT_EQ(l->cltt_, 12344678); // expire (12345678) - valid_lft(1000) + EXPECT_EQ(6218189367 - 1000, l->cltt_); // expire (6218189367) - valid_lft(1000) EXPECT_EQ(true, l->fqdn_fwd_); EXPECT_EQ(true, l->fqdn_rev_); EXPECT_EQ("urania.example.org", l->hostname_); -- GitLab From 3fcdc28814310ac93b3903897e952b3b60cfa0bc Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 11 Dec 2018 18:13:14 +0100 Subject: [PATCH 2/2] [#337,!167] Guard against expiration time lower than valid lifetime. --- src/hooks/dhcp/lease_cmds/lease_parser.cc | 9 ++++ .../lease_cmds/tests/lease_cmds_unittest.cc | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/hooks/dhcp/lease_cmds/lease_parser.cc b/src/hooks/dhcp/lease_cmds/lease_parser.cc index e7dd3cbdee..24b252ba2d 100644 --- a/src/hooks/dhcp/lease_cmds/lease_parser.cc +++ b/src/hooks/dhcp/lease_cmds/lease_parser.cc @@ -99,6 +99,10 @@ Lease4Parser::parse(ConstSrvConfigPtr& cfg, if (expire_time <= 0) { isc_throw(BadValue , "expiration time must be positive for address " << addr); + + } else if (expire_time < valid_lft) { + isc_throw(BadValue, "expiration time must be greater than valid lifetime" + " for address " << addr); } cltt = static_cast(expire_time - valid_lft); } else { @@ -290,7 +294,12 @@ Lease6Parser::parse(ConstSrvConfigPtr& cfg, if (expire_time <= 0) { isc_throw(BadValue , "expiration time must be positive for address " << addr); + + } else if (expire_time < valid_lft) { + isc_throw(BadValue, "expiration time must be greater than valid lifetime" + " for address " << addr); } + cltt = static_cast(expire_time - valid_lft); } else { cltt = time(NULL); diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc index 3fbb1e22e2..6c6cdd6e39 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc @@ -800,6 +800,35 @@ TEST_F(LeaseCmdsTest, Lease4AddNegativeExpireTime) { ASSERT_FALSE(l); } +// Check that the lease with negative cltt is rejected. +TEST_F(LeaseCmdsTest, Lease4AddNegativeCltt) { + + // Initialize lease manager (false = v4, false = don't add leases) + initLeaseMgr(false, false); + + // Check that the lease manager pointer is there. + ASSERT_TRUE(lmptr_); + + // Add a lease with negative cltt (expiration time - valid lifetime) + string txt = + "{\n" + " \"command\": \"lease4-add\",\n" + " \"arguments\": {" + " \"ip-address\": \"192.0.2.202\",\n" + " \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"expire\": 123456,\n" + " \"valid-lft\": 123457" + " }\n" + "}"; + string exp_rsp = "expiration time must be greater than valid lifetime for " + "address 192.0.2.202"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + + // Now check that the lease was not added. + Lease4Ptr l = lmptr_->getLease4(IOAddress("192.0.2.202")); + ASSERT_FALSE(l); +} + // Check that a well formed lease4 with tons of parameters can be added. TEST_F(LeaseCmdsTest, Lease4AddFull) { @@ -1077,6 +1106,24 @@ TEST_F(LeaseCmdsTest, Lease6AddBadParams) { exp_rsp = "expiration time must be positive for address 2001:db8:1::1"; testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + // Negative cltt + txt = + "{\n" + " \"command\": \"lease6-add\",\n" + " \"arguments\": {" + " \"subnet-id\": 66,\n" + " \"ip-address\": \"2001:db8:1::1\",\n" + " \"duid\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"iaid\": 1234\n," + " \"user-context\": { \"comment\": \"in user context\" },\n" + " \"expire\": 123456,\n" + " \"valid-lft\": 123457" + " }\n" + "}"; + exp_rsp = "expiration time must be greater than valid lifetime for address " + "2001:db8:1::1"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + } // Check that a simple, well formed lease6 can be added. -- GitLab