From: Thomas Markwalder Date: Tue, 6 Mar 2018 15:00:45 +0000 (-0500) Subject: [5551] Addressed review comments X-Git-Tag: trac5530a_base~3^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b791aaefdf28777b64b5042609d081b05e3365d8;p=thirdparty%2Fkea.git [5551] Addressed review comments Added commentary, updated sub-options unpacking, and added support for skip logic to kea-dhcp6. src/lib/dhcp/option.h Added detail to SkipRemainingOptionsError commentary src/lib/dhcp/libdhcp++.cc LibDHCP::unpackVendorOptions4() LibDHCP::unpackVendorOptions6() src/lib/dhcp/tests/pkt4_unittest.cc TEST_F(Pkt4Test, unpackVendorMalformed) - updated for new exception type TEST_F(Pkt4Test, truncatedVendorLength) - removed extraneous byte streams src/lib/dhcp/tests/pkt6_unittest.cc TEST_F(Pkt6Test, unpackVendorMalformed) - updated for new exception type TEST_F(Pkt6Test, truncatedVendorLength) - new test for truncated vendor options src/lib/dhcp/tests/pkt_captures.h src/lib/dhcp/tests/pkt_captures.cc isc::dhcp::Pkt6Ptr captureSolicitWithVIVSO() isc::dhcp::Pkt6Ptr captureSolicitWithTruncatedVIVSO() - new captured packets src/bin/dhcp6/dhcp6_messages.mes DHCP6_PACKET_OPTIONS_SKIPPED - new log message src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::processPacket() - added explicit catch of SkipRemainingOptionsError to allow processing the packet src/bin/dhcp6/tests/dhcp6_srv_unittest.cc TEST_F(Dhcpv6SrvTest, truncatedVIVSO) - new test to verify server's ability to handle SkipRemainingOptionsError --- diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index d69bf81398..d636e77c49 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -452,6 +452,11 @@ because packets of this type must be sent to multicast. The first argument specifies the client and transaction identification information, the second argument specifies packet type. +% DHCP6_PACKET_OPTIONS_SKIPPED An error upacking an option, caused subsequent options to be skipped: %1 +A debug message issued when an option failed to unpack correctly, making it +impossible to unpack the remaining options in the packet. The server will +server will still attempt to service the packet. + % DHCP6_PACKET_PROCESS_EXCEPTION exception occurred during packet processing This error message indicates that a non-standard exception was raised during packet processing that was not caught by other, more specific diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 9bf8219311..5127234d14 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -596,6 +596,12 @@ Dhcpv6Srv::processPacket(Pkt6Ptr& query, Pkt6Ptr& rsp) { .arg(query->getLocalAddr().toText()) .arg(query->getIface()); query->unpack(); + } catch (const SkipRemainingOptionsError& e) { + // An option failed to unpack but we are to attempt to process it + // anyway. Log it and let's hope for the best. + LOG_DEBUG(options6_logger, DBG_DHCP6_DETAIL, + DHCP6_PACKET_OPTIONS_SKIPPED) + .arg(e.what()); } catch (const std::exception &e) { // Failed to parse the packet. LOG_DEBUG(bad_packet6_logger, DBG_DHCP6_DETAIL, diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index d6cac08161..5571a96a2f 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2018 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 @@ -2512,6 +2512,28 @@ TEST_F(Dhcpv6SrvTest, userContext) { EXPECT_EQ("{ \"type\": \"prefixes\" }", pools[0]->getContext()->str()); } +// Verifies that the server will still process a packet which fails to +// parse with a SkipRemainingOptions exception +TEST_F(Dhcpv6SrvTest, truncatedVIVSO) { + NakedDhcpv6Srv srv(0); + + // Let's create a SOLICIT with a VIVSO whose length is too short + Pkt6Ptr sol = PktCaptures::captureSolicitWithTruncatedVIVSO(); + + // Simulate that we have received that traffic + srv.fakeReceive(sol); + + // Server will now process to run its normal loop, but instead of calling + // IfaceMgr::receive6(), it will read all packets from the list set by + // fakeReceive() + srv.run(); + + // Make sure we got an Advertise... + ASSERT_FALSE(srv.fake_sent_.empty()); + Pkt6Ptr adv = srv.fake_sent_.front(); + ASSERT_TRUE(adv); +} + /// @todo: Add more negative tests for processX(), e.g. extend sanityCheck() test /// to call processX() methods. diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index da85930362..330c02b140 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2018 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 @@ -607,7 +607,7 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id, // a two-byte type code and a two-byte length field. while (offset < length) { if (offset + 4 > length) { - isc_throw(OutOfRange, + isc_throw(SkipRemainingOptionsError, "Vendor option parse failed: truncated header"); } @@ -618,9 +618,10 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id, offset += 2; if (offset + opt_len > length) { - isc_throw(OutOfRange, "Vendor option parse failed. Tried to parse " - << offset + opt_len << " bytes from " << length - << "-byte long buffer."); + isc_throw(SkipRemainingOptionsError, + "Vendor option parse failed. Tried to parse " + << offset + opt_len << " bytes from " << length + << "-byte long buffer."); } OptionPtr opt; @@ -703,7 +704,8 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe if (offset + data_len > buf.size()) { // The option is truncated. - isc_throw(OutOfRange, "Attempt to parse truncated vendor option"); + isc_throw(SkipRemainingOptionsError, + "Attempt to parse truncated vendor option"); } uint8_t offset_end = offset + data_len; @@ -718,14 +720,15 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe // opt_type must be cast to integer so as it is not // treated as unsigned char value (a number is // presented in error message). - isc_throw(OutOfRange, + isc_throw(SkipRemainingOptionsError, "Attempt to parse truncated vendor option " << static_cast(opt_type)); } uint8_t opt_len = buf[offset++]; if (offset + opt_len > offset_end) { - isc_throw(OutOfRange, "Option parse failed. Tried to parse " + isc_throw(SkipRemainingOptionsError, + "Option parse failed. Tried to parse " << offset + opt_len << " bytes from " << buf.size() << "-byte long buffer."); } diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h index 6afe4205bd..992ac0fb5c 100644 --- a/src/lib/dhcp/option.h +++ b/src/lib/dhcp/option.h @@ -43,6 +43,12 @@ typedef std::multimap OptionCollection; typedef boost::shared_ptr OptionCollectionPtr; /// @brief Exception thrown during option unpacking +/// This exception is thrown when an error has occurred, unpacking +/// an option from a packet and we wish to abandon any any further +/// unpacking efforts and allow the server to attempt to process +/// the packet as it stands. In other words, the option that failed +/// is perhaps optional, and rather than drop the packet as unusable +/// we wish to attempt to proces it. class SkipRemainingOptionsError : public Exception { public: SkipRemainingOptionsError (const char* file, size_t line, const char* what) : diff --git a/src/lib/dhcp/option_vendor.h b/src/lib/dhcp/option_vendor.h index c566eae218..655bcbd19b 100644 --- a/src/lib/dhcp/option_vendor.h +++ b/src/lib/dhcp/option_vendor.h @@ -67,8 +67,10 @@ public: /// @param begin iterator to first byte of option data /// @param end iterator to end of option data (first byte after option end) /// - /// @throw isc::SkipRemainingOptionsBuffer if provided buffer is - /// shorter than data size. + /// @throw isc::SkipRemainingOptionsBuffer if an error is encountered + /// unpacking the option. This exception is thrown to indicate to the + /// caller that a: remaining options cannot be parsed and b: the packet + /// should be considered for processing anyway. virtual void unpack(OptionBufferConstIter begin, OptionBufferConstIter end); /// @brief Sets enterprise identifier diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index a20c8aa939..970f4ef367 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -763,7 +763,7 @@ TEST_F(Pkt4Test, unpackVendorMalformed) { baddatalen.resize(orig.size() - 5); baddatalen[full_len_index] = 10; Pkt4Ptr bad_data_len_pkt(new Pkt4(&baddatalen[0], baddatalen.size())); - EXPECT_THROW(bad_data_len_pkt->unpack(), InvalidOptionValue); + EXPECT_THROW(bad_data_len_pkt->unpack(), SkipRemainingOptionsError); // A suboption must have a length byte vector nolength = orig; @@ -771,7 +771,7 @@ TEST_F(Pkt4Test, unpackVendorMalformed) { nolength[full_len_index] = 11; nolength[data_len_index] = 6; Pkt4Ptr no_length_pkt(new Pkt4(&nolength[0], nolength.size())); - EXPECT_THROW(no_length_pkt->unpack(), InvalidOptionValue); + EXPECT_THROW(no_length_pkt->unpack(), SkipRemainingOptionsError); // Truncated data is not accepted either vector shorty = orig; @@ -779,7 +779,7 @@ TEST_F(Pkt4Test, unpackVendorMalformed) { shorty[full_len_index] = 14; shorty[data_len_index] = 9; Pkt4Ptr too_short_pkt(new Pkt4(&shorty[0], shorty.size())); - EXPECT_THROW(too_short_pkt->unpack(), InvalidOptionValue); + EXPECT_THROW(too_short_pkt->unpack(), SkipRemainingOptionsError); } // This test verifies methods that are used for manipulating meta fields @@ -1138,46 +1138,6 @@ TEST_F(Pkt4Test, getType) { // short (i.e. less than sizeof(uint8_t), unpack throws a // SkipRemainingOptionsError exception TEST_F(Pkt4Test, truncatedVendorLength) { - // Raw data for a valid DISCOVER packet. Its option 125 has a valid length - // of 133 (x85). Look for 303a7d: next two digits = 85 - const char* good_discover = - "010106012d5d43cb000080000000000000000000000000000ace50017896845ef7af0" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000063825363350" - "10137070102030407067d3c0a646f63736973332e303a7d850000118b80010102057b" - "01010102010303010104010105010106010107010f0801100901030a01010b01180c0" - "1010d0201000e0201000f010110040000000211010113010114010015013f16010117" - "01011801041901041a01041b01201c01021d01081e01201f011020011021010222010" - "1230100240100250101260200ff2701012801d82b7c020345434d030b45434d3a4552" - "4f5554455208030020400418333936373739343234343335353037373031303134303" - "035050131061e534247365838322d382e362e302e302d47412d30312d3937312d4e4f" - "5348070432343030090a534247363738322d41430a144d6f746f726f6c6120436f727" - "06f726174696f6e3d0fff845ef7af000300017896845ef7af390205dc521b01048005" - "03f802067896845ef7af090b0000118b06010401020300ff"; - - // Same DISCOVER as above but with the option 125 length changed to 01. - const char* bad_discover = - "010106012d5d43cb000080000000000000000000000000000ace50017896845ef7af0" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000000000000000" - "000000000000000000000000000000000000000000000000000000000063825363350" - "10137070102030407067d3c0a646f63736973332e303a7d010000118b80010102057b" - "01010102010303010104010105010106010107010f0801100901030a01010b01180c0" - "1010d0201000e0201000f010110040000000211010113010114010015013f16010117" - "01011801041901041a01041b01201c01021d01081e01201f011020011021010222010" - "1230100240100250101260200ff2701012801d82b7c020345434d030b45434d3a4552" - "4f5554455208030020400418333936373739343234343335353037373031303134303" - "035050131061e534247365838322d382e362e302e302d47412d30312d3937312d4e4f" - "5348070432343030090a534247363738322d41430a144d6f746f726f6c6120436f727" - "06f726174696f6e3d0fff845ef7af000300017896845ef7af390205dc521b01048005" - "03f802067896845ef7af090b0000118b06010401020300ff"; // Build a good discover packet Pkt4Ptr pkt = test::PktCaptures::discoverWithValidVIVSO(); diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc index f45b937c81..8289bee262 100644 --- a/src/lib/dhcp/tests/pkt6_unittest.cc +++ b/src/lib/dhcp/tests/pkt6_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2018 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 @@ -410,14 +410,14 @@ TEST_F(Pkt6Test, unpackVendorMalformed) { shorth.resize(orig.size() - 4); shorth[len_index] = 12; Pkt6Ptr too_short_header_pkt(new Pkt6(&shorth[0], shorth.size())); - EXPECT_THROW(too_short_header_pkt->unpack(), OutOfRange); + EXPECT_THROW(too_short_header_pkt->unpack(), SkipRemainingOptionsError); // Truncated option data is not accepted vector shorto = orig; shorto.resize(orig.size() - 2); shorto[len_index] = 16; Pkt6Ptr too_short_option_pkt(new Pkt6(&shorto[0], shorto.size())); - EXPECT_THROW(too_short_option_pkt->unpack(), OutOfRange); + EXPECT_THROW(too_short_option_pkt->unpack(), SkipRemainingOptionsError); } // This test verifies that options can be added (addOption()), retrieved @@ -1694,4 +1694,36 @@ TEST_F(Pkt6Test, getLabelEmptyClientId) { EXPECT_EQ("duid=[no info], tid=0x2312", pkt.getLabel()); } +// Verifies that when the VIVSO, 17, has length that is too +// short (i.e. less than sizeof(uint8_t), unpack throws a +// SkipRemainingOptionsError exception +TEST_F(Pkt6Test, truncatedVendorLength) { + + // Build a good discover packet + Pkt6Ptr pkt = test::PktCaptures::captureSolicitWithVIVSO(); + + // Unpacking should not throw + ASSERT_NO_THROW(pkt->unpack()); + ASSERT_EQ(DHCPV6_SOLICIT, pkt->getType()); + + // VIVSO option should be there + OptionPtr x = pkt->getOption(D6O_VENDOR_OPTS); + ASSERT_TRUE(x); + ASSERT_EQ(D6O_VENDOR_OPTS, x->getType()); + OptionVendorPtr vivso = boost::dynamic_pointer_cast(x); + ASSERT_TRUE(vivso); + EXPECT_EQ(8, vivso->len()); // data + opt code + len + + // Build a bad discover packet + pkt = test::PktCaptures::captureSolicitWithTruncatedVIVSO(); + + // Unpack should throw Skip exception + ASSERT_THROW(pkt->unpack(), SkipRemainingOptionsError); + ASSERT_EQ(DHCPV6_SOLICIT, pkt->getType()); + + // VIVSO option should not be there + x = pkt->getOption(D6O_VENDOR_OPTS); + ASSERT_FALSE(x); +} + } diff --git a/src/lib/dhcp/tests/pkt_captures.h b/src/lib/dhcp/tests/pkt_captures.h index 9b280cd383..6fdf6f9dff 100644 --- a/src/lib/dhcp/tests/pkt_captures.h +++ b/src/lib/dhcp/tests/pkt_captures.h @@ -1,7 +1,6 @@ // Copyright (C) 2014-2018 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 +// 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 PKT_CAPTURES_H @@ -56,6 +55,8 @@ public: static isc::dhcp::Pkt6Ptr captureeRouterRelayedSolicit(); static isc::dhcp::Pkt6Ptr captureCableLabsShortVendorClass(); static isc::dhcp::Pkt6Ptr captureRelayed2xRSOO(); + static isc::dhcp::Pkt6Ptr captureSolicitWithVIVSO(); + static isc::dhcp::Pkt6Ptr captureSolicitWithTruncatedVIVSO(); protected: /// @brief Auxiliary method that sets Pkt6 fields diff --git a/src/lib/dhcp/tests/pkt_captures6.cc b/src/lib/dhcp/tests/pkt_captures6.cc index f086f4b6ce..d1fa4165b6 100644 --- a/src/lib/dhcp/tests/pkt_captures6.cc +++ b/src/lib/dhcp/tests/pkt_captures6.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2018 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 @@ -392,6 +392,118 @@ isc::dhcp::Pkt6Ptr PktCaptures::captureRelayed2xRSOO() { return (pkt); } +isc::dhcp::Pkt6Ptr PktCaptures::captureSolicitWithVIVSO() { + + // Message type: Solicit (1) + // Transaction ID: 0xba048e + // Client Identifier + // Option: Client Identifier (1) + // Length: 10 + // Value: 0003000108002725d3f4 + // DUID: 0003000108002725d3f4 + // DUID Type: link-layer address (3) + // Hardware type: Ethernet (1) + // Link-layer address: 08:00:27:25:d3:f4 + // Identity Association for Non-temporary Address + // Option: Identity Association for Non-temporary Address (3) + // Length: 40 + // Value: 00aabbcc0000000000000000000500180000000000000000... + // IAID: 00aabbcc + // T1: 0 + // T2: 0 + // IA Address + // Option: IA Address (5) + // Length: 24 + // Value: 000000000000000000000000000000000000000000000000 + // IPv6 address: :: + // Preferred lifetime: 0 + // Valid lifetime: + // Option Request + // Option: Option Request (6) + // Length: 6 + // Value: 00d100d2000c + // Vendor-specific Information + // Option: Vendor-specific Information (17) + // Length: 4 + // Value: 00001e61 + // Enterprise ID: E-DYNAMICS.ORG (7777) + string hex_string = + "01ba048e0001000a0003000108002725d3f40003002800aabbcc" + "00000000000000000005001800000000000000000000000000000" + "00000000000000000000006000600d100d2000c0011000400001e61"; + + std::vector bin; + + // Decode the hex string and store it in bin (which happens + // to be OptionBuffer format) + isc::util::encode::decodeHex(hex_string, bin); + + Pkt6Ptr pkt(new Pkt6(&bin[0], bin.size())); + pkt->setRemotePort(547); + pkt->setRemoteAddr(IOAddress("fe80::1234")); + pkt->setLocalPort(547); + pkt->setLocalAddr(IOAddress("ff05::1:3")); + pkt->setIndex(2); + pkt->setIface("eth0"); + return (pkt); +} + +isc::dhcp::Pkt6Ptr PktCaptures::captureSolicitWithTruncatedVIVSO() { + + // Message type: Solicit (1) + // Transaction ID: 0xba048e + // Client Identifier + // Option: Client Identifier (1) + // Length: 10 + // Value: 0003000108002725d3f4 + // DUID: 0003000108002725d3f4 + // DUID Type: link-layer address (3) + // Hardware type: Ethernet (1) + // Link-layer address: 08:00:27:25:d3:f4 + // Identity Association for Non-temporary Address + // Option: Identity Association for Non-temporary Address (3) + // Length: 40 + // Value: 00aabbcc0000000000000000000500180000000000000000... + // IAID: 00aabbcc + // T1: 0 + // T2: 0 + // IA Address + // Option: IA Address (5) + // Length: 24 + // Value: 000000000000000000000000000000000000000000000000 + // IPv6 address: :: + // Preferred lifetime: 0 + // Valid lifetime: + // Option Request + // Option: Option Request (6) + // Length: 6 + // Value: 00d100d2000c + // Vendor-specific Information + // Option: Vendor-specific Information (17) + // Length: 1 <-------- length too short! + // Value: 00001e61 + // Enterprise ID: E-DYNAMICS.ORG (7777) + string hex_string = + "01ba048e0001000a0003000108002725d3f40003002800aabbcc" + "00000000000000000005001800000000000000000000000000000" + "00000000000000000000006000600d100d2000c0011000100001e61"; + + std::vector bin; + + // Decode the hex string and store it in bin (which happens + // to be OptionBuffer format) + isc::util::encode::decodeHex(hex_string, bin); + + Pkt6Ptr pkt(new Pkt6(&bin[0], bin.size())); + pkt->setRemotePort(547); + pkt->setRemoteAddr(IOAddress("fe80::1234")); + pkt->setLocalPort(547); + pkt->setLocalAddr(IOAddress("ff05::1:3")); + pkt->setIndex(2); + pkt->setIface("eth0"); + return (pkt); +} + }; // end of isc::dhcp::test namespace }; // end of isc::dhcp namespace }; // end of isc namespace