From: JINMEI Tatuya Date: Thu, 23 Aug 2012 20:10:01 +0000 (-0700) Subject: [2100] prohibit overriding original data on LabelSequence::serialize. X-Git-Tag: trac2351_base~115^2~3^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=48ff20698455bb03fadef08daa7c1176534d7e41;p=thirdparty%2Fkea.git [2100] prohibit overriding original data on LabelSequence::serialize. added a test case for it and updated the doc. rbtree/domaintree was modified to avoid such operations. --- diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index 33efc88082..2c4bd5910a 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -1648,8 +1648,12 @@ DomainTree::insert(util::MemorySegment& mem_sgmt, isc::dns::LabelSequence target_labels(target_name); int order = -1; + // For possible LabelSequence serialization we always store labels data + // in the separate local buffer. + uint8_t labels_buf[dns::LabelSequence::MAX_SERIALIZED_LENGTH]; while (current != NULL) { - const dns::LabelSequence current_labels(current->getLabels()); + const dns::LabelSequence current_labels( + dns::LabelSequence(current->getLabels(), labels_buf)); const isc::dns::NameComparisonResult compare_result = target_labels.compare(current_labels); const isc::dns::NameComparisonResult::NameRelation relation = diff --git a/src/lib/datasrc/rbtree.h b/src/lib/datasrc/rbtree.h index 2ec53474e2..eb971e80c5 100644 --- a/src/lib/datasrc/rbtree.h +++ b/src/lib/datasrc/rbtree.h @@ -1622,8 +1622,12 @@ RBTree::insert(util::MemorySegment& mem_sgmt, isc::dns::LabelSequence target_labels(target_name); int order = -1; + // For possible LabelSequence serialization we always store labels data + // in the separate local buffer. + uint8_t labels_buf[dns::LabelSequence::MAX_SERIALIZED_LENGTH]; while (current != NULL) { - const dns::LabelSequence current_labels(current->getLabels()); + const dns::LabelSequence current_labels( + dns::LabelSequence(current->getLabels(), labels_buf)); const isc::dns::NameComparisonResult compare_result = target_labels.compare(current_labels); const isc::dns::NameComparisonResult::NameRelation relation = diff --git a/src/lib/dns/labelsequence.cc b/src/lib/dns/labelsequence.cc index ba85b91808..ed23f269c5 100644 --- a/src/lib/dns/labelsequence.cc +++ b/src/lib/dns/labelsequence.cc @@ -91,6 +91,19 @@ LabelSequence::getSerializedLength() const { return (1 + getLabelCount() + getDataLength()); } +namespace { +// Check if buf is not in the range of [bp, ep), which means +// - end of buffer is before bp, or +// - beginning of buffer is on or after ep +bool +isOutOfRange(const uint8_t* bp, const uint8_t* ep, + const uint8_t* buf, size_t buf_len) +{ + return (bp >= buf + buf_len || // end of buffer is before bp + ep <= buf); // beginning of buffer is on or after ep +} +} + void LabelSequence::serialize(void* buf, size_t buf_len) const { const size_t expected_size = getSerializedLength(); @@ -101,12 +114,19 @@ LabelSequence::serialize(void* buf, size_t buf_len) const { const size_t offsets_len = getLabelCount(); assert(offsets_len < 256); // should be in the 8-bit range + // Overridden check. Buffer shouldn't overwrap the offset of name data + // regions. uint8_t* bp = reinterpret_cast(buf); + const size_t ndata_len = getDataLength(); + if (!isOutOfRange(offsets_, offsets_ + offsets_len, bp, buf_len) || + !isOutOfRange(data_, data_ + ndata_len, bp, buf_len)) { + isc_throw(BadValue, "serialize would break the source sequence"); + } + *bp++ = offsets_len; for (size_t i = 0; i < offsets_len; ++i) { *bp++ = offsets_[first_label_ + i] - offsets_[first_label_]; } - const size_t ndata_len = getDataLength(); std::memcpy(bp, &data_[offsets_[first_label_]], ndata_len); bp += ndata_len; diff --git a/src/lib/dns/labelsequence.h b/src/lib/dns/labelsequence.h index 186bda6ff8..caadd90314 100644 --- a/src/lib/dns/labelsequence.h +++ b/src/lib/dns/labelsequence.h @@ -179,6 +179,34 @@ public: /// the value returned by getSerializedLength() (it can be larger than /// that). /// + /// Be careful about where the buffer is located; due to the nature + /// of the buffer, it's quite possible that the memory region is being used + /// to construct another active \c LabelSequence. In such a case + /// the serialization would silently break that sequence object, and + /// it will be very difficult to identify the cause. This method + /// has minimal level checks to avoid such disruption: If the serialization + /// would break "this" \c LabelSequence object, it doesn't write anything + /// to the given buffer and throw a \c isc::BadValue exception. + /// + /// In general, it should be safe to call this method on a + /// \c LabelSequence object constructed from a \c Name object or + /// a copy of such \c LabelSequence. When you construct \c LabelSequence + /// from pre-serialized data, calling this method on it can be unsafe. + /// One safe (but a bit less efficient) way in such a case is to make + /// the source \c LabelSequence temporary and immediately create a + /// local copy using an explicit buffer, and call this method on the + /// latter: + /// \code + /// // don't do this, it's not safe (and would result in exception): + /// // LabelSequence(buf).serialize(buf, buf_len); + /// + /// // The following are the safe way: + /// uint8_t ext_buf[LabelSequence::MAX_SERIALIZED_LENGTH]; + /// LabelSequence seq(LabelSequence(buf), ext_buf); + /// ... (strip the labels, etc) + /// seq.serialize(buf, buf_len); // it's safe to override buf here + /// \endcode + /// /// The serialized image would be as follows: /// - olen: number of offsets (1 byte) /// - binary sequence of offsets (olen bytes, verbatim copy of offsets_ @@ -186,13 +214,14 @@ public: /// - binary sequence of name data (length determined by itself, verbatim /// copy of data_ of the corresponding size) /// - /// Applications must use the resulting image opaque value and must not + /// Applications must use the resulting image as opaque value and must not /// use it for other purposes than input to the corresponding constructor /// to restore it. Application behavior that assumes the specific /// organization of the image is not guaranteed. /// /// \throw isc::BadValue buf_len is too short (this method never throws - /// otherwise) + /// otherwise) or the serialization would override internal data of + /// of the source LabelSequence. /// /// \param buf Pointer to the placeholder to dump the serialized image /// \param buf_len The size of available region in \c buf diff --git a/src/lib/dns/tests/labelsequence_unittest.cc b/src/lib/dns/tests/labelsequence_unittest.cc index e8e2846d7e..2ce1ef7382 100644 --- a/src/lib/dns/tests/labelsequence_unittest.cc +++ b/src/lib/dns/tests/labelsequence_unittest.cc @@ -712,8 +712,9 @@ TEST_F(LabelSequenceTest, LeftShiftOperator) { } TEST_F(LabelSequenceTest, serialize) { - // placeholder for serialized data - uint8_t labels_buf[LabelSequence::MAX_SERIALIZED_LENGTH]; + // placeholder for serialized data. We use a sufficiently large space + // for testing the overwrapping cases below. + uint8_t labels_buf[LabelSequence::MAX_SERIALIZED_LENGTH * 3]; // vector to store expected and actual data vector actual_labelseqs; @@ -777,6 +778,32 @@ TEST_F(LabelSequenceTest, serialize) { EXPECT_EQ(NameComparisonResult::EQUAL, LabelSequence(labels_buf).compare(*itl).getRelation()); + + // Shift the data to the middle of the buffer for overwrap check + uint8_t* const bp = labels_buf; + std::memcpy(bp + serialized_len, bp, serialized_len); + // Memory layout is now as follows: + // <- ser_len -> <- ser_len ------> + // bp bp+ser_len bp+(ser_len*2) + // olen,odata,ndata + + // end of buffer would be the first byte of offsets: invalid. + EXPECT_THROW(LabelSequence(bp + serialized_len). + serialize(bp + 2, serialized_len), + isc::BadValue); + // begin of buffer would be the last byte of ndata: invalid. + EXPECT_THROW(LabelSequence(bp + serialized_len). + serialize(bp + (2 * serialized_len) - 1, serialized_len), + isc::BadValue); + // A boundary safe case: buffer is placed after the sequence data. + // should cause no disruption. + LabelSequence(bp + serialized_len). + serialize(bp + 2 * serialized_len, serialized_len); + // A boundary safe case: buffer is placed before the sequence data + // should cause no disruption. (but the original serialized data will + // be overridden, so it can't be used any more) + LabelSequence(bp + serialized_len). + serialize(bp + 1, serialized_len); } EXPECT_THROW(ls1.serialize(labels_buf, ls1.getSerializedLength() - 1),