From: Thomas Markwalder Date: Thu, 23 May 2019 19:07:43 +0000 (-0400) Subject: [#539, !330] OPT_STRING_TYPE options now trim trailing nulls X-Git-Tag: Kea-1.6.0-beta~79 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=81cff105d4f63a75411e451bacadc534d26bf3f4;p=thirdparty%2Fkea.git [#539, !330] OPT_STRING_TYPE options now trim trailing nulls src/lib/dhcp/option_data_types.* OptionDataTypeUtil::readString() - modified to trim off trailing nulls and to throw if the result is zero length src/lib/dhcp/option_string.* OptionString::setValue() OptionString::unpack() - modified to trim off trailing nulls and to throw if the result is zero length src/lib/dhcp/tests/option_data_types_unittest.cc src/lib/dhcp/tests/option_string_unittest.cc src/lib/dhcp/tests/pkt4_unittest.cc Udpated and/or added tests to verify behavior with permutations of nulls in inputs --- diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc index e159aa0865..a64707a9ba 100644 --- a/src/lib/dhcp/option_data_types.cc +++ b/src/lib/dhcp/option_data_types.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 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 @@ -574,8 +574,19 @@ std::string OptionDataTypeUtil::readString(const std::vector& buf) { std::string value; if (!buf.empty()) { - value.insert(value.end(), buf.begin(), buf.end()); + // Per RFC 2132, section 2 we need to drop trailing NULLs + auto begin = buf.begin(); + auto end = buf.end(); + for ( ; end != begin && *(end - 1) == 0x0; --end); + + if (std::distance(begin, end) == 0) { + isc_throw(isc::OutOfRange, "string value carried by the option " + << " contained only NULLs"); + } + + value.insert(value.end(), begin, end); } + return (value); } diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h index 358dcc0728..1408b48c64 100644 --- a/src/lib/dhcp/option_data_types.h +++ b/src/lib/dhcp/option_data_types.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 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 @@ -610,9 +610,11 @@ public: /// @brief Read string value from a buffer. /// + /// To be compliant with RFC 2132, Sec. 2, trailing NULLs are trimmed. /// @param buf input buffer. /// /// @return string value being read. + /// @throw isc::dhcp::OutOfRange is the payload contains only NULLs. static std::string readString(const std::vector& buf); /// @brief Write UTF8-encoded string into a buffer. diff --git a/src/lib/dhcp/option_string.cc b/src/lib/dhcp/option_string.cc index 1f5c12f329..2b185520a5 100644 --- a/src/lib/dhcp/option_string.cc +++ b/src/lib/dhcp/option_string.cc @@ -50,7 +50,18 @@ OptionString::setValue(const std::string& value) { << getType() << "' must not be empty"); } - setData(value.begin(), value.end()); + // Trim off any trailing NULLs. + auto begin = value.begin(); + auto end = value.end(); + for ( ; end != begin && *(end - 1) == 0x0; --end); + + if (std::distance(begin, end) == 0) { + isc_throw(isc::OutOfRange, "string value carried by the option '" + << getType() << "' contained only NULLs"); + } + + // Now set the value. + setData(begin, end); } @@ -74,11 +85,16 @@ OptionString::pack(isc::util::OutputBuffer& buf) const { void OptionString::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { + // Trim off trailing null(s) + for ( ; end != begin && *(end - 1) == 0x0; --end); + if (std::distance(begin, end) == 0) { isc_throw(isc::OutOfRange, "failed to parse an option '" << getType() << "' holding string value" - << " - empty value is not accepted"); + << "' was empty or contained only NULLs"); } + + // Now set the data. setData(begin, end); } diff --git a/src/lib/dhcp/option_string.h b/src/lib/dhcp/option_string.h index 51be5c2394..c590fe1fe2 100644 --- a/src/lib/dhcp/option_string.h +++ b/src/lib/dhcp/option_string.h @@ -20,7 +20,8 @@ namespace dhcp { /// /// This class represents an option carrying a single string value. /// Currently this class imposes that the minimal length of the carried -/// string is 1. +/// string is 1. Per RFC 2132, Sec 2 trailing NULLs are trimmed during +/// either construction or unpacking. /// /// @todo In the future this class may be extended with some more string /// content checks and encoding methods if required. @@ -32,7 +33,7 @@ public: /// This constructor creates an instance of option which carries a /// string value specified as constructor's parameter. This constructor /// is most often used to create an instance of an option which will - /// be sent in the outgoing packet. + /// be sent in the outgoing packet. Trailing NULLs will be trimmed. /// /// @param u universe (V4 or V6). /// @param type option code. @@ -46,6 +47,7 @@ public: /// /// This constructor creates an instance of the option from the provided /// chunk of buffer. This buffer may hold the data received on the wire. + /// Trailing NULLs will be trimmed. /// /// @param u universe (V4 or V6). /// @param type option code. @@ -71,6 +73,8 @@ public: /// @brief Sets the string value to be held by the option. /// + /// Trailing NULLs will be trimmed. + /// /// @param value string value to be set. /// /// @throw isc::OutOfRange if a string value to be set is empty. @@ -91,6 +95,7 @@ public: /// it does not decode the option code and length, so the iterators must /// point to the beginning and end of the option payload respectively. /// The size of the decoded payload must be at least 1 byte. + /// Trailing NULLs will be trimmed. /// /// @param begin the iterator pointing to the option payload. /// @param end the iterator pointing to the end of the option payload. diff --git a/src/lib/dhcp/tests/option_data_types_unittest.cc b/src/lib/dhcp/tests/option_data_types_unittest.cc index f90ebec723..e43fb2b13c 100644 --- a/src/lib/dhcp/tests/option_data_types_unittest.cc +++ b/src/lib/dhcp/tests/option_data_types_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 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 @@ -847,6 +847,7 @@ TEST_F(OptionDataTypesTest, writePsid) { // The purpose of this test is to verify that the string // can be read from a buffer correctly. TEST_F(OptionDataTypesTest, readString) { + // Prepare a buffer with some string in it. std::vector buf; writeString("hello world", buf); @@ -858,6 +859,34 @@ TEST_F(OptionDataTypesTest, readString) { ); // Check that it is valid. EXPECT_EQ("hello world", value); + + // Only nulls should throw. + OptionBuffer buffer = { 0, 0 }; + ASSERT_THROW(OptionDataTypeUtil::readString(buffer), isc::OutOfRange); + + // One trailing null should trim off. + buffer = {'o', 'n', 'e', 0 }; + ASSERT_NO_THROW(value = OptionDataTypeUtil::readString(buffer)); + EXPECT_EQ(3, value.length()); + EXPECT_EQ(value, std::string("one")); + + // More than one trailing null should trim off. + buffer = { 't', 'h', 'r', 'e', 'e', 0, 0, 0 }; + ASSERT_NO_THROW(value = OptionDataTypeUtil::readString(buffer)); + EXPECT_EQ(5, value.length()); + EXPECT_EQ(value, std::string("three")); + + // Embedded null should be left in place. + buffer = { 'e', 'm', 0, 'b', 'e', 'd' }; + ASSERT_NO_THROW(value = OptionDataTypeUtil::readString(buffer)); + EXPECT_EQ(6, value.length()); + EXPECT_EQ(value, (std::string{"em\0bed", 6})); + + // Leading null should be left in place. + buffer = { 0, 'l', 'e', 'a', 'd', 'i', 'n', 'g' }; + ASSERT_NO_THROW(value = OptionDataTypeUtil::readString(buffer)); + EXPECT_EQ(8, value.length()); + EXPECT_EQ(value, (std::string{"\0leading", 8})); } // The purpose of this test is to verify that a string can be diff --git a/src/lib/dhcp/tests/option_string_unittest.cc b/src/lib/dhcp/tests/option_string_unittest.cc index 26c84fade3..ae9346e1fc 100644 --- a/src/lib/dhcp/tests/option_string_unittest.cc +++ b/src/lib/dhcp/tests/option_string_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2019 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 @@ -55,6 +55,11 @@ TEST_F(OptionStringTest, constructorFromString) { // Check that an attempt to use empty string in the constructor // will result in an exception. EXPECT_THROW(OptionString(Option::V6, 123, ""), isc::OutOfRange); + + // Check that an attempt to use string containgin only nulls + // in the constructor will result in an exception. + std::string nulls{"\0\0",2}; + EXPECT_THROW(OptionString(Option::V6, 123, nulls), isc::OutOfRange); } // This test verifies that the constructor which creates an option instance @@ -69,6 +74,13 @@ TEST_F(OptionStringTest, constructorFromBuffer) { isc::OutOfRange ); + // NULLs should result in an exception. + std::vectornulls = { 0, 0, 0 }; + EXPECT_THROW( + OptionString(Option::V4, 234, nulls.begin(), nulls.begin()), + isc::OutOfRange + ); + // Declare option as a scoped pointer here so as its scope is // function wide. The initialization (constructor invocation) // is pushed to the ASSERT_NO_THROW macro below, as it may @@ -145,7 +157,7 @@ TEST_F(OptionStringTest, pack) { // And create a string from it. std::string test_string(data.begin(), data.end()); // This string should be equal to the string used to create - // option's instance. + // option's instance. EXPECT_TRUE(option_value == test_string); } @@ -163,4 +175,67 @@ TEST_F(OptionStringTest, toText) { EXPECT_EQ("type=00512, len=00016: \"is a filler text\" (string)", optv6.toText()); } +// This test checks proper handling of trailing and embedded NULLs in +// data use to create or option value. +TEST_F(OptionStringTest, setValueNullsHandling) { + OptionString optv4(Option::V4, 123, "123"); + + // Only nulls should throw. + ASSERT_THROW(optv4.setValue(std::string{"\0\0", 2}), isc::OutOfRange); + + // One trailing null should trim off. + ASSERT_NO_THROW(optv4.setValue(std::string{"one\0", 4})); + EXPECT_EQ(3, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), std::string("one")); + + // More than one trailing null should trim off. + ASSERT_NO_THROW(optv4.setValue(std::string{"three\0\0\0", 8})); + EXPECT_EQ(5, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), std::string("three")); + + // Embedded null should be left in place. + ASSERT_NO_THROW(optv4.setValue(std::string{"em\0bed", 6})); + EXPECT_EQ(6, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), (std::string{"em\0bed", 6})); + + // Leading null should be left in place. + ASSERT_NO_THROW(optv4.setValue(std::string{"\0leading", 8})); + EXPECT_EQ(8, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), (std::string{"\0leading", 8})); +} + +// This test checks proper handling of trailing and embedded NULLs in +// data use to create or option value. +TEST_F(OptionStringTest, unpackNullsHandling) { + OptionString optv4(Option::V4, 123, "123"); + + // Only nulls should throw. + OptionBuffer buffer = { 0, 0 }; + ASSERT_THROW(optv4.unpack(buffer.begin(), buffer.end()), isc::OutOfRange); + + // One trailing null should trim off. + buffer = {'o', 'n', 'e', 0 }; + ASSERT_NO_THROW(optv4.unpack(buffer.begin(), buffer.end())); + EXPECT_EQ(3, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), std::string("one")); + + // More than one trailing null should trim off. + buffer = { 't', 'h', 'r', 'e', 'e', 0, 0, 0 }; + ASSERT_NO_THROW(optv4.unpack(buffer.begin(), buffer.end())); + EXPECT_EQ(5, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), std::string("three")); + + // Embedded null should be left in place. + buffer = { 'e', 'm', 0, 'b', 'e', 'd' }; + ASSERT_NO_THROW(optv4.unpack(buffer.begin(), buffer.end())); + EXPECT_EQ(6, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), (std::string{"em\0bed", 6})); + + // Leading null should be left in place. + buffer = { 0, 'l', 'e', 'a', 'd', 'i', 'n', 'g' }; + ASSERT_NO_THROW(optv4.unpack(buffer.begin(), buffer.end())); + EXPECT_EQ(8, optv4.getValue().length()); + EXPECT_EQ(optv4.getValue(), (std::string{"\0leading", 8})); +} + } // anonymous namespace diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index c052865a37..89c6142089 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2019 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 @@ -188,11 +188,12 @@ public: boost::shared_ptr