diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 6b82344dfb..be505911ba 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -81,6 +81,12 @@ This message indicates that the server failed to grant (in response to received REQUEST) a lease for a given client. There may be many reasons for such failure. Each specific failure is logged in a separate log entry. +% DHCP6_REQUIRED_OPTIONS_CHECK_FAIL %1 message received from %2 failed the following check: %3 +This message indicates that received message has invalid number of options: +mandatory client-id option is missing, server-id forbidden in that particular +type of message is present, there is more than one instance of client-id +or server-id etc. Exact reason for rejecting the packed is printed. + % DHCP6_NOT_RUNNING IPv6 DHCP server is not running A warning message is issued when an attempt is made to shut down the IPv6 DHCP server but it is not running. diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 876e954bc5..38bf46f09d 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -132,44 +132,51 @@ bool Dhcpv6Srv::run() { .arg(query->getBuffer().getLength()) .arg(query->toText()); - switch (query->getType()) { - case DHCPV6_SOLICIT: - rsp = processSolicit(query); - break; + try { + switch (query->getType()) { + case DHCPV6_SOLICIT: + rsp = processSolicit(query); + break; - case DHCPV6_REQUEST: - rsp = processRequest(query); - break; + case DHCPV6_REQUEST: + rsp = processRequest(query); + break; - case DHCPV6_RENEW: - rsp = processRenew(query); - break; + case DHCPV6_RENEW: + rsp = processRenew(query); + break; - case DHCPV6_REBIND: - rsp = processRebind(query); - break; + case DHCPV6_REBIND: + rsp = processRebind(query); + break; - case DHCPV6_CONFIRM: - rsp = processConfirm(query); - break; + case DHCPV6_CONFIRM: + rsp = processConfirm(query); + break; - case DHCPV6_RELEASE: + case DHCPV6_RELEASE: rsp = processRelease(query); break; - case DHCPV6_DECLINE: - rsp = processDecline(query); - break; + case DHCPV6_DECLINE: + rsp = processDecline(query); + break; - case DHCPV6_INFORMATION_REQUEST: - rsp = processInfRequest(query); - break; + case DHCPV6_INFORMATION_REQUEST: + rsp = processInfRequest(query); + break; - default: - // Only action is to output a message if debug is enabled, - // and that will be covered by the debug statement before - // the "switch" statement. - ; + default: + // Only action is to output a message if debug is enabled, + // and that will be covered by the debug statement before + // the "switch" statement. + ; + } + } catch (const RFCViolation& e) { + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_REQUIRED_OPTIONS_CHECK_FAIL) + .arg(query->getName()) + .arg(query->getRemoteAddr()) + .arg(e.what()); } if (rsp) { @@ -195,9 +202,6 @@ bool Dhcpv6Srv::run() { } } } - - // TODO add support for config session (see src/bin/auth/main.cc) - // so this daemon can be controlled from bob } return (true); @@ -357,6 +361,54 @@ OptionPtr Dhcpv6Srv::createStatusCode(uint16_t code, const std::string& text) { return (status); } +void Dhcpv6Srv::sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid, + RequirementLevel serverid) { + Option::OptionCollection client_ids = pkt->getOptions(D6O_CLIENTID); + switch (clientid) { + case MANDATORY: + if (client_ids.size() != 1) { + isc_throw(RFCViolation, "Exactly 1 client-id option expected in " + << pkt->getName() << ", but " << client_ids.size() + << " received"); + } + break; + case OPTIONAL: + if (client_ids.size() > 1) { + isc_throw(RFCViolation, "Too many (" << client_ids.size() + << ") client-id options received in " << pkt->getName()); + } + break; + + case FORBIDDEN: + // doesn't make sense - client-id is always allowed + break; + } + + Option::OptionCollection server_ids = pkt->getOptions(D6O_SERVERID); + switch (serverid) { + case FORBIDDEN: + if (server_ids.size() > 0) { + isc_throw(RFCViolation, "Exactly 1 server-id option expected, but " + << server_ids.size() << " received in " << pkt->getName()); + } + break; + + case MANDATORY: + if (server_ids.size() != 1) { + isc_throw(RFCViolation, "Invalid number of server-id options received (" + << server_ids.size() << "), exactly 1 expected in message " + << pkt->getName()); + } + break; + + case OPTIONAL: + if (server_ids.size() > 1) { + isc_throw(RFCViolation, "Too many (" << server_ids.size() + << ") client-id options received in " << pkt->getName()); + } + } +} + Subnet6Ptr Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question) { Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr()); @@ -370,7 +422,7 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) { // We need to select a subnet the client is connected in. Subnet6Ptr subnet = selectSubnet(question); - if (subnet) { + if (!subnet) { // This particular client is out of luck today. We do not have // information about the subnet he is connected to. This likely means // misconfiguration of the server (or some relays). We will continue to @@ -378,12 +430,13 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) { // addresses or prefixes, no subnet specific configuration etc. The only // thing this client can get is some global information (like DNS // servers). - LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED) - .arg(subnet->toText()); - } else { + // perhaps this should be logged on some higher level? This is most likely // configuration bug. LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_SUBNET_SELECTION_FAILED); + } else { + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED) + .arg(subnet->toText()); } // @todo: We should implement Option6Duid some day, but we can do without it @@ -514,6 +567,14 @@ OptionPtr Dhcpv6Srv::handleIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, Pkt6Ptr Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) { + if (!sanityCheck(solicit, MANDATORY, FORBIDDEN)) { + return (Pkt6Ptr()); + } + +Pkt6Ptr Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) { + + sanityCheck(solicit, MANDATORY, FORBIDDEN); + Pkt6Ptr advertise(new Pkt6(DHCPV6_ADVERTISE, solicit->getTransid())); copyDefaultOptions(solicit, advertise); @@ -526,6 +587,9 @@ Pkt6Ptr Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) { } Pkt6Ptr Dhcpv6Srv::processRequest(const Pkt6Ptr& request) { + + sanityCheck(request, MANDATORY, MANDATORY); + Pkt6Ptr reply(new Pkt6(DHCPV6_REPLY, request->getTransid())); copyDefaultOptions(request, reply); diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 60bc2928a4..782e7a8981 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -30,6 +30,23 @@ namespace isc { namespace dhcp { + +/// An exception that is thrown if a DHCPv6 protocol violation occurs while +/// processing a message (e.g. a mandatory option is missing) +class RFCViolation : public isc::Exception { +public: + +/// @brief constructor +/// +/// @param file name of the file, where exception occurred +/// @param line line of the file, where exception occurred +/// @param what text description of the issue that caused exception +RFCViolation(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) {} +}; + + + /// @brief DHCPv6 server service. /// /// This class represents DHCPv6 server. It contains all @@ -45,6 +62,12 @@ namespace dhcp { class Dhcpv6Srv : public boost::noncopyable { public: + /// @brief defines if certain option may, must or must not appear + typedef enum { + FORBIDDEN, + MANDATORY, + OPTIONAL + } RequirementLevel; /// @brief Minimum length of a MAC address to be used in DUID generation. static const size_t MIN_MAC_LEN = 6; @@ -84,6 +107,19 @@ public: void shutdown(); protected: + + /// @brief verifies if specified packet meets RFC requirements + /// + /// Checks if mandatory option is really there, that forbidden option + /// is not there, and that client-id or server-id appears only once. + /// + /// @param pkt packet to be checked + /// @param clientid expectation regarding client-id option + /// @param serverid expectation regarding server-id option + /// @throw RFCViolation if any issues are detected + void sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid, + RequirementLevel serverid); + /// @brief Processes incoming SOLICIT and returns response. /// /// Processes received SOLICIT message and verifies that its sender diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 12f2a27f2e..24273d3e0e 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -59,6 +59,7 @@ public: using Dhcpv6Srv::processRequest; using Dhcpv6Srv::createStatusCode; using Dhcpv6Srv::selectSubnet; + using Dhcpv6Srv::sanityCheck; }; class Dhcpv6SrvTest : public ::testing::Test { @@ -651,6 +652,9 @@ TEST_F(Dhcpv6SrvTest, RequestBasic) { OptionPtr clientid = generateClientId(); req->addOption(clientid); + // server-id is mandatory in REQUEST + req->addOption(srv->getServerID()); + // Pass it to the server and hope for a REPLY Pkt6Ptr reply = srv->processRequest(req); @@ -709,6 +713,11 @@ TEST_F(Dhcpv6SrvTest, ManyRequests) { req2->addOption(clientid2); req3->addOption(clientid3); + // server-id is mandatory in REQUEST + req1->addOption(srv->getServerID()); + req2->addOption(srv->getServerID()); + req3->addOption(srv->getServerID()); + // Pass it to the server and get an advertise Pkt6Ptr reply1 = srv->processRequest(req1); Pkt6Ptr reply2 = srv->processRequest(req2); @@ -763,8 +772,8 @@ TEST_F(Dhcpv6SrvTest, StatusCode) { EXPECT_TRUE(status->getData() == exp); } -// This test verifies if the selectSubnet() method works as expected. -TEST_F(Dhcpv6SrvTest, SelectSubnet) { +// This test verifies if the sanityCheck() really checks options presence. +TEST_F(Dhcpv6SrvTest, sanityCheck) { boost::scoped_ptr srv; ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv(0)) ); @@ -772,18 +781,63 @@ TEST_F(Dhcpv6SrvTest, SelectSubnet) { // check that the packets originating from local addresses can be pkt->setRemoteAddr(IOAddress("fe80::abcd")); - EXPECT_EQ(subnet_, srv->selectSubnet(pkt)); - // packets originating from subnet A will select subnet A - pkt->setRemoteAddr(IOAddress("2001:db8:1::6789")); - EXPECT_EQ(subnet_, srv->selectSubnet(pkt)); + // client-id is optional for information-request, so + EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::OPTIONAL)); - // packets from a subnet that is not supported will not get - // a subnet - pkt->setRemoteAddr(IOAddress("3000::faf")); - EXPECT_FALSE(srv->selectSubnet(pkt)); + // empty packet, no client-id, no server-id + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN), + RFCViolation); + + // This doesn't make much sense, but let's check it for completeness + EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::FORBIDDEN)); + + OptionPtr clientid = generateClientId(); + pkt->addOption(clientid); + + // client-id is mandatory, server-id is forbidden (as in SOLICIT or REBIND) + EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN)); + + pkt->addOption(srv->getServerID()); + + // both client-id and server-id are mandatory (as in REQUEST, RENEW, RELEASE, DECLINE) + EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY)); + + // sane section ends here, let's do some negative tests as well + + pkt->addOption(clientid); + pkt->addOption(clientid); + + // with more than one client-id it should throw, no matter what + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::OPTIONAL), + RFCViolation); + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::OPTIONAL), + RFCViolation); + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::MANDATORY), + RFCViolation); + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY), + RFCViolation); + + pkt->delOption(D6O_CLIENTID); + pkt->delOption(D6O_CLIENTID); + + // again we have only one client-id + + // let's try different type of insanity - several server-ids + pkt->addOption(srv->getServerID()); + pkt->addOption(srv->getServerID()); + + // with more than one server-id it should throw, no matter what + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::OPTIONAL), + RFCViolation); + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::OPTIONAL), + RFCViolation); + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::MANDATORY), + RFCViolation); + EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY), + RFCViolation); + - /// @todo: expand this test once support for relays is implemented } } // end of anonymous namespace