[svn] commit: r1611 - in /trunk/src/lib/dns: rdata/generic/ tests/ tests/testdata/
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Mar 19 22:54:35 UTC 2010
Author: jinmei
Date: Fri Mar 19 22:54:34 2010
New Revision: 1611
Log:
more detailed test cases for from wire construction of NSEC RDATA with
a number of bug fixes, including
- buffer overrun lookups
- incorrect boundary checks (too strict)
- other, relatively harmful, but spec-non-conformant behaviors
Added:
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire10
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire10.spec
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire5
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire5.spec
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire6
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire6.spec
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire7
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire8
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire9
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire9.spec
Modified:
trunk/src/lib/dns/rdata/generic/nsec_47.cc
trunk/src/lib/dns/tests/rdata_nsec_unittest.cc
trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire8.spec
Modified: trunk/src/lib/dns/rdata/generic/nsec_47.cc
==============================================================================
--- trunk/src/lib/dns/rdata/generic/nsec_47.cc (original)
+++ trunk/src/lib/dns/rdata/generic/nsec_47.cc Fri Mar 19 22:54:34 2010
@@ -73,7 +73,7 @@
} catch (...) {
isc_throw(InvalidRdataText, "Invalid RRtype in NSEC");
}
- } while(!iss.eof());
+ } while (!iss.eof());
for (int window = 0; window < 256; window++) {
int octet;
@@ -94,14 +94,14 @@
impl_ = new NSECImpl(Name(nextname), typebits);
}
-NSEC::NSEC(InputBuffer& buffer, size_t rdata_len)
-{
- size_t pos = buffer.getPosition();
- Name nextname(buffer);
+NSEC::NSEC(InputBuffer& buffer, size_t rdata_len) {
+ const size_t pos = buffer.getPosition();
+ const Name nextname(buffer);
// rdata_len must be sufficiently large to hold non empty bitmap.
if (rdata_len <= buffer.getPosition() - pos) {
- isc_throw(InvalidRdataLength, "NSEC too short");
+ isc_throw(DNSMessageFORMERR,
+ "NSEC RDATA from wire too short: " << rdata_len << "bytes");
}
rdata_len -= (buffer.getPosition() - pos);
@@ -109,17 +109,40 @@
buffer.readData(&typebits[0], rdata_len);
int len = 0;
- for (int i = 0; i < typebits.size(); i += len) {
- if (i + 2 > typebits.size()) {
- isc_throw(DNSMessageFORMERR, "Invalid rdata: "
- "bad NSEC type bitmap");
- }
+ bool first = true;
+ unsigned int block, lastblock = 0;
+ for (int i = 0; i < rdata_len; i += len) {
+ if (i + 2 > rdata_len) {
+ isc_throw(DNSMessageFORMERR, "NSEC RDATA from wire: "
+ "incomplete bit map filed");
+ }
+ block = typebits[i];
len = typebits[i + 1];
- if (len > 31) {
- isc_throw(DNSMessageFORMERR, "Invalid rdata: "
- "bad NSEC type bitmap");
- }
+ // Check that bitmap window blocks are in the correct order.
+ if (!first && block <= lastblock) {
+ isc_throw(DNSMessageFORMERR, "NSEC RDATA from wire: Disordered "
+ "window blocks found: " << lastblock <<
+ " then " << block);
+ }
+ // Check for legal length
+ if (len < 1 || len > 32) {
+ isc_throw(DNSMessageFORMERR, "NSEC RDATA from wire: Invalid bitmap "
+ "length: " << len);
+ }
+ // Check for overflow.
i += 2;
+ if (i + len > rdata_len) {
+ isc_throw(DNSMessageFORMERR, "NSEC RDATA from wire: bitmap length "
+ "too large: " << len);
+ }
+ // The last octet of the bitmap must be non zero.
+ if (typebits[i + len - 1] == 0) {
+ isc_throw(DNSMessageFORMERR, "NSEC RDATA from wire: bitmap ending "
+ "an all-zero byte");
+ }
+
+ lastblock = block;
+ first = false;
}
impl_ = new NSECImpl(nextname, typebits);
@@ -130,8 +153,7 @@
{}
NSEC&
-NSEC::operator=(const NSEC& source)
-{
+NSEC::operator=(const NSEC& source) {
if (impl_ == source.impl_) {
return (*this);
}
@@ -143,33 +165,37 @@
return (*this);
}
-NSEC::~NSEC()
-{
+NSEC::~NSEC() {
delete impl_;
}
string
-NSEC::toText() const
-{
+NSEC::toText() const {
ostringstream s;
int len = 0;
s << impl_->nextname_;
+
+ // In the following loop we use string::at() rather than operator[].
+ // Since the index calculation is a bit complicated, it will be safer
+ // and easier to find a bug (if any). Note that this conversion method
+ // is generally not expected to be very efficient, so the slight overhead
+ // of at() should be acceptable.
for (int i = 0; i < impl_->typebits_.size(); i += len) {
assert(i + 2 <= impl_->typebits_.size());
- int window = impl_->typebits_[i];
- len = impl_->typebits_[i + 1];
- assert(len >= 0 && len < 32);
+ const int block = impl_->typebits_.at(i);
+ len = impl_->typebits_.at(i + 1);
+ assert(len >= 0 && len <= 32);
i += 2;
for (int j = 0; j < len; j++) {
- if (impl_->typebits_[i + j] == 0) {
+ if (impl_->typebits_.at(i + j) == 0) {
continue;
}
for (int k = 0; k < 8; k++) {
- if ((impl_->typebits_[i + j] & (0x80 >> k)) == 0) {
+ if ((impl_->typebits_.at(i + j) & (0x80 >> k)) == 0) {
continue;
}
- int t = window * 256 + j * 8 + k;
- s << " " << RRType(t).toText();
+ const int t = block * 256 + j * 8 + k;
+ s << " " << RRType(t);
}
}
}
@@ -178,22 +204,19 @@
}
void
-NSEC::toWire(OutputBuffer& buffer) const
-{
+NSEC::toWire(OutputBuffer& buffer) const {
impl_->nextname_.toWire(buffer);
buffer.writeData(&impl_->typebits_[0], impl_->typebits_.size());
}
void
-NSEC::toWire(MessageRenderer& renderer) const
-{
+NSEC::toWire(MessageRenderer& renderer) const {
impl_->nextname_.toWire(renderer);
renderer.writeData(&impl_->typebits_[0], impl_->typebits_.size());
}
int
-NSEC::compare(const Rdata& other) const
-{
+NSEC::compare(const Rdata& other) const {
const NSEC& other_nsec = dynamic_cast<const NSEC&>(other);
int cmp = compareNames(impl_->nextname_, other_nsec.impl_->nextname_);
@@ -201,9 +224,9 @@
return (cmp);
}
- size_t this_len = impl_->typebits_.size();
- size_t other_len = other_nsec.impl_->typebits_.size();
- size_t cmplen = min(this_len, other_len);
+ const size_t this_len = impl_->typebits_.size();
+ const size_t other_len = other_nsec.impl_->typebits_.size();
+ const size_t cmplen = min(this_len, other_len);
cmp = memcmp(&impl_->typebits_[0], &other_nsec.impl_->typebits_[0],
cmplen);
if (cmp != 0) {
Modified: trunk/src/lib/dns/tests/rdata_nsec_unittest.cc
==============================================================================
--- trunk/src/lib/dns/tests/rdata_nsec_unittest.cc (original)
+++ trunk/src/lib/dns/tests/rdata_nsec_unittest.cc Fri Mar 19 22:54:34 2010
@@ -41,22 +41,19 @@
string nsec_txt("www2.isc.org. CNAME RRSIG NSEC");
-TEST_F(Rdata_NSEC_Test, toText_NSEC)
-{
+TEST_F(Rdata_NSEC_Test, toText_NSEC) {
const generic::NSEC rdata_nsec(nsec_txt);
EXPECT_EQ(nsec_txt, rdata_nsec.toText());
}
-TEST_F(Rdata_NSEC_Test, badText_NSEC)
-{
+TEST_F(Rdata_NSEC_Test, badText_NSEC) {
EXPECT_THROW(generic::NSEC rdata_nsec("www.isc.org. BIFF POW SPOON"),
InvalidRdataText);
EXPECT_THROW(generic::NSEC rdata_nsec("www.isc.org."),
InvalidRdataText);
}
-TEST_F(Rdata_NSEC_Test, createFromWire_NSEC)
-{
+TEST_F(Rdata_NSEC_Test, createFromWire_NSEC) {
const generic::NSEC rdata_nsec(nsec_txt);
EXPECT_EQ(0, rdata_nsec.compare(
*rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
@@ -65,22 +62,51 @@
// Too short RDLENGTH
EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
"testdata/rdata_nsec_fromWire2"),
- InvalidRdataLength);
+ DNSMessageFORMERR);
EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
"testdata/rdata_nsec_fromWire3"),
DNSMessageFORMERR);
-#if 0 // currently fails
// A malformed NSEC bitmap length field that could cause overflow.
EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
"testdata/rdata_nsec_fromWire4"),
DNSMessageFORMERR);
-#endif
+
+ // The bitmap field is incomplete (only the first byte is included)
+ EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
+ "testdata/rdata_nsec_fromWire5"),
+ DNSMessageFORMERR);
+
+ // Bitmap length is 0, which is invalid.
+ EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
+ "testdata/rdata_nsec_fromWire6"),
+ DNSMessageFORMERR);
+
+ // A boundary case: longest possible bitmaps (32 maps). This should be
+ // accepted.
+ EXPECT_NO_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
+ "testdata/rdata_nsec_fromWire7"));
+
+ // Another boundary condition: 33 bitmaps, which should be rejected.
+ EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
+ "testdata/rdata_nsec_fromWire8"),
+ DNSMessageFORMERR);
+
+ // Disordered bitmap window blocks.
+ EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
+ "testdata/rdata_nsec_fromWire9"),
+ DNSMessageFORMERR);
+
+ // Bitmap ending with all-zero bytes. Not necessarily harmful except
+ // the additional overhead of parsing, but invalid according to the
+ // spec anyway.
+ EXPECT_THROW(rdataFactoryFromFile(RRType::NSEC(), RRClass::IN(),
+ "testdata/rdata_nsec_fromWire10"),
+ DNSMessageFORMERR);
}
-TEST_F(Rdata_NSEC_Test, toWireRenderer_NSEC)
-{
+TEST_F(Rdata_NSEC_Test, toWireRenderer_NSEC) {
renderer.skip(2);
const generic::NSEC rdata_nsec(nsec_txt);
rdata_nsec.toWire(renderer);
@@ -92,14 +118,12 @@
obuffer.getLength() - 2, &data[2], data.size() - 2);
}
-TEST_F(Rdata_NSEC_Test, toWireBuffer_NSEC)
-{
+TEST_F(Rdata_NSEC_Test, toWireBuffer_NSEC) {
const generic::NSEC rdata_nsec(nsec_txt);
rdata_nsec.toWire(obuffer);
}
-TEST_F(Rdata_NSEC_Test, assign)
-{
+TEST_F(Rdata_NSEC_Test, assign) {
generic::NSEC rdata_nsec(nsec_txt);
generic::NSEC rdata_nsec2 = rdata_nsec;
EXPECT_EQ(0, rdata_nsec.compare(rdata_nsec2));
Modified: trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire8.spec
==============================================================================
--- trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire8.spec (original)
+++ trunk/src/lib/dns/tests/testdata/rdata_nsec_fromWire8.spec Fri Mar 19 22:54:34 2010
@@ -1,5 +1,5 @@
#
-# A invalid NSEC RDATA with a longest bitmap field (33 bitmap bytes)
+# An invalid NSEC RDATA with an oversized bitmap field (33 bitmap bytes)
#
[custom]
More information about the bind10-changes
mailing list