From: Razvan Becheriu Date: Fri, 4 Jun 2021 13:01:59 +0000 (+0300) Subject: [#1858] always check if psid is valid according to psid_len X-Git-Tag: Kea-1.9.9~116 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=63249ec632bd3c045581f8d0133eeb6d20897062;p=thirdparty%2Fkea.git [#1858] always check if psid is valid according to psid_len --- diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc index 6f1bd04300..a274d3fd52 100644 --- a/src/lib/dhcp/option_data_types.cc +++ b/src/lib/dhcp/option_data_types.cc @@ -522,15 +522,13 @@ OptionDataTypeUtil::readPsid(const std::vector& buf) { // We need to check that the PSID value does not exceed the maximum value // for a specified PSID length. That means that all bits placed further than - // psid_len from the left must be set to 0. So, we create a bit mask - // by shifting a value of 0xFFFF to the left and right by psid_len. This - // leaves us with psid_len leftmost bits unset and the rest set. Next, we - // apply the mask on the PSID value from the buffer and make sure the result - // is 0. Otherwise, it means that there are some bits set in the PSID which - // aren't supposed to be set. - if ((psid_len > 0) && - ((psid & static_cast(static_cast(0xFFFF << psid_len) - >> psid_len)) != 0)) { + // psid_len from the left must be set to 0. So, we create a bit mask by + // shifting a value of 0xFFFF to the right by psid_len. This leaves us with + // psid_len leftmost bits unset and the rest set. Next, we apply the mask on + // the PSID value from the buffer and make sure the result is 0. Otherwise, + // it means that there are some bits set in the PSID which aren't supposed + // to be set. + if ((psid_len > 0) && ((psid & (0xFFFFU >> psid_len)) != 0)) { isc_throw(BadDataTypeCast, "invalid PSID value " << psid << " for a specified PSID length " << static_cast(psid_len)); @@ -538,11 +536,11 @@ OptionDataTypeUtil::readPsid(const std::vector& buf) { // All is good, so we can convert the PSID value read from the buffer to // the port set number. - if (psid_len == sizeof(psid) * 8) { + if (psid_len == 0) { // Shift by 16 always gives zero (CID 1398333) psid = 0; } else { - psid = psid >> (sizeof(psid) * 8 - psid_len); + psid >>= (sizeof(psid) * 8 - psid_len); } return (std::make_pair(PSIDLen(psid_len), PSID(psid))); } @@ -556,7 +554,7 @@ OptionDataTypeUtil::writePsid(const PSIDLen& psid_len, const PSID& psid, << ", this value is expected to be in range of 0 to 16"); } - if (psid_len.asUint8() > 0 && + if ((psid_len.asUint8() > 0) && (psid.asUint16() > (0xFFFF >> (sizeof(uint16_t) * 8 - psid_len.asUint8())))) { isc_throw(BadDataTypeCast, "invalid PSID value " << psid.asUint16() << " for a specified PSID length " @@ -570,7 +568,6 @@ OptionDataTypeUtil::writePsid(const PSIDLen& psid_len, const PSID& psid, &buf[buf.size() - 2], 2); } - std::string OptionDataTypeUtil::readString(const std::vector& buf) { std::string value; diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc index 33bdc5e094..ffbc565f1d 100644 --- a/src/lib/dhcp/tests/option_custom_unittest.cc +++ b/src/lib/dhcp/tests/option_custom_unittest.cc @@ -679,7 +679,7 @@ TEST_F(OptionCustomTest, psidData) { // The PSID comprises a PSID length and PSID value. EXPECT_EQ(4, psid.first.asUnsigned()); - EXPECT_EQ(0x08, psid.second.asUint16()); + EXPECT_EQ(0x8, psid.second.asUint16()); // Parsed option should have one suboption. EXPECT_TRUE(hasV6Suboption(option.get())); @@ -1058,17 +1058,17 @@ TEST_F(OptionCustomTest, psidDataArray) { ASSERT_NO_THROW(psid1 = option->readPsid(1)); ASSERT_NO_THROW(psid2 = option->readPsid(2)); - // PSID value is equal to '1000b' (8). + // PSID value is equal to '1000b' (0x8). EXPECT_EQ(4, psid0.first.asUnsigned()); - EXPECT_EQ(0x08, psid0.second.asUint16()); + EXPECT_EQ(0x8, psid0.second.asUint16()); - // PSID value is equal to '110101b' (0x35) + // PSID value is equal to '110101b' (0x35). EXPECT_EQ(6, psid1.first.asUnsigned()); EXPECT_EQ(0x35, psid1.second.asUint16()); - // PSID value is equal to '1b' (1). + // PSID value is equal to '1b' (0x1). EXPECT_EQ(1, psid2.first.asUnsigned()); - EXPECT_EQ(0x01, psid2.second.asUint16()); + EXPECT_EQ(0x1, psid2.second.asUint16()); } // The purpose of this test is to verify that the data from a buffer @@ -1236,10 +1236,10 @@ TEST_F(OptionCustomTest, recordData) { writeAddress(IOAddress("192.168.0.1"), buf); // Initialize field 4 to IPv6 address. writeAddress(IOAddress("2001:db8:1::1"), buf); - // Initialize PSID len and PSID value. + // Initialize field 5 PSID len and PSID value. writeInt(6, buf); writeInt(0xD400, buf); - // Initialize field 5 to string value. + // Initialize field 6 to string value. writeString("ABCD", buf); boost::scoped_ptr option; @@ -1321,7 +1321,7 @@ TEST_F(OptionCustomTest, recordArrayData) { writeAddress(IOAddress("192.168.0.1"), buf); // Initialize field 4 to IPv6 address. writeAddress(IOAddress("2001:db8:1::1"), buf); - // Initialize PSID len and PSID value. + // Initialize field 5 PSID len and PSID value. writeInt(6, buf); writeInt(0xD400, buf); // Initialize last field 6 to a pair of int 12345678 and 87654321. @@ -1615,7 +1615,7 @@ TEST_F(OptionCustomTest, setPsidData) { PSIDTuple psid; ASSERT_NO_THROW(psid = option->readPsid()); EXPECT_EQ(0, psid.first.asUnsigned()); - EXPECT_EQ(0, psid.second.asUint16()); + EXPECT_EQ(0x0, psid.second.asUint16()); // Write PSID. ASSERT_NO_THROW(option->writePsid(PSIDLen(4), PSID(8))); @@ -1623,7 +1623,7 @@ TEST_F(OptionCustomTest, setPsidData) { // Read PSID back and make sure it is the one we just set. ASSERT_NO_THROW(psid = option->readPsid()); EXPECT_EQ(4, psid.first.asUnsigned()); - EXPECT_EQ(8, psid.second.asUint16()); + EXPECT_EQ(0x8, psid.second.asUint16()); } // The purpose of this test is to verify that an option comprising @@ -1861,19 +1861,19 @@ TEST_F(OptionCustomTest, setPSIDPrefixArray) { ASSERT_NO_THROW({ PSIDTuple psid0 = option->readPsid(0); EXPECT_EQ(4, psid0.first.asUnsigned()); - EXPECT_EQ(1, psid0.second.asUint16()); + EXPECT_EQ(0x1, psid0.second.asUint16()); }); ASSERT_NO_THROW({ PSIDTuple psid1 = option->readPsid(1); EXPECT_EQ(0, psid1.first.asUnsigned()); - EXPECT_EQ(0, psid1.second.asUint16()); + EXPECT_EQ(0x0, psid1.second.asUint16()); }); ASSERT_NO_THROW({ PSIDTuple psid2 = option->readPsid(2); EXPECT_EQ(1, psid2.first.asUnsigned()); - EXPECT_EQ(1, psid2.second.asUint16()); + EXPECT_EQ(0x1, psid2.second.asUint16()); }); } @@ -2060,7 +2060,7 @@ TEST_F(OptionCustomTest, setRecordData) { PSIDTuple value5; ASSERT_NO_THROW(value5 = option->readPsid(5)); EXPECT_EQ(0, value5.first.asUnsigned()); - EXPECT_EQ(0, value5.second.asUint16()); + EXPECT_EQ(0x0, value5.second.asUint16()); PrefixTuple value6(ZERO_PREFIX_TUPLE); ASSERT_NO_THROW(value6 = option->readPrefix(6)); EXPECT_EQ(0, value6.first.asUnsigned()); @@ -2097,7 +2097,7 @@ TEST_F(OptionCustomTest, setRecordData) { EXPECT_EQ("2001:db8:1::100", value4.toText()); ASSERT_NO_THROW(value5 = option->readPsid(5)); EXPECT_EQ(4, value5.first.asUnsigned()); - EXPECT_EQ(8, value5.second.asUint16()); + EXPECT_EQ(0x8, value5.second.asUint16()); ASSERT_NO_THROW(value6 = option->readPrefix(6)); EXPECT_EQ(48, value6.first.asUnsigned()); EXPECT_EQ("2001:db8:1::", value6.second.toText()); @@ -2151,7 +2151,7 @@ TEST_F(OptionCustomTest, setRecordArrayData) { PSIDTuple value5; ASSERT_NO_THROW(value5 = option->readPsid(5)); EXPECT_EQ(0, value5.first.asUnsigned()); - EXPECT_EQ(0, value5.second.asUint16()); + EXPECT_EQ(0x0, value5.second.asUint16()); PrefixTuple value6(ZERO_PREFIX_TUPLE); ASSERT_NO_THROW(value6 = option->readPrefix(6)); EXPECT_EQ(0, value6.first.asUnsigned()); @@ -2191,7 +2191,7 @@ TEST_F(OptionCustomTest, setRecordArrayData) { EXPECT_EQ("2001:db8:1::100", value4.toText()); ASSERT_NO_THROW(value5 = option->readPsid(5)); EXPECT_EQ(4, value5.first.asUnsigned()); - EXPECT_EQ(8, value5.second.asUint16()); + EXPECT_EQ(0x8, value5.second.asUint16()); ASSERT_NO_THROW(value6 = option->readPrefix(6)); EXPECT_EQ(48, value6.first.asUnsigned()); EXPECT_EQ("2001:db8:1::", value6.second.toText()); diff --git a/src/lib/dhcp/tests/option_data_types_unittest.cc b/src/lib/dhcp/tests/option_data_types_unittest.cc index 3c0ef5c0e2..58bac71a32 100644 --- a/src/lib/dhcp/tests/option_data_types_unittest.cc +++ b/src/lib/dhcp/tests/option_data_types_unittest.cc @@ -750,6 +750,18 @@ TEST_F(OptionDataTypesTest, readPsid) { buf.clear(); + // PSID length is 16 (bits) + writeInt(16, buf); + // 0xF000 is represented as 1111000000000000b, which is equivalent + // of portset 0xF000. + writeInt(0xF000, buf); + + ASSERT_NO_THROW(psid = OptionDataTypeUtil::readPsid(buf)); + EXPECT_EQ(16, psid.first.asUnsigned()); + EXPECT_EQ(0xF000, psid.second.asUint16()); + + buf.clear(); + // PSID length is 0, in which case PSID should be ignored. writeInt(0, buf); // Let's put some junk into the PSID field to make sure it will