From: Francis Dupont Date: Thu, 18 Jun 2015 16:13:06 +0000 (+0200) Subject: [3618] addressed comments: fix docs, improved unpack{,Vendor}Options[46], fixed/impro... X-Git-Tag: trac3910_base~11^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a6f27a7e9f17dd192c0f6e59bd3bd2a35f623480;p=thirdparty%2Fkea.git [3618] addressed comments: fix docs, improved unpack{,Vendor}Options[46], fixed/improved tests --- diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index 1036ea03cf..7211c2c61d 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -249,6 +249,7 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, size_t* relay_msg_len /* = 0 */) { size_t offset = 0; size_t length = buf.size(); + size_t last_offset = 0; // Get the list of standard option definitions. OptionDefContainer option_defs; @@ -265,7 +266,17 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, // The buffer being read comprises a set of options, each starting with // a two-byte type code and a two-byte length field. - while (offset + 4 <= length) { + while (offset < length) { + // Save the current offset for backtracking + last_offset = offset; + + // Check if there is room for another option + if (offset + 4 > length) { + // Still something but smaller than an option + return (last_offset); + } + + // Parse the option header uint16_t opt_type = isc::util::readUint16(&buf[offset], 2); offset += 2; @@ -276,12 +287,12 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, // We peeked at the option header of the next option, but // discovered that it would end up beyond buffer end, so // the option is truncated. Hence we can't parse - // it. Therefore we revert back by those four bytes (as if + // it. Therefore we revert back by those bytes (as if // we never parsed them). // // @note it is the responsability of the caller to throw // an exception on partial parsing - return (offset - 4); + return (last_offset); } if (opt_type == D6O_RELAY_MSG && relay_msg_offset && relay_msg_len) { @@ -299,7 +310,7 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, // Truncated vendor-option. We expect at least // 4 bytes for the enterprise-id field. Let's roll back // option code + option length (4 bytes) and return. - return (offset - 4); + return (last_offset); } // Parse this as vendor option @@ -359,6 +370,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, const std::string& option_space, isc::dhcp::OptionCollection& options) { size_t offset = 0; + size_t last_offset = 0; // Get the list of standard option definitions. OptionDefContainer option_defs; @@ -375,12 +387,20 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, // The buffer being read comprises a set of options, each starting with // a one-byte type code and a one-byte length field. - while (offset + 1 <= buf.size()) { + while (offset < buf.size()) { + // Save the current offset for backtracking + last_offset = offset; + + // Get the option type uint8_t opt_type = buf[offset++]; // DHO_END is a special, one octet long option - if (opt_type == DHO_END) - return (offset); // just return. Don't need to add DHO_END option + if (opt_type == DHO_END) { + // just return. Don't need to add DHO_END option + // Don't return offset because it makes this condition + // and partial parsing impossible to recognize. + return (last_offset); + } // DHO_PAD is just a padding after DHO_END. Let's continue parsing // in case we receive a message without DHO_END. @@ -391,12 +411,11 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, // We peeked at the option header of the next option, but // discovered that it would end up beyond buffer end, so // the option is truncated. Hence we can't parse - // it. Therefore we revert back by that byte (as if - // we never parsed it). + // it. Therefore we revert back (as if we never parsed it). // // @note it is the responsability of the caller to throw // an exception on partial parsing - return (offset - 1); + return (last_offset); } uint8_t opt_len = buf[offset++]; @@ -404,9 +423,8 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, // We peeked at the option header of the next option, but // discovered that it would end up beyond buffer end, so // the option is truncated. Hence we can't parse - // it. Therefore we revert back by two bytes (as if we - // never parsed them). - return (offset - 2); + // it. Therefore we revert back (as if we never parsed it). + return (last_offset); } // Get all definitions with the particular option code. Note @@ -470,7 +488,8 @@ 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, "Option parse failed: truncated header"); + isc_throw(OutOfRange, + "Vendor option parse failed: truncated header"); } uint16_t opt_type = isc::util::readUint16(&buf[offset], 2); @@ -480,7 +499,7 @@ size_t LibDHCP::unpackVendorOptions6(const uint32_t vendor_id, offset += 2; if (offset + opt_len > length) { - isc_throw(OutOfRange, "Option parse failed. Tried to parse " + isc_throw(OutOfRange, "Vendor option parse failed. Tried to parse " << offset + opt_len << " bytes from " << length << "-byte long buffer."); } @@ -558,7 +577,7 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe // The buffer being read comprises a set of options, each starting with // a one-byte type code and a one-byte length field. - while (offset + 1 <= buf.size()) { + while (offset < buf.size()) { // Note that Vendor-Specific info option (RFC3925) has a // different option format than Vendor-Spec info for // DHCPv6. (there's additional layer of data-length) @@ -572,12 +591,12 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe uint8_t offset_end = offset + data_len; // beginning of data-chunk parser - while (offset + 1 <= offset_end) { + while (offset < offset_end) { uint8_t opt_type = buf[offset++]; // No DHO_END or DHO_PAD in vendor options - if (offset + 1 > buf.size()) { + if (offset + 1 > offset_end) { // opt_type must be cast to integer so as it is not // treated as unsigned char value (a number is // presented in error message). @@ -587,7 +606,7 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe } uint8_t opt_len = buf[offset++]; - if (offset + opt_len > buf.size()) { + if (offset + opt_len > offset_end) { isc_throw(OutOfRange, "Option parse failed. Tried to parse " << offset + opt_len << " bytes from " << buf.size() << "-byte long buffer."); @@ -640,6 +659,7 @@ size_t LibDHCP::unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffe } // end of data-chunk + break; // end of the vendor block. } return (offset); } diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index 3876fac513..380e0aeacb 100644 --- a/src/lib/dhcp/libdhcp++.h +++ b/src/lib/dhcp/libdhcp++.h @@ -137,30 +137,17 @@ public: static void packOptions(isc::util::OutputBuffer& buf, const isc::dhcp::OptionCollection& options); - /// @brief Parses provided buffer as DHCPv4 options and creates Option objects. - /// - /// Parses provided buffer and stores created Option objects - /// in options container. - /// - /// @param buf Buffer to be parsed. - /// @param option_space A name of the option space which holds definitions - /// to be used to parse options in the packets. - /// @param options Reference to option container. Options will be - /// put here. - /// @return offset to the first byte after the last successfully parsed option - static size_t unpackOptions4(const OptionBuffer& buf, - const std::string& option_space, - isc::dhcp::OptionCollection& options); - - /// @brief Parses provided buffer as DHCPv6 options and creates Option objects. - /// - /// Parses provided buffer and stores created Option objects in options - /// container. The last two parameters are optional and are used in - /// relay parsing. If they are specified, relay-msg option is not created, - /// but rather those two parameters are specified to point out where - /// the relay-msg option resides and what is its length. This is a performance - /// optimization that avoids unnecessary copying of potentially large - /// relay-msg option. It is not used for anything, except in the next + /// @brief Parses provided buffer as DHCPv6 options and creates + /// Option objects. + /// + /// Parses provided buffer and stores created Option objects in + /// options container. The last two parameters are optional and + /// are used in relay parsing. If they are specified, relay-msg + /// option is not created, but rather those two parameters are + /// specified to point out where the relay-msg option resides and + /// what is its length. This is a performance optimization that + /// avoids unnecessary copying of potentially large relay-msg + /// option. It is not used for anything, except in the next /// iteration its content will be treated as buffer to be parsed. /// /// @param buf Buffer to be parsed. @@ -172,13 +159,38 @@ public: /// offset to beginning of relay_msg option will be stored in it. /// @param relay_msg_len reference to a size_t structure. If specified, /// length of the relay_msg option will be stored in it. - /// @return offset to the first byte after the last successfully parsed option + /// @return offset to the first byte after the last successfully + /// parsed option + /// + /// @note This function throws when an option type is defined more + /// than once, and it calls option building routines which can throw. + /// Partial parsing does not throw: it is the responsibility of the + /// caller to handle this condition. static size_t unpackOptions6(const OptionBuffer& buf, const std::string& option_space, isc::dhcp::OptionCollection& options, size_t* relay_msg_offset = 0, size_t* relay_msg_len = 0); + /// @brief Parses provided buffer as DHCPv4 options and creates + /// Option objects. + /// + /// Parses provided buffer and stores created Option objects + /// in options container. + /// + /// @param buf Buffer to be parsed. + /// @param option_space A name of the option space which holds definitions + /// to be used to parse options in the packets. + /// @param options Reference to option container. Options will be + /// put here. + /// @return offset to the first byte after the last successfully + /// parsed option or the offset of the DHO_END option type. + /// + /// The unpackOptions6 note applies too. + static size_t unpackOptions4(const OptionBuffer& buf, + const std::string& option_space, + isc::dhcp::OptionCollection& options); + /// Registers factory method that produces options of specific option types. /// /// @throw isc::BadValue if provided the type is already registered, has @@ -215,9 +227,13 @@ public: /// /// @param vendor_id enterprise-id of the vendor /// @param buf Buffer to be parsed. - /// @param options Reference to option container. Options will be + /// @param options Reference to option container. Suboptions will be /// put here. - /// @return offset to the first byte after the last successfully parsed option + /// @return offset to the first byte after the last successfully + /// parsed suboption + /// + /// @note unpackVendorOptions6 throws when it fails to parse a suboption + /// so the return value is currently always the buffer length. static size_t unpackVendorOptions6(const uint32_t vendor_id, const OptionBuffer& buf, isc::dhcp::OptionCollection& options); @@ -230,10 +246,14 @@ public: /// /// @param vendor_id enterprise-id of the vendor /// @param buf Buffer to be parsed. - /// @param options Reference to option container. Options will be + /// @param options Reference to option container. Suboptions will be /// put here. - /// @return offset to the first byte after the last successfully parsed option - static size_t unpackVendorOptions4(const uint32_t vendor_id, const OptionBuffer& buf, + /// @return offset to the first byte after the last successfully + /// parsed suboption + /// + /// The unpackVendorOptions6 note applies + static size_t unpackVendorOptions4(const uint32_t vendor_id, + const OptionBuffer& buf, isc::dhcp::OptionCollection& options); private: diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc index 5435a2dad8..8ac3b73202 100644 --- a/src/lib/dhcp/pkt4.cc +++ b/src/lib/dhcp/pkt4.cc @@ -223,9 +223,10 @@ Pkt4::unpack() { offset = callback_(opts_buffer, "dhcp4", options_, NULL, NULL); } - // If offset is not equal to the size, then something is wrong here. We - // either parsed past input buffer (bug in our code) or we haven't parsed - // everything (received trailing garbage or truncated option). + // If offset is not equal to the size and there is no DHO_END, + // then something is wrong here. We either parsed past input + // buffer (bug in our code) or we haven't parsed everything + // (received trailing garbage or truncated option). // // Invoking Jon Postel's law here: be conservative in what you send, and be // liberal in what you accept. There's no easy way to log something from @@ -233,7 +234,7 @@ Pkt4::unpack() { // bytes. We also need to quell compiler warning about unused offset // variable. // - // if (offset != size) { + // if ((offset != size) && (opts_buffer[offset] != DHO_END)) { // isc_throw(BadValue, "Received DHCPv6 buffer of size " << size // << ", were able to parse " << offset << " bytes."); // } diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 627a79d4ad..19b57457b5 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -949,11 +949,19 @@ TEST_F(LibDhcpTest, stdOptionDefs4) { 3, 1, 2, 3 // first byte is opaque data length, the rest is opaque data }; std::vector vivco_buf(vivco_data, vivco_data + sizeof(vivco_data)); + const char vivsio_data[] = { + 1, 2, 3, 4, // enterprise id + 4, // first byte is vendor block length + 1, 2, 3, 4 // option type=1 length=2 + }; + std::vector vivsio_buf(vivsio_data, vivsio_data + sizeof(vivsio_data)); + LibDhcpTest::testStdOptionDefs4(DHO_VIVCO_SUBOPTIONS, vivco_buf.begin(), vivco_buf.end(), typeid(OptionVendorClass)); - LibDhcpTest::testStdOptionDefs4(DHO_VIVSO_SUBOPTIONS, begin, end, - typeid(OptionVendor)); + + LibDhcpTest::testStdOptionDefs4(DHO_VIVSO_SUBOPTIONS, vivsio_buf.begin(), + vivsio_buf.end(), typeid(OptionVendor)); } // Test that definitions of standard options have been initialized diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index 255156f9bc..a17e33653c 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -735,7 +735,7 @@ TEST_F(Pkt4Test, unpackVendorMalformed) { Pkt4Ptr success(new Pkt4(&orig[0], orig.size())); EXPECT_NO_THROW(success->unpack()); - // Data-len must match but it doesn't throw + // Data-len must match vector baddatalen = orig; baddatalen.resize(orig.size() - 5); baddatalen[full_len_index] = 10; diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc index 56d759daaf..0fc0059d24 100755 --- a/src/lib/dhcp/tests/pkt6_unittest.cc +++ b/src/lib/dhcp/tests/pkt6_unittest.cc @@ -376,6 +376,21 @@ TEST_F(Pkt6Test, unpackMalformed) { // ... but there should be no option 123 as it was malformed. EXPECT_FALSE(trunc_option->getOption(123)); + + // Check with truncated length field + Pkt6Ptr trunc_length(new Pkt6(&malform2[0], malform2.size() - 1)); + EXPECT_NO_THROW(trunc_length->unpack()); + EXPECT_FALSE(trunc_length->getOption(123)); + + // Check with missing length field + Pkt6Ptr no_length(new Pkt6(&malform2[0], malform2.size() - 2)); + EXPECT_NO_THROW(no_length->unpack()); + EXPECT_FALSE(no_length->getOption(123)); + + // Check with truncated type field + Pkt6Ptr trunc_type(new Pkt6(&malform2[0], malform2.size() - 3)); + EXPECT_NO_THROW(trunc_type->unpack()); + EXPECT_FALSE(trunc_type->getOption(123)); } // Checks if the code is able to handle a malformed vendor option