[master] Finished merge of trac3618 (truncated packet/option/vendor-option)

ChangeLog

+962.	[func]		fdupont
+	Make the parsing of options and vendor options more consistent
+	between v4 and v6. In addition make the parsing more robust
+	against malformed packets.
+	(Trac #3618, git xxx)
+
 961.	[func]		fdupont
 	Improved error messages when handling invalid or malformed
 	configuration file. File and line number are printed, when

src/lib/dhcp/libdhcp++.h

-// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2015 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -137,48 +137,60 @@ 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
-    /// of 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 perfromance
-    /// 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.
     /// @param option_space A name of the option space which holds definitions
-    /// of to be used to parse options in the packets.
+    ///        to be used to parse options in the packets.
     /// @param options Reference to option container. Options will be
     ///        put here.
     /// @param relay_msg_offset reference to a size_t structure. If specified,
     ///        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:

src/lib/dhcp/option_vendor.cc

@@ -58,7 +58,7 @@ void OptionVendor::unpack(OptionBufferConstIter begin,
 
     vendor_id_ = isc::util::readUint32(&(*begin), distance(begin, end));
 
-    OptionBuffer vendor_buffer(begin +4, end);
+    OptionBuffer vendor_buffer(begin + 4, end);
 
     if (universe_ == Option::V6) {
         LibDHCP::unpackVendorOptions6(vendor_id_, vendor_buffer, options_);

src/lib/dhcp/pkt4.cc

@@ -225,9 +225,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
@@ -235,7 +236,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.");
     // }

src/lib/dhcp/pkt6.cc

@@ -256,7 +256,7 @@ void
 Pkt6::packTCP() {
     /// TODO Implement this function.
     isc_throw(NotImplemented, "DHCPv6 over TCP (bulk leasequery and failover)"
-              "not implemented yet.");
+              " not implemented yet.");
 }
 
 void

src/lib/dhcp/pkt_filter_inet.cc

@@ -237,7 +237,7 @@ PktFilterInet::send(const Iface&, uint16_t sockfd,
     struct in_pktinfo* pktinfo =(struct in_pktinfo *)CMSG_DATA(cmsg);
     memset(pktinfo, 0, sizeof(struct in_pktinfo));
     pktinfo->ipi_ifindex = pkt->getIndex();
-        pktinfo->ipi_spec_dst.s_addr = htonl(pkt->getLocalAddr()); // set the source IP address
+    pktinfo->ipi_spec_dst.s_addr = htonl(pkt->getLocalAddr()); // set the source IP address
     m.msg_controllen = CMSG_SPACE(sizeof(struct in_pktinfo));
 #endif
 

src/lib/dhcp/tests/libdhcp++_unittest.cc

@@ -950,11 +950,19 @@ TEST_F(LibDhcpTest, stdOptionDefs4) {
         3, 1, 2, 3  // first byte is opaque data length, the rest is opaque data
     };
     std::vector<uint8_t> 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<uint8_t> 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
@@ -983,6 +991,16 @@ TEST_F(LibDhcpTest, stdOptionDefs6) {
     // Initialize a vector with the FQDN data.
     std::vector<uint8_t> fqdn_buf(data, data + sizeof(data));
 
+    // Prepare buffer holding a vendor option
+    const char vopt_data[] = {
+        1, 2, 3, 4,                               // enterprise=0x1020304
+        0, 100,                                   // type=100
+        0, 6,                                     // length=6
+        102, 111, 111, 98, 97, 114                // data="foobar"
+    };
+    // Initialize a vector with the suboption data.
+    std::vector<uint8_t> vopt_buf(vopt_data, vopt_data + sizeof(vopt_data));
+
     // The CLIENT_FQDN holds a uint8_t value and FQDN. We have
     // to add the uint8_t value to it and then append the buffer
     // holding some valid FQDN.
@@ -1039,7 +1057,8 @@ TEST_F(LibDhcpTest, stdOptionDefs6) {
                                     vclass_buf.end(),
                                     typeid(OptionVendorClass));
 
-    LibDhcpTest::testStdOptionDefs6(D6O_VENDOR_OPTS, begin, end,
+    LibDhcpTest::testStdOptionDefs6(D6O_VENDOR_OPTS, vopt_buf.begin(),
+                                    vopt_buf.end(),
                                     typeid(OptionVendor),
                                     "vendor-opts-space");
 

src/lib/dhcp/tests/pkt4_unittest.cc

@@ -654,6 +654,111 @@ TEST_F(Pkt4Test, unpackOptions) {
     verifyParsedOptions(pkt);
 }
 
+// Checks if the code is able to handle a malformed option
+TEST_F(Pkt4Test, unpackMalformed) {
+
+    vector<uint8_t> orig = generateTestPacket2();
+
+    orig.push_back(0x63);
+    orig.push_back(0x82);
+    orig.push_back(0x53);
+    orig.push_back(0x63);
+
+    orig.push_back(53); // Message Type 
+    orig.push_back(1); // length=1
+    orig.push_back(2); // type=2
+
+    orig.push_back(12); // Hostname
+    orig.push_back(3); // length=3
+    orig.push_back(102); // data="foo"
+    orig.push_back(111);
+    orig.push_back(111);
+
+    // That's our original content. It should be sane.
+    Pkt4Ptr success(new Pkt4(&orig[0], orig.size()));
+    EXPECT_NO_THROW(success->unpack());
+
+    // With the exception of END and PAD an option must have a length byte
+    vector<uint8_t> nolength = orig;
+    nolength.resize(orig.size() - 4);
+    Pkt4Ptr no_length_pkt(new Pkt4(&nolength[0], nolength.size()));
+    EXPECT_NO_THROW(no_length_pkt->unpack());
+
+    // The unpack() operation doesn't throw but there is no option 12
+    EXPECT_FALSE(no_length_pkt->getOption(12));
+
+    // Truncated data is not accepted too but doesn't throw
+    vector<uint8_t> shorty = orig;
+    shorty.resize(orig.size() - 1);
+    Pkt4Ptr too_short_pkt(new Pkt4(&shorty[0], shorty.size()));
+    EXPECT_NO_THROW(too_short_pkt->unpack());
+
+    // The unpack() operation doesn't throw but there is no option 12
+    EXPECT_FALSE(no_length_pkt->getOption(12));
+}
+
+// Checks if the code is able to handle a malformed vendor option
+TEST_F(Pkt4Test, unpackVendorMalformed) {
+
+    vector<uint8_t> orig = generateTestPacket2();
+
+    orig.push_back(0x63);
+    orig.push_back(0x82);
+    orig.push_back(0x53);
+    orig.push_back(0x63);
+
+    orig.push_back(53); // Message Type 
+    orig.push_back(1); // length=1
+    orig.push_back(2); // type=2
+
+    orig.push_back(125); // vivso suboptions
+    size_t full_len_index = orig.size();
+    orig.push_back(15); // length=15
+    orig.push_back(1); // vendor_id=0x1020304
+    orig.push_back(2);
+    orig.push_back(3);
+    orig.push_back(4);
+    size_t data_len_index = orig.size();
+    orig.push_back(10); // data-len=10
+    orig.push_back(128); // suboption type=128
+    orig.push_back(3); // suboption length=3
+    orig.push_back(102); // data="foo"
+    orig.push_back(111);
+    orig.push_back(111);
+    orig.push_back(129); // suboption type=129
+    orig.push_back(3); // suboption length=3
+    orig.push_back(99); // data="bar"
+    orig.push_back(98);
+    orig.push_back(114);
+
+    // That's our original content. It should be sane.
+    Pkt4Ptr success(new Pkt4(&orig[0], orig.size()));
+    EXPECT_NO_THROW(success->unpack());
+
+    // Data-len must match
+    vector<uint8_t> baddatalen = orig;
+    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);
+
+    // A suboption must have a length byte
+    vector<uint8_t> nolength = orig;
+    nolength.resize(orig.size() - 4);
+    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);
+
+    // Truncated data is not accepted either
+    vector<uint8_t> shorty = orig;
+    shorty.resize(orig.size() - 1);
+    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);
+}
+
 // This test verifies that it is possible to specify custom implementation of
 // the option parsing algorithm by installing a callback function.
 TEST_F(Pkt4Test, unpackOptionsWithCallback) {

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -345,8 +345,8 @@ TEST_F(Pkt6Test, unpackMalformed) {
     Pkt6Ptr too_short_pkt(new Pkt6(&shorty[0], shorty.size()));
     EXPECT_THROW(too_short_pkt->unpack(), isc::BadValue);
 
-    // The code should complain about remaining bytes that can't
-    // be parsed.
+    // The code should complain about remaining bytes that can't be parsed
+    // but doesn't do so yet.
     Pkt6Ptr trailing_garbage(new Pkt6(&malform1[0], malform1.size()));
     EXPECT_NO_THROW(trailing_garbage->unpack());
 
@@ -369,6 +369,78 @@ 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
+TEST_F(Pkt6Test, unpackVendorMalformed) {
+    // Get a packet. We're really interested in its on-wire
+    // representation only.
+    scoped_ptr<Pkt6> donor(capture1());
+
+    // Add a vendor option
+    OptionBuffer orig = donor->data_;
+
+    orig.push_back(0); // vendor options
+    orig.push_back(17);
+    orig.push_back(0);
+    size_t len_index = orig.size();
+    orig.push_back(18); // length=18
+    orig.push_back(1); // vendor_id=0x1020304
+    orig.push_back(2);
+    orig.push_back(3);
+    orig.push_back(4);
+    orig.push_back(1); // suboption type=0x101
+    orig.push_back(1); 
+    orig.push_back(0); // suboption length=3
+    orig.push_back(3);
+    orig.push_back(102); // data="foo"
+    orig.push_back(111);
+    orig.push_back(111);
+    orig.push_back(1); // suboption type=0x102
+    orig.push_back(2);
+    orig.push_back(0); // suboption length=3
+    orig.push_back(3);
+    orig.push_back(99); // data="bar'
+    orig.push_back(98);
+    orig.push_back(114);
+
+    Pkt6Ptr success(new Pkt6(&orig[0], orig.size()));
+    EXPECT_NO_THROW(success->unpack());
+
+    // Truncated vendor option is not accepted but doesn't throw
+    vector<uint8_t> shortv = orig;
+    shortv[len_index] = 20;
+    Pkt6Ptr too_short_vendor_pkt(new Pkt6(&shortv[0], shortv.size()));
+    EXPECT_NO_THROW(too_short_vendor_pkt->unpack());
+    
+    // Truncated option header is not accepted
+    vector<uint8_t> shorth = orig;
+    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);
+
+    // Truncated option data is not accepted
+    vector<uint8_t> 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);
 }
 
 // This test verifies that it is possible to specify custom implementation of