From: Marcin Siodelski Date: Wed, 20 Nov 2013 13:04:40 +0000 (+0100) Subject: [3035] Skip processing invalid Hostname options sent by a client. X-Git-Tag: bind10-1.2.0beta1-release~102^2~21^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9ea3131be861042b883033530ffaec46d190dfa2;p=thirdparty%2Fkea.git [3035] Skip processing invalid Hostname options sent by a client. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index d7b260797c..b4b834f7c2 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -27,6 +27,11 @@ successfully established a session with the BIND 10 control channel. This debug message is issued just before the IPv4 DHCP server attempts to establish a session with the BIND 10 control channel. +% DHCP4_CLIENT_NAME_PROC_FAIL failed to process the fqdn or hostname sent by a client: %1 +This debug message is issued when the DHCP server was unable to process the +FQDN or Hostname option sent by a client. This is likely because the client's +name was malformed or due to internal server error. + % DHCP4_COMMAND_RECEIVED received command %1, arguments: %2 A debug message listing the command (and possible arguments) received from the BIND 10 control system by the IPv4 DHCP server. @@ -75,6 +80,11 @@ This error message is logged when the attempt to compute DHCID for a specified lease has failed. The lease details and reason for failure is logged in the message. +% DHCP4_EMPTY_HOSTNAME received empty hostname from the client, skipping processing of this option +This debug message is issued when the server received an empty Hostname option +from a client. Server does not process empty Hostname options and therefore +option is skipped. + % DHCP4_HOOK_BUFFER_RCVD_SKIP received DHCPv4 buffer was dropped because a callout set the skip flag. This debug message is printed when a callout installed on buffer4_receive hook point set the skip flag. For this particular hook point, the diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index dcec6d03eb..b14bd20341 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -33,6 +33,7 @@ #include #include #include +#include #include @@ -747,18 +748,31 @@ Dhcpv4Srv::processClientName(const Pkt4Ptr& query, Pkt4Ptr& answer) { // It is possible that client has sent both Client FQDN and Hostname // option. In such case, server should prefer Client FQDN option and // ignore the Hostname option. - Option4ClientFqdnPtr fqdn = boost::dynamic_pointer_cast - (query->getOption(DHO_FQDN)); - if (fqdn) { - processClientFqdnOption(fqdn, answer); + try { + Option4ClientFqdnPtr fqdn = boost::dynamic_pointer_cast + (query->getOption(DHO_FQDN)); + if (fqdn) { + processClientFqdnOption(fqdn, answer); - } else { - OptionCustomPtr hostname = boost::dynamic_pointer_cast - (query->getOption(DHO_HOST_NAME)); - if (hostname) { - processHostnameOption(hostname, answer); - } + } else { + OptionCustomPtr hostname = boost::dynamic_pointer_cast + (query->getOption(DHO_HOST_NAME)); + if (hostname) { + processHostnameOption(hostname, answer); + } + } + } catch (const Exception& ex) { + // In some rare cases it is possible that the client's name processing + // fails. For example, the Hostname option may be malformed, or there + // may be an error in the server's logic which would cause multiple + // attempts to add the same option to the response message. This + // error message aggregates all these errors so they can be diagnosed + // from the log. We don't want to throw an exception here because, + // it will impact the processing of the whole packet. We rather want + // the processing to continue, even if the client's name is wrong. + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_CLIENT_NAME_PROC_FAIL) + .arg(ex.what()); } } @@ -855,8 +869,14 @@ Dhcpv4Srv::processHostnameOption(const OptionCustomPtr& opt_hostname, return; } - std::string hostname = opt_hostname->readString(); + std::string hostname = isc::util::str::trim(opt_hostname->readString()); unsigned int label_count = OptionDataTypeUtil::getLabelCount(hostname); + // The hostname option sent by the client should be at least 1 octet long. + // If it isn't we ignore this option. + if (label_count == 0) { + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_EMPTY_HOSTNAME); + return; + } // Copy construct the hostname provided by the client. It is entirely // possible that we will use the hostname option provided by the client // to perform the DNS update and we will send the same option to him to @@ -870,8 +890,12 @@ Dhcpv4Srv::processHostnameOption(const OptionCustomPtr& opt_hostname, // By checking the number of labels present in the hostname we may infer // whether client has sent the fully qualified or unqualified hostname. - // If there is only one label, it is a root. We will have to generate - // the whole domain name for the client. + // @todo We may want to reconsider whether it is appropriate for the + // client to send a root domain name as a Hostname. There are + // also extensions to the auto generation of the client's name, + // e.g. conversion to the puny code which may be considered at some point. + // For now, we just remain liberal and expect that the DNS will handle + // conversion if needed and possible. if (FQDN_REPLACE_CLIENT_NAME || (label_count < 2)) { opt_hostname_resp->writeString(""); // If there are two labels, it means that the client has specified diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 59ecaced97..87e362ed87 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -194,11 +194,14 @@ public: } - // Test that the Hostname option is returned in the server's response - // to the message holding Hostname option sent by a client. - void testProcessHostname(const Pkt4Ptr& query, - const std::string& exp_hostname) { - ASSERT_TRUE(getHostnameOption(query)); + // Processes the Hostname option in the client's message and returns + // the hostname option which would be sent to the client. It will + // throw NULL pointer if the hostname option is not to be included + // in the response. + OptionCustomPtr processHostname(const Pkt4Ptr& query) { + if (!getHostnameOption(query)) { + ADD_FAILURE() << "Hostname option not carried in the query"; + } Pkt4Ptr answer; if (query->getType() == DHCPDISCOVER) { @@ -208,16 +211,13 @@ public: answer.reset(new Pkt4(DHCPACK, 1234)); } - ASSERT_NO_THROW(srv_->processClientName(query, answer)); + srv_->processClientName(query, answer); OptionCustomPtr hostname = getHostnameOption(answer); - ASSERT_TRUE(hostname); - - EXPECT_EQ(exp_hostname, hostname->readString()); + return (hostname); } - // Test that the client message holding an FQDN is processed and the // NameChangeRequests are generated. void testProcessMessageWithFqdn(const uint8_t msg_type, @@ -356,11 +356,37 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateForwardFqdn) { // responds with the Hostname option to confirm that the server has // taken responsibility for the update. TEST_F(NameDhcpv4SrvTest, serverUpdateHostname) { - Pkt4Ptr query = generatePktWithHostname(DHCPREQUEST, - "myhost.example.com."); - testProcessHostname(query, "myhost.example.com."); + Pkt4Ptr query; + ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST, + "myhost.example.com.")); + OptionCustomPtr hostname; + ASSERT_NO_THROW(hostname = processHostname(query)); + + ASSERT_TRUE(hostname); + EXPECT_EQ("myhost.example.com.", hostname->readString()); + +} + +// Test that the server skips processing of the empty Hostname option. +TEST_F(NameDhcpv4SrvTest, serverUpdateEmptyHostname) { + Pkt4Ptr query; + ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST, "")); + OptionCustomPtr hostname; + ASSERT_NO_THROW(hostname = processHostname(query)); + EXPECT_FALSE(hostname); +} + +// Test that the server skips processing of a wrong Hostname option. +TEST_F(NameDhcpv4SrvTest, serverUpdateWrongHostname) { + Pkt4Ptr query; + ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST, + "abc..example.com")); + OptionCustomPtr hostname; + ASSERT_NO_THROW(hostname = processHostname(query)); + EXPECT_FALSE(hostname); } + // Test that server generates the fully qualified domain name for the client // if client supplies the partial name. TEST_F(NameDhcpv4SrvTest, serverUpdateForwardPartialNameFqdn) { @@ -380,8 +406,14 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateForwardPartialNameFqdn) { // Test that server generates the fully qualified domain name for the client // if client supplies the unqualified name in the Hostname option. TEST_F(NameDhcpv4SrvTest, serverUpdateUnqualifiedHostname) { - Pkt4Ptr query = generatePktWithHostname(DHCPREQUEST, "myhost"); - testProcessHostname(query, "myhost.example.com."); + Pkt4Ptr query; + ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST, "myhost")); + OptionCustomPtr hostname; + ASSERT_NO_THROW(hostname = processHostname(query)); + + ASSERT_TRUE(hostname); + EXPECT_EQ("myhost.example.com.", hostname->readString()); + } // Test that server sets empty domain-name in the FQDN option when client @@ -587,7 +619,6 @@ TEST_F(NameDhcpv4SrvTest, processRequestEmptyHostname) { 0, subnet_->getValid()); } - // Test that client may send two requests, each carrying FQDN option with // a different domain-name. Server should use existing lease for the second // request but modify the DNS entries for the lease according to the contents diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc index b2e84a33cb..4c29596919 100644 --- a/src/lib/dhcp/option_data_types.cc +++ b/src/lib/dhcp/option_data_types.cc @@ -229,6 +229,14 @@ OptionDataTypeUtil::writeFqdn(const std::string& fqdn, unsigned int OptionDataTypeUtil::getLabelCount(const std::string& text_name) { + // The isc::dns::Name class doesn't accept empty names. However, in some + // cases we may be dealing with empty names (e.g. sent by the DHCP clients). + // Empty names should not be sent as hostnames but if they are, for some + // reason, we don't want to throw an exception from this function. We + // rather want to signal empty name by returning 0 number of labels. + if (text_name.empty()) { + return (0); + } try { isc::dns::Name name(text_name); return (name.getLabelCount()); diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h index 253776f29a..d9d8a52379 100644 --- a/src/lib/dhcp/option_data_types.h +++ b/src/lib/dhcp/option_data_types.h @@ -377,10 +377,13 @@ public: /// @brief Return the number of labels in the Name. /// + /// If the specified name is empty the 0 is returned. + /// /// @param text_name A text representation of the name. /// - /// @return A number of labels in the provided name. - /// @throw isc::BadCast if provided name is malformed. + /// @return A number of labels in the provided name or 0 if the + /// name string is empty. + /// @throw isc::dhcp::BadDataTypeCast if provided name is malformed. static unsigned int getLabelCount(const std::string& text_name); /// @brief Read string value from a buffer. diff --git a/src/lib/dhcp/tests/option_data_types_unittest.cc b/src/lib/dhcp/tests/option_data_types_unittest.cc index fd5294c309..717a330410 100644 --- a/src/lib/dhcp/tests/option_data_types_unittest.cc +++ b/src/lib/dhcp/tests/option_data_types_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -62,6 +62,20 @@ public: } }; +// The goal of this test is to verify that the getLabelCount returns the +// correct number of labels in the domain name specified as a string +// parameter. +TEST_F(OptionDataTypesTest, getLabelCount) { + EXPECT_EQ(0, OptionDataTypeUtil::getLabelCount("")); + EXPECT_EQ(1, OptionDataTypeUtil::getLabelCount(".")); + EXPECT_EQ(2, OptionDataTypeUtil::getLabelCount("example")); + EXPECT_EQ(3, OptionDataTypeUtil::getLabelCount("example.com")); + EXPECT_EQ(3, OptionDataTypeUtil::getLabelCount("example.com.")); + EXPECT_EQ(4, OptionDataTypeUtil::getLabelCount("myhost.example.com")); + EXPECT_THROW(OptionDataTypeUtil::getLabelCount(".abc."), + isc::dhcp::BadDataTypeCast); +} + // The goal of this test is to verify that an IPv4 address being // stored in a buffer (wire format) can be read into IOAddress // object.