From: Marcin Siodelski Date: Tue, 17 Mar 2015 10:37:41 +0000 (+0100) Subject: [3749] Relayed message with hops=0 is accepted by the DHCPv4 server. X-Git-Tag: kea-0.9.1~22^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=5c15d55e69562efb7a15a0d5e7b2b2c09265ef56;p=thirdparty%2Fkea.git [3749] Relayed message with hops=0 is accepted by the DHCPv4 server. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index e9528ae798..3aa3fad787 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -162,13 +162,6 @@ This debug message is issued when the client being in the INIT-REBOOT state requested an address which is not assigned to him. The server will respond to this client with DHCPNAK. -% DHCP4_INVALID_RELAY_INFO malformed packet received from client-id %1, hwaddr %2: %3 -This message is logged when the client sends invalid combination of -values in the giaddr and hops fields. If the packet is relayed it should -contain non-zero values in both fields. If the packet is sent from the -directly connected client, both values should be set to zero. All other -combinations are invalid and trigger packet drop. - % DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3) This debug message indicates that the server successfully advertised a lease. It is up to the client to choose one server out of other advertised diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index cab088d547..532376f0ca 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1770,23 +1770,9 @@ Dhcpv4Srv::accept(const Pkt4Ptr& query) const { bool Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const { - try { - // This is the first call to the isRelayed function for this packet, - // so we have to catch exceptions which will be emitted if the - // packet contains invalid combination of hops and giaddr. For all - // other invocations of isRelayed function we will not catch - // exceptions because we eliminate malformed packets here. - if (pkt->isRelayed()) { - return (true); - } - } catch (const Exception& ex) { - OptionPtr client_id = pkt->getOption(DHO_DHCP_CLIENT_IDENTIFIER); - HWAddrPtr hwaddr = pkt->getHWAddr(); - LOG_INFO(dhcp4_logger, DHCP4_INVALID_RELAY_INFO) - .arg(client_id ? client_id->toText():"(no client-id)") - .arg(hwaddr ? hwaddr->toText():"(no hwaddr info)") - .arg(ex.what()); - return (false); + // Accept all relayed messages. + if (pkt->isRelayed()) { + return (true); } // The source address must not be zero for the DHCPINFORM message from // the directly connected client because the server will not know where diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 5c9e2adbc8..97849fc7ea 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -342,45 +342,6 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataBroadcast) { } -// This test verifies that exception is thrown of the invalid combination -// of giaddr and hops is specified in a client's message. -TEST_F(Dhcpv4SrvTest, adjustIfaceDataInvalid) { - IfaceMgrTestConfig test_config(true); - IfaceMgr::instance().openSockets4(); - - boost::shared_ptr req(new Pkt4(DHCPDISCOVER, 1234)); - - // The hops and giaddr values are used to determine if the client's - // message has been relayed or sent directly. The allowed combinations - // are (giaddr = 0 and hops = 0) or (giaddr != 0 and hops != 0). Any - // other combination is invalid and the adjustIfaceData should throw - // an exception. We will test that exception is indeed thrown. - req->setGiaddr(IOAddress("0.0.0.0")); - req->setHops(1); - - // Clear client address as it hasn't got any address configured yet. - req->setCiaddr(IOAddress("0.0.0.0")); - // The query is sent to the broadcast address in the Select state. - req->setLocalAddr(IOAddress("255.255.255.255")); - // The query has been received on the DHCPv4 server port 67. - req->setLocalPort(DHCP4_SERVER_PORT); - // Set the interface. The response should be sent via the same interface. - req->setIface("eth0"); - req->setIndex(1); - - // Create the exchange using the req. - Dhcpv4Exchange ex = createExchange(req); - Pkt4Ptr resp = ex.getResponse(); - - // Assign some new address for this client. - resp->setYiaddr(IOAddress("192.0.2.13")); - - // Clear the remote address. - resp->setRemoteAddr(IOAddress("0.0.0.0")); - - EXPECT_THROW(NakedDhcpv4Srv::adjustIfaceData(ex), isc::BadValue); -} - // This test verifies that the server identifier option is appended to // a specified DHCPv4 message and the server identifier is correct. TEST_F(Dhcpv4SrvTest, appendServerID) { @@ -3479,16 +3440,15 @@ TEST_F(Dhcpv4SrvTest, acceptDirectRequest) { Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 1234)); // Set Giaddr and local server's unicast address, but don't set hops. - // Hops value must be greater than 0, when giaddr is set. Otherwise, - // message is considered malformed and the accept() function should - // return false. + // Hops value should not matter. The server will treat the message + // with the hops value of 0 and non-zero giaddr as relayed. pkt->setGiaddr(IOAddress("192.0.10.1")); pkt->setRemoteAddr(IOAddress("0.0.0.0")); pkt->setLocalAddr(IOAddress("192.0.2.3")); pkt->setIface("eth1"); - EXPECT_FALSE(srv.accept(pkt)); + EXPECT_TRUE(srv.accept(pkt)); - // Let's set hops and check that the message is now accepted as + // Let's set hops and check that the message is still accepted as // a relayed message. pkt->setHops(1); EXPECT_TRUE(srv.accept(pkt)); diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc index 2710789311..06fd12db30 100644 --- a/src/lib/dhcp/pkt4.cc +++ b/src/lib/dhcp/pkt4.cc @@ -436,21 +436,7 @@ Pkt4::addOption(const OptionPtr& opt) { bool Pkt4::isRelayed() const { - static const IOAddress zero_addr("0.0.0.0"); - // For non-relayed message both Giaddr and Hops are zero. - if (getGiaddr() == zero_addr && getHops() == 0) { - return (false); - - // For relayed message, both Giaddr and Hops are non-zero. - } else if (getGiaddr() != zero_addr && getHops() > 0) { - return (true); - } - // In any other case, the packet is considered malformed. - isc_throw(isc::BadValue, "invalid combination of giaddr = " - << getGiaddr().toText() << " and hops = " - << static_cast(getHops()) << ". Valid values" - " are: (giaddr = 0 and hops = 0) or (giaddr != 0 and" - "hops != 0)"); + return (!giaddr_.isV4Zero() && !giaddr_.isV4Bcast()); } } // end of namespace isc::dhcp diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h index 012cd6ab48..d93a56e695 100755 --- a/src/lib/dhcp/pkt4.h +++ b/src/lib/dhcp/pkt4.h @@ -332,17 +332,11 @@ public: /// This function returns a boolean value which indicates whether a DHCPv4 /// message has been relayed (if true is returned) or not (if false). /// - /// This function uses a combination of Giaddr and Hops. It is expected that - /// if Giaddr is not 0, the Hops is greater than 0. In this case the message - /// is considered relayed. If Giaddr is 0, the Hops value must also be 0. In - /// this case the message is considered non-relayed. For any other - /// combination of Giaddr and Hops, an exception is thrown to indicate that - /// the message is malformed. + /// The message is considered relayed if the giaddr field is non-zero and + /// non-broadcast. /// /// @return Boolean value which indicates whether the message is relayed /// (true) or non-relayed (false). - /// @throw isc::BadValue if invalid combination of Giaddr and Hops values is - /// found. bool isRelayed() const; /// @brief That's the data of input buffer used in RX packet. diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index 1ebf77125a..523ea8c727 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -800,25 +800,25 @@ TEST_F(Pkt4Test, hwaddrSrcRemote) { } // This test verifies that the check for a message being relayed is correct. -// It also checks that the exception is thrown if the combination of hops and -// giaddr is invalid. TEST_F(Pkt4Test, isRelayed) { Pkt4 pkt(DHCPDISCOVER, 1234); // By default, the hops and giaddr should be 0. - ASSERT_EQ("0.0.0.0", pkt.getGiaddr().toText()); + ASSERT_TRUE(pkt.getGiaddr().isV4Zero()); ASSERT_EQ(0, pkt.getHops()); - // For hops = 0 and giaddr = 0, the message is non-relayed. + // For zero giaddr the packet is non-relayed. EXPECT_FALSE(pkt.isRelayed()); - // Set giaddr but leave hops = 0. This should result in exception. + // Set giaddr but leave hops = 0. pkt.setGiaddr(IOAddress("10.0.0.1")); - EXPECT_THROW(pkt.isRelayed(), isc::BadValue); - // Set hops. Now both hops and giaddr is set. The message is relayed. + EXPECT_TRUE(pkt.isRelayed()); + // After setting hops the message should still be relayed. pkt.setHops(10); EXPECT_TRUE(pkt.isRelayed()); - // Set giaddr to 0. For hops being set to non-zero value the function - // should throw an exception. - pkt.setGiaddr(IOAddress("0.0.0.0")); - EXPECT_THROW(pkt.isRelayed(), isc::BadValue); + // Set giaddr to 0. The message is now not-relayed. + pkt.setGiaddr(IOAddress(IOAddress::IPV4_ZERO_ADDRESS())); + EXPECT_FALSE(pkt.isRelayed()); + // Setting the giaddr to 255.255.255.255 should not cause it to + // be relayed message. + pkt.setGiaddr(IOAddress(IOAddress::IPV4_BCAST_ADDRESS())); } // Tests whether a packet can be assigned to a class and later