[2185] Don't allow empty TLSA certificate association data

2 files changed, 46 insertions, 90 deletions

diff --git a/src/lib/dns/rdata/generic/tlsa_52.cc b/src/lib/dns/rdata/generic/tlsa_52.cc
index a896e8d47f..774bdef6cf 100644
--- a/src/lib/dns/rdata/generic/tlsa_52.cc
+++ b/src/lib/dns/rdata/generic/tlsa_52.cc
@@ -89,16 +89,16 @@ TLSA::constructFromLexer(MasterLexer& lexer) {
     }
     lexer.ungetToken();
 
+    if (certificate_assoc_data.empty()) {
+        isc_throw(InvalidRdataText, "Empty TLSA certificate association data");
+    }
+
     vector<uint8_t> data;
-    // If certificate association data is missing, it's OK. See the API
-    // documentation of the constructor.
-    if (certificate_assoc_data.size() > 0) {
-        try {
-            decodeHex(certificate_assoc_data, data);
-        } catch (const isc::BadValue& e) {
-            isc_throw(InvalidRdataText,
-                      "Bad TLSA certificate association data: " << e.what());
-        }
+    try {
+        decodeHex(certificate_assoc_data, data);
+    } catch (const isc::BadValue& e) {
+        isc_throw(InvalidRdataText,
+                  "Bad TLSA certificate association data: " << e.what());
     }
 
     return (new TLSAImpl(certificate_usage, selector, matching_type, data));
@@ -187,18 +187,26 @@ TLSA::TLSA(InputBuffer& buffer, size_t rdata_len) {
 
     vector<uint8_t> data;
     rdata_len -= 3;
-    if (rdata_len > 0) {
-        data.resize(rdata_len);
-        buffer.readData(&data[0], rdata_len);
+
+    if (rdata_len == 0) {
+        isc_throw(InvalidRdataLength,
+                  "Empty TLSA certificate association data");
     }
 
+    data.resize(rdata_len);
+    buffer.readData(&data[0], rdata_len);
+
     impl_ = new TLSAImpl(certificate_usage, selector, matching_type, data);
 }
 
 TLSA::TLSA(uint8_t certificate_usage, uint8_t selector,
-         uint8_t matching_type, const std::string& certificate_assoc_data) :
+           uint8_t matching_type, const std::string& certificate_assoc_data) :
     impl_(NULL)
 {
+    if (certificate_assoc_data.empty()) {
+        isc_throw(InvalidRdataText, "Empty TLSA certificate association data");
+    }
+
     vector<uint8_t> data;
     try {
         decodeHex(certificate_assoc_data, data);
@@ -237,10 +245,10 @@ TLSA::toWire(OutputBuffer& buffer) const {
     buffer.writeUint8(impl_->selector_);
     buffer.writeUint8(impl_->matching_type_);
 
-    if (!impl_->data_.empty()) {
-        buffer.writeData(&impl_->data_[0],
-                         impl_->data_.size());
-    }
+    // The constructors must ensure that the certificate association
+    // data field is not empty.
+    assert(!impl_->data_.empty());
+    buffer.writeData(&impl_->data_[0], impl_->data_.size());
 }
 
 void
@@ -249,19 +257,22 @@ TLSA::toWire(AbstractMessageRenderer& renderer) const {
     renderer.writeUint8(impl_->selector_);
     renderer.writeUint8(impl_->matching_type_);
 
-    if (!impl_->data_.empty()) {
-        renderer.writeData(&impl_->data_[0],
-                           impl_->data_.size());
-    }
+    // The constructors must ensure that the certificate association
+    // data field is not empty.
+    assert(!impl_->data_.empty());
+    renderer.writeData(&impl_->data_[0], impl_->data_.size());
 }
 
 string
 TLSA::toText() const {
+    // The constructors must ensure that the certificate association
+    // data field is not empty.
+    assert(!impl_->data_.empty());
+
     return (lexical_cast<string>(static_cast<int>(impl_->certificate_usage_)) + " " +
             lexical_cast<string>(static_cast<int>(impl_->selector_)) + " " +
-            lexical_cast<string>(static_cast<int>(impl_->matching_type_)) +
-            (impl_->data_.empty() ? "" :
-             " " + encodeHex(impl_->data_)));
+            lexical_cast<string>(static_cast<int>(impl_->matching_type_)) + " " +
+            encodeHex(impl_->data_));
 }
 
 int
diff --git a/src/lib/dns/tests/rdata_tlsa_unittest.cc b/src/lib/dns/tests/rdata_tlsa_unittest.cc
index 3e494c2623..b1129afc70 100644
--- a/src/lib/dns/tests/rdata_tlsa_unittest.cc
+++ b/src/lib/dns/tests/rdata_tlsa_unittest.cc
@@ -217,6 +217,11 @@ TEST_F(Rdata_TLSA_Test, createFromWire) {
                                       "rdata_tlsa_fromWire10"),
                  InvalidBufferPosition);
 
+    // certificate association data is empty
+    EXPECT_THROW(rdataFactoryFromFile(RRType("TLSA"), RRClass("IN"),
+                                      "rdata_tlsa_fromWire12"),
+                 InvalidRdataLength);
+
     // all RDATA is missing
     EXPECT_THROW(rdataFactoryFromFile(RRType("TLSA"), RRClass("IN"),
                                       "rdata_tlsa_fromWire11"),
@@ -228,17 +233,14 @@ TEST_F(Rdata_TLSA_Test, createFromParams) {
         0, 0, 1, "d2abde240d7cd3ee6b4b28c54df034b9"
                  "7983a1d16e8a410e4561cb106618e971");
     EXPECT_EQ(0, rdata_tlsa2.compare(rdata_tlsa));
+
+    // empty certificate association data should throw
+    EXPECT_THROW(const generic::TLSA rdata_tlsa2(0, 0, 1, ""),
+                 InvalidRdataText);
 }
 
 TEST_F(Rdata_TLSA_Test, toText) {
     EXPECT_TRUE(boost::iequals(tlsa_txt, rdata_tlsa.toText()));
-
-    const string tlsa_txt2("3 2 1");
-    const generic::TLSA rdata_tlsa2(tlsa_txt2);
-    EXPECT_TRUE(boost::iequals(tlsa_txt2, rdata_tlsa2.toText()));
-
-    const generic::TLSA rdata_tlsa3("3 2 1 ");
-    EXPECT_TRUE(boost::iequals(tlsa_txt2, rdata_tlsa3.toText()));
 }
 
 TEST_F(Rdata_TLSA_Test, toWire) {
@@ -255,7 +257,8 @@ TEST_F(Rdata_TLSA_Test, toWire) {
 }
 
 TEST_F(Rdata_TLSA_Test, compare) {
-    const generic::TLSA rdata_tlsa2("0 0 0");
+    const generic::TLSA rdata_tlsa2("0 0 0 d2abde240d7cd3ee6b4b28c54df034b9"
+                                    "7983a1d16e8a410e4561cb106618e971");
     EXPECT_EQ(-1, rdata_tlsa2.compare(rdata_tlsa));
     EXPECT_EQ(1, rdata_tlsa.compare(rdata_tlsa2));
 }
@@ -275,62 +278,4 @@ TEST_F(Rdata_TLSA_Test, getMatchingType) {
 TEST_F(Rdata_TLSA_Test, getDataLength) {
     EXPECT_EQ(32, rdata_tlsa.getDataLength());
 }
-
-TEST_F(Rdata_TLSA_Test, emptyCertificateAssociationDataFromWire) {
-    const uint8_t rdf_wiredata[] = {
-        // certificate usage
-        0x03,
-        // selector
-        0x01,
-        // matching type
-        0x02,
-    };
-
-    const generic::TLSA rdf =
-        dynamic_cast<const generic::TLSA&>
-        (*rdataFactoryFromFile(RRType("TLSA"), RRClass("IN"),
-                               "rdata_tlsa_fromWire12"));
-
-    EXPECT_EQ(3, rdf.getCertificateUsage());
-    EXPECT_EQ(1, rdf.getSelector());
-    EXPECT_EQ(2, rdf.getMatchingType());
-    EXPECT_EQ(0, rdf.getDataLength());
-
-    this->obuffer.clear();
-    rdf.toWire(this->obuffer);
-
-    EXPECT_EQ(3, this->obuffer.getLength());
-
-    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
-                        this->obuffer.getData(),
-                        this->obuffer.getLength(),
-                        rdf_wiredata, sizeof(rdf_wiredata));
-}
-
-TEST_F(Rdata_TLSA_Test, emptyCertificateAssociationDataFromString) {
-    const generic::TLSA rdata_tlsa2("3 2 1");
-    const uint8_t rdata_tlsa2_wiredata[] = {
-        // certificate usage
-        0x03,
-        // selector
-        0x02,
-        // matching type
-        0x01
-    };
-
-    EXPECT_EQ(3, rdata_tlsa2.getCertificateUsage());
-    EXPECT_EQ(2, rdata_tlsa2.getSelector());
-    EXPECT_EQ(1, rdata_tlsa2.getMatchingType());
-    EXPECT_EQ(0, rdata_tlsa2.getDataLength());
-
-    this->obuffer.clear();
-    rdata_tlsa2.toWire(this->obuffer);
-
-    EXPECT_EQ(3, this->obuffer.getLength());
-
-    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
-                        this->obuffer.getData(),
-                        this->obuffer.getLength(),
-                        rdata_tlsa2_wiredata, sizeof(rdata_tlsa2_wiredata));
-}
 }