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


git-svn-id: svn://bind10.isc.org/svn/bind10/trunk@1611 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/dns/rdata/generic/nsec_47.cc

@@ -73,7 +73,7 @@ NSEC::NSEC(const string& nsec_str) :
         } 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 @@ NSEC::NSEC(const string& nsec_str) :
     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 @@ NSEC::NSEC(InputBuffer& buffer, size_t rdata_len)
     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(const NSEC& source) :
 {}
 
 NSEC&
-NSEC::operator=(const NSEC& source)
-{
+NSEC::operator=(const NSEC& source) {
     if (impl_ == source.impl_) {
         return (*this);
     }
@@ -143,33 +165,37 @@ NSEC::operator=(const NSEC& source)
     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 @@ NSEC::toText() const
 }
 
 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 @@ NSEC::compare(const Rdata& other) const
         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) {

src/lib/dns/tests/rdata_nsec_unittest.cc

@@ -41,22 +41,19 @@ class Rdata_NSEC_Test : public RdataTest {
 
 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 @@ TEST_F(Rdata_NSEC_Test, createFromWire_NSEC)
     // 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 @@ TEST_F(Rdata_NSEC_Test, toWireRenderer_NSEC)
                         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));

src/lib/dns/tests/testdata/rdata_nsec_fromWire10

@@ -0,0 +1,10 @@
+###
+### This data file was auto-generated from rdata_nsec_fromWire10.spec
+###
+
+# NSEC RDATA (RDLEN=24)
+0018
+# Next Name=next.example.com (18 bytes)
+046e657874076578616d706c6503636f6d00
+# Bitmap: Block=0, Length=4
+00 04 01000000

src/lib/dns/tests/testdata/rdata_nsec_fromWire10.spec

@@ -0,0 +1,8 @@
+#
+# An invalid NSEC RDATA: a bitmap block containing empty bytes
+#
+
+[custom]
+sections: nsec
+[nsec]
+bitmap: '01000000'

src/lib/dns/tests/testdata/rdata_nsec_fromWire5

@@ -0,0 +1,10 @@
+###
+### This data file was auto-generated from rdata_nsec_fromWire5.spec
+###
+
+# NSEC RDATA (RDLEN=19)
+0013
+# Next Name=next.example.com
+046e657874076578616d706c6503636f6d00
+# Bitmap: Block=0, Length=31
+00 1f 00

src/lib/dns/tests/testdata/rdata_nsec_fromWire5.spec

@@ -0,0 +1,13 @@
+#
+# A malformed NSEC RDATA: incomplete bit map field
+#
+
+[custom]
+sections: nsec
+[nsec]
+# only containing the block field of the bitmap
+rdlen: 19
+#dummy data
+maplen: 31
+#dummy data
+bitmap: '00'

src/lib/dns/tests/testdata/rdata_nsec_fromWire6

@@ -0,0 +1,10 @@
+###
+### This data file was auto-generated from rdata_nsec_fromWire6.spec
+###
+
+# NSEC RDATA (RDLEN=20)
+0014
+# Next Name=next.example.com
+046e657874076578616d706c6503636f6d00
+# Bitmap: Block=0, Length=0
+00 00 01

src/lib/dns/tests/testdata/rdata_nsec_fromWire6.spec

@@ -0,0 +1,11 @@
+#
+# A malformed NSEC RDATA: bit map length being 0
+#
+
+[custom]
+sections: nsec
+[nsec]
+rdlen: 20
+maplen: 0
+# dummy data:
+bitmap: '01' 

src/lib/dns/tests/testdata/rdata_nsec_fromWire7

@@ -0,0 +1,10 @@
+###
+### This data file was auto-generated from rdata_nsec_fromWire7.spec
+###
+
+# NSEC RDATA (RDLEN=52)
+0034
+# Next Name=next.example.com
+046e657874076578616d706c6503636f6d00
+# Bitmap: Block=0, Length=32
+00 20 0101010101010101010101010101010101010101010101010101010101010101

src/lib/dns/tests/testdata/rdata_nsec_fromWire8

@@ -0,0 +1,10 @@
+###
+### This data file was auto-generated from rdata_nsec_fromWire8.spec
+###
+
+# NSEC RDATA (RDLEN=53)
+0035
+# Next Name=next.example.com
+046e657874076578616d706c6503636f6d00
+# Bitmap: Block=0, Length=33
+00 21 010101010101010101010101010101010101010101010101010101010101010101

src/lib/dns/tests/testdata/rdata_nsec_fromWire8.spec

@@ -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]

src/lib/dns/tests/testdata/rdata_nsec_fromWire9

@@ -0,0 +1,12 @@
+###
+### This data file was auto-generated from rdata_nsec_fromWire9.spec
+###
+
+# NSEC RDATA (RDLEN=34)
+0022
+# Next Name=next.example.com (18 bytes)
+046e657874076578616d706c6503636f6d00
+# Bitmap: Block=2, Length=6
+02 06 040000000003
+# Bitmap: Block=1, Length=6
+01 06 040000000003

src/lib/dns/tests/testdata/rdata_nsec_fromWire9.spec

@@ -0,0 +1,10 @@
+#
+# An invalid NSEC RDATA: disordered bitmap blocks
+#
+
+[custom]
+sections: nsec
+[nsec]
+nbitmap: 2
+block0: 2
+block1: 1