From: Thomas Markwalder Date: Wed, 4 Sep 2019 15:38:07 +0000 (-0400) Subject: [#821,!501] kea-dhcp4 now sanity checks inbound messages X-Git-Tag: Kea-1.7.0~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=83d5b6333487953eb34b79598a9b76093fe97271;p=thirdparty%2Fkea.git [#821,!501] kea-dhcp4 now sanity checks inbound messages src/bin/dhcp4/dhcp4_srv.cc Dhcpv4Srv::processRequest() Dhcpv4Srv::processRelease() Dhcpv4Srv::processDecline() Dhcpv4Srv::processInform() - now all call sanityCheck() src/bin/dhcp4/tests/dhcp4_srv_unittest.cc TEST_F(Dhcpv4SrvTest, sanityCheckDiscover) TEST_F(Dhcpv4SrvTest, sanityCheckRequest) TEST_F(Dhcpv4SrvTest, sanityCheckDecline) TEST_F(Dhcpv4SrvTest, sanityCheckRelease) TEST_F(Dhcpv4SrvTest, sanityCheckInform) - new tests src/lib/testutils/gtest_utils.h New file with handy new test macros: EXPECT_THROW_MSG() ASSERT_THROW_MSG() src/lib/testutils/Makefile.am Added new file gtest_utils.h Added a ChangeLog entry --- diff --git a/ChangeLog b/ChangeLog index 9c07f26e54..0a8884589b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +1661. [bug] tmark + kea-dhcp4 now rejects inbound client messages that have + neither a hardware address nor a client identifier. + (Gitlab #821,!501, git TBD) + 1660. [func] franek Statistics of the DHCP packets are now initialized upon the server startup. This makes the statistics available for fetching diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index aa8ae547cc..f658489a56 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2661,6 +2661,7 @@ Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) { Pkt4Ptr Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { + // server-id is forbidden. sanityCheck(discover, FORBIDDEN); bool drop = false; @@ -2720,8 +2721,9 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { Pkt4Ptr Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& context) { - /// @todo Uncomment this (see ticket #3116) - /// sanityCheck(request, MANDATORY); + // Since we cannot distinguish between client states + // we'll make server-id is optional for REQUESTs. + sanityCheck(request, OPTIONAL); bool drop = false; Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request, drop)); @@ -2782,8 +2784,9 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& cont void Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& context) { - /// @todo Uncomment this (see ticket #3116) - /// sanityCheck(release, MANDATORY); + // Server-id is mandatory in DHCPRELEASE (see table 5, RFC2131) + // but ISC DHCP does not enforce this, so we'll follow suit. + sanityCheck(release, OPTIONAL); // Try to find client-id. Note that for the DHCPRELEASE we don't check if the // match-client-id configuration parameter is disabled because this parameter @@ -2897,10 +2900,9 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& cont void Dhcpv4Srv::processDecline(Pkt4Ptr& decline, AllocEngine::ClientContext4Ptr& context) { - // Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131) - /// @todo Uncomment this (see ticket #3116) - // sanityCheck(decline, MANDATORY); + // but ISC DHCP does not enforce this, so we'll follow suit. + sanityCheck(decline, OPTIONAL); // Client is supposed to specify the address being declined in // Requested IP address option, but must not set its ciaddr. @@ -2911,7 +2913,7 @@ Dhcpv4Srv::processDecline(Pkt4Ptr& decline, AllocEngine::ClientContext4Ptr& cont if (!opt_requested_address) { isc_throw(RFCViolation, "Mandatory 'Requested IP address' option missing" - "in DHCPDECLINE sent from " << decline->getLabel()); + " in DHCPDECLINE sent from " << decline->getLabel()); } IOAddress addr(opt_requested_address->readAddress()); @@ -3047,8 +3049,9 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline, Pkt4Ptr Dhcpv4Srv::processInform(Pkt4Ptr& inform) { - // DHCPINFORM MUST not include server identifier. - sanityCheck(inform, FORBIDDEN); + // server-id is supposed to be forbidden (as is requested address) + // but ISC DHCP does not enforce either. So neither will we. + sanityCheck(inform, OPTIONAL); bool drop = false; Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform, drop)); @@ -3319,15 +3322,15 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) { switch (serverid) { case FORBIDDEN: if (server_id) { - isc_throw(RFCViolation, "Server-id option was not expected, but " - << "received in " + isc_throw(RFCViolation, "Server-id option was not expected, but" + << " received in message " << query->getName()); } break; case MANDATORY: if (!server_id) { - isc_throw(RFCViolation, "Server-id option was expected, but not " + isc_throw(RFCViolation, "Server-id option was expected, but not" " received in message " << query->getName()); } @@ -3349,7 +3352,7 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) { // If there's no client-id (or a useless one is provided, i.e. 0 length) if (!client_id || client_id->len() == client_id->getHeaderLen()) { - isc_throw(RFCViolation, "Missing or useless client-id and no HW address " + isc_throw(RFCViolation, "Missing or useless client-id and no HW address" " provided in message " << query->getName()); } diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 9cd123b2f3..5c0b5d186b 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -32,8 +32,8 @@ #include #include #include -#include #include +#include #include #include @@ -717,12 +717,212 @@ TEST_F(Dhcpv4SrvTest, processRequest) { testDiscoverRequest(DHCPREQUEST); } -TEST_F(Dhcpv4SrvTest, processRelease) { +// Verifies that DHCPDISCOVERs are sanity checked correctly. +// 1. They must have either hardware address or client id +// 2. They must not have server id +TEST_F(Dhcpv4SrvTest, sanityCheckDiscover) { + NakedDhcpv4Srv srv; + Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 1234)); + + // Should throw, no hardware address or client id + ASSERT_THROW_MSG(srv.processDiscover(pkt), RFCViolation, + "Missing or useless client-id and no HW address" + " provided in message DHCPDISCOVER"); + + // Add a hardware address. This should not throw. + uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; + HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER)); + pkt->setHWAddr(hwaddr); + ASSERT_NO_THROW(srv.processDiscover(pkt)); + + // Now let's make a new pkt with client-id only, it should not throw. + pkt.reset(new Pkt4(DHCPDISCOVER, 1234)); + pkt->addOption(generateClientId()); + ASSERT_NO_THROW(srv.processDiscover(pkt)); + + // Now let's add a server-id. This should throw. + OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_SERVER_IDENTIFIER); + ASSERT_TRUE(server_id_def); + + OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4)); + server_id->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(server_id); + EXPECT_THROW_MSG(srv.processDiscover(pkt), RFCViolation, + "Server-id option was not expected," + " but received in message DHCPDISCOVER"); +} + +// Verifies that DHCPREQEUSTs are sanity checked correctly. +// 1. They must have either hardware address or client id +// 2. They must have a requested address +// 3. They may or may not have a server id +TEST_F(Dhcpv4SrvTest, sanityCheckRequest) { + NakedDhcpv4Srv srv; + Pkt4Ptr pkt(new Pkt4(DHCPREQUEST, 1234)); + + // Should throw, no hardware address or client id + ASSERT_THROW_MSG(srv.processRequest(pkt), RFCViolation, + "Missing or useless client-id and no HW address" + " provided in message DHCPREQUEST"); + + // Add a hardware address. Should not throw. + uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; + HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER)); + pkt->setHWAddr(hwaddr); + EXPECT_NO_THROW(srv.processRequest(pkt)); + + // Now let's add a requested address. This should not throw. + OptionDefinitionPtr req_addr_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_REQUESTED_ADDRESS); + ASSERT_TRUE(req_addr_def); + OptionCustomPtr req_addr(new OptionCustom(*req_addr_def, Option::V4)); + req_addr->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(req_addr); + ASSERT_NO_THROW(srv.processRequest(pkt)); + + // Now let's make a new pkt with client-id only and an address, it should not throw. + pkt.reset(new Pkt4(DHCPREQUEST, 1234)); + pkt->addOption(generateClientId()); + pkt->addOption(req_addr); + ASSERT_NO_THROW(srv.processRequest(pkt)); + + // Now let's add a server-id. This should not throw. + OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_SERVER_IDENTIFIER); + ASSERT_TRUE(server_id_def); + + OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4)); + server_id->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(server_id); + EXPECT_NO_THROW(srv.processRequest(pkt)); +} + +// Verifies that DHCPDECLINEs are sanity checked correctly. +// 1. They must have either hardware address or client id +// 2. They must have a requested address +// 3. They may or may not have a server id +TEST_F(Dhcpv4SrvTest, sanityCheckDecline) { + NakedDhcpv4Srv srv; + Pkt4Ptr pkt(new Pkt4(DHCPDECLINE, 1234)); + + // Should throw, no hardware address or client id + ASSERT_THROW_MSG(srv.processDecline(pkt), RFCViolation, + "Missing or useless client-id and no HW address" + " provided in message DHCPDECLINE"); + + // Add a hardware address. Should throw because of missing address. + uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; + HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER)); + pkt->setHWAddr(hwaddr); + ASSERT_THROW_MSG(srv.processDecline(pkt), RFCViolation, + "Mandatory 'Requested IP address' option missing in DHCPDECLINE" + " sent from [hwtype=1 00:fe:fe:fe:fe:fe], cid=[no info], tid=0x4d2"); + + + // Now let's add a requested address. This should not throw. + OptionDefinitionPtr req_addr_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_REQUESTED_ADDRESS); + ASSERT_TRUE(req_addr_def); + OptionCustomPtr req_addr(new OptionCustom(*req_addr_def, Option::V4)); + req_addr->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(req_addr); + ASSERT_NO_THROW(srv.processDecline(pkt)); + + // Now let's make a new pkt with client-id only and an address, it should not throw. + pkt.reset(new Pkt4(DHCPDECLINE, 1234)); + pkt->addOption(generateClientId()); + pkt->addOption(req_addr); + ASSERT_NO_THROW(srv.processDecline(pkt)); + + // Now let's add a server-id. This should not throw. + OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_SERVER_IDENTIFIER); + ASSERT_TRUE(server_id_def); + + OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4)); + server_id->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(server_id); + EXPECT_NO_THROW(srv.processDecline(pkt)); +} + +// Verifies that DHCPRELEASEs are sanity checked correctly. +// 1. They must have either hardware address or client id +// 2. They may or may not have a server id +TEST_F(Dhcpv4SrvTest, sanityCheckRelease) { NakedDhcpv4Srv srv; Pkt4Ptr pkt(new Pkt4(DHCPRELEASE, 1234)); - // Should not throw + // Should throw, no hardware address or client id + ASSERT_THROW_MSG(srv.processRelease(pkt), RFCViolation, + "Missing or useless client-id and no HW address" + " provided in message DHCPRELEASE"); + + // Add a hardware address. Should not throw. + uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; + HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER)); + pkt->setHWAddr(hwaddr); EXPECT_NO_THROW(srv.processRelease(pkt)); + + // Make a new pkt with client-id only. Should not throw. + pkt.reset(new Pkt4(DHCPRELEASE, 1234)); + pkt->addOption(generateClientId()); + ASSERT_NO_THROW(srv.processRelease(pkt)); + + // Now let's add a server-id. This should not throw. + OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_SERVER_IDENTIFIER); + ASSERT_TRUE(server_id_def); + + OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4)); + server_id->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(server_id); + EXPECT_NO_THROW(srv.processRelease(pkt)); +} + +// Verifies that DHCPINFORMs are sanity checked correctly. +// 1. They must have either hardware address or client id +// 2. They may or may not have requested address +// 3. They may or may not have a server id +TEST_F(Dhcpv4SrvTest, sanityCheckInform) { + NakedDhcpv4Srv srv; + Pkt4Ptr pkt(new Pkt4(DHCPINFORM, 1234)); + + // Should throw, no hardware address or client id + ASSERT_THROW_MSG(srv.processInform(pkt), RFCViolation, + "Missing or useless client-id and no HW address" + " provided in message DHCPINFORM"); + + // Add a hardware address. Should not throw. + uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe}; + HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER)); + pkt->setHWAddr(hwaddr); + ASSERT_NO_THROW(srv.processInform(pkt)); + + // Now let's add a requested address. This should not throw. + OptionDefinitionPtr req_addr_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_REQUESTED_ADDRESS); + ASSERT_TRUE(req_addr_def); + OptionCustomPtr req_addr(new OptionCustom(*req_addr_def, Option::V4)); + req_addr->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(req_addr); + ASSERT_NO_THROW(srv.processInform(pkt)); + + // Now let's make a new pkt with client-id only and an address, it should not throw. + pkt.reset(new Pkt4(DHCPINFORM, 1234)); + pkt->addOption(generateClientId()); + pkt->addOption(req_addr); + ASSERT_NO_THROW(srv.processInform(pkt)); + + // Now let's add a server-id. This should not throw. + OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE, + DHO_DHCP_SERVER_IDENTIFIER); + ASSERT_TRUE(server_id_def); + + OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4)); + server_id->writeAddress(IOAddress("192.0.2.3")); + pkt->addOption(server_id); + EXPECT_NO_THROW(srv.processInform(pkt)); } // This test verifies that incoming DISCOVER can be handled properly, that an diff --git a/src/lib/testutils/Makefile.am b/src/lib/testutils/Makefile.am index 4eeb3c7aa6..1afd543328 100644 --- a/src/lib/testutils/Makefile.am +++ b/src/lib/testutils/Makefile.am @@ -14,6 +14,7 @@ libkea_testutils_la_SOURCES += test_to_element.cc test_to_element.h libkea_testutils_la_SOURCES += threaded_test.cc threaded_test.h libkea_testutils_la_SOURCES += unix_control_client.cc unix_control_client.h libkea_testutils_la_SOURCES += user_context_utils.cc user_context_utils.h +libkea_testutils_la_SOURCES += gtest_utils.h libkea_testutils_la_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES) libkea_testutils_la_LIBADD = $(top_builddir)/src/lib/asiolink/libkea-asiolink.la libkea_testutils_la_LIBADD += $(top_builddir)/src/lib/dns/libkea-dns++.la diff --git a/src/lib/testutils/gtest_utils.h b/src/lib/testutils/gtest_utils.h new file mode 100644 index 0000000000..c8820b0aa0 --- /dev/null +++ b/src/lib/testutils/gtest_utils.h @@ -0,0 +1,60 @@ +// Copyright (C) 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 +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#ifndef GTEST_UTIL_H +#define GTEST_UTIL_H + +#include + +namespace isc { +namespace test { + +/// @brief Verifies an expected exception type and message +/// +/// If the statement does not generate the expected exception +/// containing the expected message it will generate a non-fatal +/// failure. +/// +/// @param statement - statement block to execute +/// @param etype - type of exception expected +/// @param emsg - exact content expected to be returned by ex.what() +#define EXPECT_THROW_MSG(statement,etype,emsg) \ +{ \ + try { \ + statement; \ + ADD_FAILURE() << "no exception, expected:" << #etype; \ + } catch (const etype& ex) { \ + EXPECT_EQ(std::string(ex.what()), emsg); \ + } catch (...) { \ + ADD_FAILURE() << "wrong exception type thrown, expected " << #etype; \ + } \ +} \ + +/// @brief Verifies an expected exception type and message +/// +/// If the statement does not generate the expected exception +/// containing the expected message it will generate a fatal +/// failure. +/// +/// @param statement - statement block to execute +/// @param etype - type of exception expected +/// @param emsg - exact content expected to be returned by ex.what() +#define ASSERT_THROW_MSG(statement,etype,emsg) \ +{ \ + try { \ + statement; \ + GTEST_FAIL() << "no exception, expected:" << #etype; \ + } catch (const etype& ex) { \ + ASSERT_EQ(std::string(ex.what()), emsg); \ + } catch (...) { \ + GTEST_FAIL() << "wrong exception type thrown, expected " << #etype; \ + } \ +} \ + +}; // end of isc::test namespace +}; // end of isc namespace + +#endif // GTEST_UTIL_H