From 8d8b9bcd5a6975b3d05dcaa9465ef2cbd6a74049 Mon Sep 17 00:00:00 2001 From: Dan Theisen Date: Fri, 5 Nov 2021 06:03:12 -0700 Subject: [PATCH 1/2] [#2021] Allow 0 length OpaqueDataTuple to be pack()ed --- src/lib/dhcp/opaque_data_tuple.cc | 5 +-- .../dhcp/tests/opaque_data_tuple_unittest.cc | 42 +++++++++++++++---- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/lib/dhcp/opaque_data_tuple.cc b/src/lib/dhcp/opaque_data_tuple.cc index 724a466107..a8a5750df3 100644 --- a/src/lib/dhcp/opaque_data_tuple.cc +++ b/src/lib/dhcp/opaque_data_tuple.cc @@ -51,10 +51,7 @@ OpaqueDataTuple::getText() const { void OpaqueDataTuple::pack(isc::util::OutputBuffer& buf) const { - if (getLength() == 0) { - isc_throw(OpaqueDataTupleError, "failed to create on-wire format of the" - " opaque data field, because the field appears to be empty"); - } else if ((1 << (getDataFieldSize() * 8)) <= getLength()) { + if ((1 << (getDataFieldSize() * 8)) <= getLength()) { isc_throw(OpaqueDataTupleError, "failed to create on-wire format of the" " opaque data field, because current data length " << getLength() << " exceeds the maximum size for the length" diff --git a/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc b/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc index fa3fa0f1ed..974a0bf27b 100644 --- a/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc +++ b/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc @@ -270,17 +270,22 @@ TEST(OpaqueDataTuple, pack1Byte) { OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_1_BYTE); // Initially, the tuple should be empty. ASSERT_EQ(0, tuple.getLength()); - // The empty data doesn't make much sense, so the pack() should not - // allow it. + // It turns out that Option 124 can be sent with 0 length Opaque Data + // See #2021 for more details OutputBuffer out_buf(10); - EXPECT_THROW(tuple.pack(out_buf), OpaqueDataTupleError); + ASSERT_NO_THROW(tuple.pack(out_buf)); + ASSERT_EQ(1, out_buf.getLength()); + const uint8_t* zero_len = static_cast(out_buf.getData()); + ASSERT_EQ(0, *zero_len); + // Reset the output buffer for another test. + out_buf.clear(); // Set the data for tuple. std::vector data; for (uint8_t i = 0; i < 100; ++i) { data.push_back(i); } tuple.assign(data.begin(), data.size()); - // The pack should now succeed. + // Packing the data should succeed. ASSERT_NO_THROW(tuple.pack(out_buf)); // The rendered buffer should be 101 bytes long - 1 byte for length, // 100 bytes for the actual data. @@ -329,17 +334,22 @@ TEST(OpaqueDataTuple, pack2Bytes) { OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES); // Initially, the tuple should be empty. ASSERT_EQ(0, tuple.getLength()); - // The empty data doesn't make much sense, so the pack() should not - // allow it. + // It turns out that Option 124 can be sent with 0 length Opaque Data + // See #2021 for more details OutputBuffer out_buf(10); - EXPECT_THROW(tuple.pack(out_buf), OpaqueDataTupleError); + ASSERT_NO_THROW(tuple.pack(out_buf)); + ASSERT_EQ(2, out_buf.getLength()); + const uint16_t* zero_len = static_cast(out_buf.getData()); + ASSERT_EQ(0, *zero_len); + // Reset the output buffer for another test. + out_buf.clear(); // Set the data for tuple. std::vector data; for (unsigned i = 0; i < 512; ++i) { data.push_back(i & 0xff); } tuple.assign(data.begin(), data.size()); - // The pack should now succeed. + // Packing the data should succeed. ASSERT_NO_THROW(tuple.pack(out_buf)); // The rendered buffer should be 514 bytes long - 2 bytes for length, // 512 bytes for the actual data. @@ -403,6 +413,22 @@ TEST(OpaqueDataTuple, unpack1ByteZeroLength) { EXPECT_EQ(0, tuple.getLength()); } +// This test verifies that the tuple having a length of 0, followed by no +// data, is decoded from the wire format. +TEST(OpaqueDataTuple, unpack1ByteZeroLengthNoData) { + OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_1_BYTE); + OpaqueDataTuple::Buffer wire_data = {0}; + ASSERT_NO_THROW(tuple.unpack(wire_data.begin(), wire_data.end())); +} + +// This test verifies that the tuple having a length of 0, followed by no +// data, is decoded from the wire format. +TEST(OpaqueDataTuple, unpack2ByteZeroLengthNoData) { + OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES); + OpaqueDataTuple::Buffer wire_data = {0, 0}; + ASSERT_NO_THROW(tuple.unpack(wire_data.begin(), wire_data.end())); +} + // This test verifies that exception is thrown if the empty buffer is being // parsed. TEST(OpaqueDataTuple, unpack1ByteEmptyBuffer) { -- GitLab From f48a69a354f6b71cb9c73b57ea228d70132a353b Mon Sep 17 00:00:00 2001 From: Andrei Pavel Date: Fri, 19 Nov 2021 20:32:22 +0200 Subject: [PATCH 2/2] [#2021] add ChangeLog entry --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 1de5f6b11b..44f83ef93e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +1965. [func] djt + Allow Kea to pack opaque data tuples within options with zero + length to accommodate some DHCP clients who have been observed + to send DHCPv4 option 124 with zero length tuples. + (Gitlab #2021) + 1964. [func] andrei Increase the value that "maxsize" can take from 2GB to 2PB. (Gitlab #2130) -- GitLab