From: Marcin Siodelski Date: Tue, 20 May 2014 17:16:08 +0000 (+0200) Subject: [3246] Set non-significant bits of parsed prefix option to 0. X-Git-Tag: trac3434_base~26^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d34b71777150c5688bb474b705f2d2badf1b3ac6;p=thirdparty%2Fkea.git [3246] Set non-significant bits of parsed prefix option to 0. --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 13ed769a1a..78c2397c84 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1365,16 +1365,16 @@ Dhcpv6Srv::assignIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, // Check if the client sent us a hint in his IA_PD. Clients may send an // address in their IA_NA options as a suggestion (e.g. the last address // they used before). - boost::shared_ptr hintOpt = + boost::shared_ptr hint_opt = boost::dynamic_pointer_cast(ia->getOption(D6O_IAPREFIX)); IOAddress hint("::"); - if (hintOpt) { - hint = hintOpt->getAddress(); + if (hint_opt) { + hint = hint_opt->getAddress(); } LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_PROCESS_IA_PD_REQUEST) .arg(duid ? duid->toText() : "(no-duid)").arg(ia->getIAID()) - .arg(hintOpt ? hint.toText() : "(no hint)"); + .arg(hint_opt ? hint.toText() : "(no hint)"); // "Fake" allocation is processing of SOLICIT message. We pretend to do an // allocation, but we do not put the lease in the database. That is ok, diff --git a/src/bin/dhcp6/tests/sarr_unittest.cc b/src/bin/dhcp6/tests/sarr_unittest.cc index 00c9d50fd8..89e2dbd20d 100644 --- a/src/bin/dhcp6/tests/sarr_unittest.cc +++ b/src/bin/dhcp6/tests/sarr_unittest.cc @@ -79,7 +79,7 @@ TEST_F(SARRTest, directClientPrefixHint) { const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6(); ASSERT_EQ(1, subnets->size()); // Append IAPREFIX option to the client's message. - ASSERT_NO_THROW(client.useHint(100, 200, 56, "2001:db8:3:33::33")); + ASSERT_NO_THROW(client.useHint(100, 200, 64, "2001:db8:3:33::33")); // Perform 4-way exchange. ASSERT_NO_THROW(client.doSARR()); // Server should have assigned a prefix. @@ -103,7 +103,7 @@ TEST_F(SARRTest, directClientPrefixHint) { client.modifyDUID(); // Use the hint with some least significant bytes set. - ASSERT_NO_THROW(client.useHint(100, 200, 56, "2001:db8:3:33::34")); + ASSERT_NO_THROW(client.useHint(100, 200, 64, "2001:db8:3:33::34")); ASSERT_NO_THROW(client.doSARR()); // Server should assign a lease. ASSERT_EQ(1, client.getLeaseNum()); diff --git a/src/lib/dhcp/option6_iaprefix.cc b/src/lib/dhcp/option6_iaprefix.cc index 0e75e6276f..43de8fe8b0 100644 --- a/src/lib/dhcp/option6_iaprefix.cc +++ b/src/lib/dhcp/option6_iaprefix.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -86,7 +86,9 @@ void Option6IAPrefix::unpack(OptionBuffer::const_iterator begin, begin += sizeof(uint8_t); // 16 bytes: IPv6 address - addr_ = IOAddress::fromBytes(AF_INET6, &(*begin)); + OptionBuffer address_with_mask; + mask(begin, begin + V6ADDRESS_LEN, prefix_len_, address_with_mask); + addr_ = IOAddress::fromBytes(AF_INET6, &(*address_with_mask.begin())); begin += V6ADDRESS_LEN; // unpack encapsulated options (the only defined so far is PD_EXCLUDE) @@ -122,5 +124,20 @@ uint16_t Option6IAPrefix::len() { return (length); } +void +Option6IAPrefix::mask(OptionBuffer::const_iterator begin, + OptionBuffer::const_iterator end, + const uint8_t len, + OptionBuffer& output_address) { + output_address.resize(16, 0); + if (len >= 128) { + std::copy(begin, end, output_address.begin()); + } else if (len > 0) { + std::copy(begin, begin + static_cast(len/8), output_address.begin()); + output_address[len/8] = (0xff << (8 - (len % 8))); + } +} + + } // end of namespace isc::dhcp } // end of namespace isc diff --git a/src/lib/dhcp/option6_iaprefix.h b/src/lib/dhcp/option6_iaprefix.h index 803bbda8b0..f80e4748c7 100644 --- a/src/lib/dhcp/option6_iaprefix.h +++ b/src/lib/dhcp/option6_iaprefix.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -33,9 +33,29 @@ namespace dhcp { /// a prefix length) 2 methods are used: getAddress() and getLength(). Although /// using getAddress() to obtain base prefix is somewhat counter-intuitive at /// first, it becomes obvious when one realizes that an address is a special -/// case of a prefix with /128. It makes everyone's like much easier, because +/// case of a prefix with /128. It makes everyone's life much easier, because /// the base prefix doubles as a regular address in many cases, e.g. when /// searching for a lease. +/// +/// When searching for a prefix in the database or simply comparing two prefixes +/// for equality, it is important that only the significant parts of the +/// prefixes are compared. It is possible that the client or a server sends a +/// prefix which has non-significant bits (beyond prefix length) set. The +/// server or client receiving such a prefix should be liberal and not discard +/// this prefix. It should rather ignore the non-significant bits. Therefore +/// the unpack() function, which parses the prefix from the wire, always sets +/// the non-significant bits to 0 so as two prefixes received on the wire can +/// be compared without additional processing. +/// +/// @todo Currently, the constructor which creates the option from the textual +/// format doesn't set non-significant bits to 0. This is because it is assumed +/// that the prefixes from the string are created locally (not received over the +/// wire) and should be validated before the option is created. If we wanted +/// to set non-significant bits to 0 when the prefix is created from the textual +/// format it would have some peformance implications, because the option would +/// need to be turned into wire format, appropriate bits set to 0 and then +/// option would need to be created again from the wire format. We may consider +/// doing it if we find a use case where it is required. class Option6IAPrefix : public Option6IAAddr { public: @@ -100,7 +120,21 @@ public: /// returns data length (data length + DHCPv4/DHCPv6 option header) virtual uint16_t len(); -protected: +private: + + /// @brief Apply mask of the specific length to the IPv6 address. + /// + /// @param begin Iterator pointing to the buffer holding IPv6 address. + /// @param end Iterator pointing to the end of the buffer holding IPv6 + /// address. + /// @param len Length of the mask to be applied. + /// @param [out] output_address Reference to the buffer where the address + /// with a mask applied is output. + void mask(OptionBuffer::const_iterator begin, + OptionBuffer::const_iterator end, + const uint8_t len, + OptionBuffer& output_address); + uint8_t prefix_len_; }; diff --git a/src/lib/dhcp/tests/option6_iaprefix_unittest.cc b/src/lib/dhcp/tests/option6_iaprefix_unittest.cc index e5dd05e9d1..319d4dc072 100644 --- a/src/lib/dhcp/tests/option6_iaprefix_unittest.cc +++ b/src/lib/dhcp/tests/option6_iaprefix_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2014 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 @@ -36,7 +36,7 @@ using namespace isc::asiolink; namespace { class Option6IAPrefixTest : public ::testing::Test { public: - Option6IAPrefixTest() : buf_(255), outBuf_(255) { + Option6IAPrefixTest() : buf_(255), out_buf_(255) { for (int i = 0; i < 255; i++) { buf_[i] = 255 - i; } @@ -45,7 +45,7 @@ public: /// @brief creates on-wire representation of IAPREFIX option /// /// buf_ field is set up to have IAPREFIX with preferred=1000, - /// valid=3000000000 and prefix beign 2001:db8:1::dead:beef/77 + /// valid=3000000000 and prefix being 2001:db8:1:0:ffff:0:dead:beef/77 void setExampleBuffer() { for (int i = 0; i < 255; i++) { buf_[i] = 0; @@ -69,10 +69,12 @@ public: buf_[12] = 0xb8; buf_[13] = 0x00; buf_[14] = 0x01; + buf_[17] = 0xff; + buf_[18] = 0xff; buf_[21] = 0xde; buf_[22] = 0xad; buf_[23] = 0xbe; - buf_[24] = 0xef; // 2001:db8:1::dead:beef + buf_[24] = 0xef; // 2001:db8:1:0:ffff:0:dead:beef } @@ -82,15 +84,22 @@ public: /// /// @param opt IAPREFIX option being tested /// @param expected_type expected option type - void checkOption(Option6IAPrefix& opt, uint16_t expected_type) { + /// @param expected_length Expected length of the prefix. + /// @param expected_address Expected prefix value in the textual format. + void checkOption(Option6IAPrefix& opt, const uint16_t expected_type, + const uint8_t expected_length, + const std::string& expected_address) { // Check if all fields have expected values EXPECT_EQ(Option::V6, opt.getUniverse()); EXPECT_EQ(expected_type, opt.getType()); - EXPECT_EQ("2001:db8:1::dead:beef", opt.getAddress().toText()); + EXPECT_EQ(expected_address, opt.getAddress().toText()); EXPECT_EQ(1000, opt.getPreferred()); EXPECT_EQ(3000000000U, opt.getValid()); - EXPECT_EQ(77, opt.getLength()); + // uint8_t is often represented as a character type (char). Convert it + // to integer so as it is logged as a numeric value instead. + EXPECT_EQ(static_cast(expected_length), + static_cast(opt.getLength())); // 4 bytes header + 25 bytes content EXPECT_EQ(Option::OPTION6_HDR_LEN + Option6IAPrefix::OPTION6_IAPREFIX_LEN, @@ -105,7 +114,7 @@ public: /// @param expected_type expected option type void checkOutputBuffer(uint16_t expected_type) { // Check if pack worked properly: - const uint8_t* out = (const uint8_t*)outBuf_.getData(); + const uint8_t* out = static_cast(out_buf_.getData()); // - if option type is correct EXPECT_EQ(expected_type, out[0]*256 + out[1]); @@ -118,11 +127,12 @@ public: } OptionBuffer buf_; - OutputBuffer outBuf_; + OutputBuffer out_buf_; }; -// Tests if receiving option can be parsed correctly -TEST_F(Option6IAPrefixTest, basic) { +// Tests if a received option is parsed correctly. For the prefix length between +// 0 and 128 the non-significant bits should be set to 0. +TEST_F(Option6IAPrefixTest, parseShort) { setExampleBuffer(); @@ -133,17 +143,75 @@ TEST_F(Option6IAPrefixTest, basic) { ASSERT_TRUE(opt); // Pack this option - opt->pack(outBuf_); - EXPECT_EQ(29, outBuf_.getLength()); + opt->pack(out_buf_); + EXPECT_EQ(29, out_buf_.getLength()); - checkOption(*opt, D6O_IAPREFIX); + // The non-significant bits (above 77) of the received prefix should be + // set to zero. + checkOption(*opt, D6O_IAPREFIX, 77, "2001:db8:1:0:fff8::"); + // Set non-significant bits in the reference buffer to 0, so as the buffer + // can be directly compared with the option buffer. + buf_[18] = 0xf8; + buf_.insert(buf_.begin() + 19, 5, 0); checkOutputBuffer(D6O_IAPREFIX); // Check that option can be disposed safely EXPECT_NO_THROW(opt.reset()); } +// Tests if a received option holding prefix of 128 bits is parsed correctly. +TEST_F(Option6IAPrefixTest, parseLong) { + + setExampleBuffer(); + // Set prefix length to the maximal value. + buf_[8] = 128; + + // Create an option (unpack content) + boost::scoped_ptr opt; + ASSERT_NO_THROW(opt.reset(new Option6IAPrefix(D6O_IAPREFIX, buf_.begin(), + buf_.begin() + 25))); + ASSERT_TRUE(opt); + + // Pack this option + opt->pack(out_buf_); + EXPECT_EQ(29, out_buf_.getLength()); + + checkOption(*opt, D6O_IAPREFIX, 128, "2001:db8:1:0:ffff:0:dead:beef"); + + checkOutputBuffer(D6O_IAPREFIX); + + // Check that option can be disposed safely + EXPECT_NO_THROW(opt.reset()); +} + +// Check that the prefix having length of zero is represented as a "::". +TEST_F(Option6IAPrefixTest, parseZero) { + setExampleBuffer(); + // Set prefix length to 0. + buf_[8] = 0; + + // Create an option (unpack content) + boost::scoped_ptr opt; + ASSERT_NO_THROW(opt.reset(new Option6IAPrefix(D6O_IAPREFIX, buf_.begin(), + buf_.begin() + 25))); + ASSERT_TRUE(opt); + + // Pack this option + opt->pack(out_buf_); + EXPECT_EQ(29, out_buf_.getLength()); + + checkOption(*opt, D6O_IAPREFIX, 0, "::"); + + // Fill the address in the reference buffer with zeros. + buf_.insert(buf_.begin() + 9, 16, 0); + checkOutputBuffer(D6O_IAPREFIX); + + // Check that option can be disposed safely + EXPECT_NO_THROW(opt.reset()); +} + + // Checks whether a new option can be built correctly TEST_F(Option6IAPrefixTest, build) { @@ -151,14 +219,15 @@ TEST_F(Option6IAPrefixTest, build) { setExampleBuffer(); ASSERT_NO_THROW(opt.reset(new Option6IAPrefix(12345, - IOAddress("2001:db8:1::dead:beef"), 77, 1000, 3000000000u))); + IOAddress("2001:db8:1:0:ffff:0:dead:beef"), 77, + 1000, 3000000000u))); ASSERT_TRUE(opt); - checkOption(*opt, 12345); + checkOption(*opt, 12345, 77, "2001:db8:1:0:ffff:0:dead:beef"); // Check if we can build it properly - EXPECT_NO_THROW(opt->pack(outBuf_)); - EXPECT_EQ(29, outBuf_.getLength()); + EXPECT_NO_THROW(opt->pack(out_buf_)); + EXPECT_EQ(29, out_buf_.getLength()); checkOutputBuffer(12345); // Check that option can be disposed safely @@ -181,7 +250,8 @@ TEST_F(Option6IAPrefixTest, negative) { BadValue); // Prefix length can't be larger than 128 - EXPECT_THROW(Option6IAPrefix(12345, IOAddress("192.0.2.1"), 255, 1000, 2000), + EXPECT_THROW(Option6IAPrefix(12345, IOAddress("192.0.2.1"), + 255, 1000, 2000), BadValue); }