From: Thomas Markwalder Date: Wed, 25 Jul 2018 19:58:33 +0000 (-0400) Subject: [5440] libdhcp++ now quietly drops empty host name options from inbound packets X-Git-Tag: 421-create-config-backend-for-dhcpv6-base_base~18^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8d1f1d7f7265cc989bbbe464c49185d95f4f0cca;p=thirdparty%2Fkea.git [5440] libdhcp++ now quietly drops empty host name options from inbound packets src/lib/dhcp/libdhcp++.cc LibDHCP::unpackOptions4() - added logic to drop Host Name option if when empty src/lib/dhcp/tests/libdhcp++_unittest.cc TEST_F(LibDhcpTest, emptyHostName) - new unit test src/bin/dhcp4/dhcp4_srv.cc Dhcpv4Srv::processHostnameOption() - removed prior 5440 logic to ignore blank hostname options src/bin/dhcp4/dhcp4_messages.mes Removed prior 5440 message --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 83c1e21ba8..f0d486a6c3 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -98,11 +98,6 @@ client and transaction identification information. The second argument specifies the hostname carried in the Hostname option sent by the client. -% DHCP4_CLIENT_HOSTNAME_EMPTY %1: client sent bogus empty Hostname option -This informational message is issued when the server receives an -empty Hostname option. Such option is bogus (size is required to be -at least one) and is ignored. - % DHCP4_CLIENT_HOSTNAME_PROCESS %1: processing client's Hostname option This debug message is issued when the server starts processing the Hostname option sent in the client's query. The argument includes the client and diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index de43e92dd7..ab0525ae90 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1679,13 +1679,6 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) { OptionStringPtr opt_hostname = boost::dynamic_pointer_cast (ex.getQuery()->getOption(DHO_HOST_NAME)); - // Ignore empty/bogus Hostname option. - if (opt_hostname && opt_hostname->getValue().empty()) { - opt_hostname.reset(); - LOG_INFO(ddns4_logger, DHCP4_CLIENT_HOSTNAME_EMPTY) - .arg(ex.getQuery()->getLabel()); - } - if (opt_hostname) { LOG_DEBUG(ddns4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_CLIENT_HOSTNAME_DATA) .arg(ex.getQuery()->getLabel()) diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index d66a460d43..a787e4df77 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -836,7 +836,10 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateWrongHostname) { EXPECT_FALSE(hostname); } -// Test that the server skips processing of an empty Hostname option. +// Test that the server does not see an empty Hostname option. +// Suppressing the empty Hostname is done in libdhcp++ during +// unpackcing, so technically we don't need this test but, +// hey it's already written. TEST_F(NameDhcpv4SrvTest, serverUpdateEmptyHostname) { Pkt4Ptr query; ASSERT_NO_THROW(query = generatePktWithEmptyHostname(DHCPREQUEST)); @@ -845,7 +848,6 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateEmptyHostname) { 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) { diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index 9ef5c11c09..4572270de7 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -525,6 +525,15 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, return (last_offset); } + // While an empty Host Name option is non-RFC compliant, some clients + // do send it. In the spirit of being liberal, we'll just drop it, + // rather than the dropping the whole packet. We do not have a + // way to log this from here but meh... a PCAP will show it arriving, + // and we know we drop it. + if (opt_len == 0 && opt_type == DHO_HOST_NAME) { + continue; + } + // Get all definitions with the particular option code. Note // that option code is non-unique within this container // however at this point we expect to get one option diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 4dde17ba5a..43f64e7bbe 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -1016,6 +1016,37 @@ TEST_F(LibDhcpTest, unpackSubOptions4) { EXPECT_EQ(0x0, option_bar->getValue()); } +// Verifies that an Host Name (option 12), will be dropped when empty, +// while subsequent options will still be unpacked. +TEST_F(LibDhcpTest, emptyHostName) { + + uint8_t opts[] = { + 12, 0, // Empty Hostname + 60, 3, 10, 11, 12 // Class Id + }; + + vector packed(opts, opts + sizeof(opts)); + isc::dhcp::OptionCollection options; // list of options + list deferred; + + ASSERT_NO_THROW( + LibDHCP::unpackOptions4(packed, "dhcp4", options, deferred); + ); + + // Host Name should not exist, we quietly drop it when empty. + isc::dhcp::OptionCollection::const_iterator x = options.find(12); + ASSERT_TRUE(x == options.end()); + + // Verify Option 60 exists correctly + x = options.find(60); + ASSERT_FALSE(x == options.end()); + EXPECT_EQ(60, x->second->getType()); + ASSERT_EQ(3, x->second->getData().size()); + EXPECT_EQ(5, x->second->len()); + EXPECT_EQ(0, memcmp(&x->second->getData()[0], opts + 4, 3)); +}; + + TEST_F(LibDhcpTest, stdOptionDefs4) { // Create a buffer that holds dummy option data.