[master] Merge branch 'trac910'

src/lib/dns/message.cc

@@ -239,7 +239,28 @@ MessageImpl::toWire(AbstractMessageRenderer& renderer, TSIGContext* tsig_ctx) {
                   "Message rendering attempted without Opcode set");
     }
 
+    // Reserve the space for TSIG (if needed) so that we can handle truncation
+    // case correctly later when that happens.  orig_xxx variables remember
+    // some configured parameters of renderer in case they are needed in
+    // truncation processing below.
+    const size_t tsig_len = (tsig_ctx != NULL) ? tsig_ctx->getTSIGLength() : 0;
+    const size_t orig_msg_len_limit = renderer.getLengthLimit();
+    const AbstractMessageRenderer::CompressMode orig_compress_mode =
+        renderer.getCompressMode();
+    if (tsig_len > 0) {
+        if (tsig_len > orig_msg_len_limit) {
+            isc_throw(InvalidParameter, "Failed to render DNS message: "
+                      "too small limit for a TSIG (" <<
+                      orig_msg_len_limit << ")");
+        }
+        renderer.setLengthLimit(orig_msg_len_limit - tsig_len);
+    }
+
     // reserve room for the header
+    if (renderer.getLengthLimit() < HEADERLEN) {
+        isc_throw(InvalidParameter, "Failed to render DNS message: "
+                  "too small limit for a Header");
+    }
     renderer.skip(HEADERLEN);
 
     uint16_t qdcount =
@@ -284,6 +305,22 @@ MessageImpl::toWire(AbstractMessageRenderer& renderer, TSIGContext* tsig_ctx) {
         }
     }
 
+    // If we're adding a TSIG to a truncated message, clear all RRsets
+    // from the message except for the question before adding the TSIG.
+    // If even (some of) the question doesn't fit, don't include it.
+    if (tsig_ctx != NULL && renderer.isTruncated()) {
+        renderer.clear();
+        renderer.setLengthLimit(orig_msg_len_limit - tsig_len);
+        renderer.setCompressMode(orig_compress_mode);
+        renderer.skip(HEADERLEN);
+        qdcount = for_each(questions_.begin(), questions_.end(),
+                           RenderSection<QuestionPtr>(renderer,
+                                                      false)).getTotalCount();
+        ancount = 0;
+        nscount = 0;
+        arcount = 0;
+    }
+
     // Adjust the counter buffer.
     // XXX: these may not be equal to the number of corresponding entries
     // in rrsets_[] or questions_ if truncation occurred or an EDNS OPT RR
@@ -315,10 +352,16 @@ MessageImpl::toWire(AbstractMessageRenderer& renderer, TSIGContext* tsig_ctx) {
     renderer.writeUint16At(arcount, header_pos);
 
     // Add TSIG, if necessary, at the end of the message.
-    // TODO: truncate case consideration
     if (tsig_ctx != NULL) {
-        tsig_ctx->sign(qid_, renderer.getData(),
-                       renderer.getLength())->toWire(renderer);
+        // Release the reserved space in the renderer.
+        renderer.setLengthLimit(orig_msg_len_limit);
+
+        const int tsig_count =
+            tsig_ctx->sign(qid_, renderer.getData(),
+                           renderer.getLength())->toWire(renderer);
+        if (tsig_count != 1) {
+            isc_throw(Unexpected, "Failed to render a TSIG RR");
+        }
 
         // update the ARCOUNT for the TSIG RR.  Note that for a sane DNS
         // message arcount should never overflow to 0.

src/lib/dns/message.h

@@ -565,6 +565,17 @@ public:
     /// \c tsig_ctx will be updated based on the fact it was used for signing
     /// and with the latest MAC.
     ///
+    /// \exception InvalidMessageOperation The message is not in the Render
+    /// mode, or either Rcode or Opcode is not set.
+    /// \exception InvalidParameter The allowable limit of \c renderer is too
+    /// small for a TSIG or the Header section.  Note that this shouldn't
+    /// happen with parameters as defined in the standard protocols,
+    /// so it's more likely a program bug.
+    /// \exception Unexpected Rendering the TSIG RR fails.  The implementation
+    /// internally makes sure this doesn't happen, so if that ever occurs
+    /// it should mean a bug either in the TSIG context or in the renderer
+    /// implementation.
+    ///
     /// \param renderer See the other version
     /// \param tsig_ctx A TSIG context that is to be used for signing the
     /// message

src/lib/dns/python/message_python.cc

@@ -703,6 +703,15 @@ Message_toWire(s_Message* self, PyObject* args) {
             // python program has a bug.
             PyErr_SetString(po_TSIGContextError, ex.what());
             return (NULL);
+        } catch (const std::exception& ex) {
+            // Other exceptions should be rare (most likely an implementation
+            // bug)
+            PyErr_SetString(po_TSIGContextError, ex.what());
+            return (NULL);
+        } catch (...) {
+            PyErr_SetString(PyExc_RuntimeError,
+                            "Unexpected C++ exception in Message.to_wire");
+            return (NULL);
         }
     }
     PyErr_Clear();

src/lib/dns/python/tests/message_python_test.py

@@ -21,6 +21,7 @@ import unittest
 import os
 from pydnspp import *
 from testutil import *
+from pyunittests_util import fix_current_time
 
 # helper functions for tests taken from c++ unittests
 if "TESTDATA_PATH" in os.environ:
@@ -31,7 +32,7 @@ else:
 def factoryFromFile(message, file):
     data = read_wire_data(file)
     message.from_wire(data)
-    pass
+    return data
 
 # we don't have direct comparison for rrsets right now (should we?
 # should go in the cpp version first then), so also no direct list
@@ -44,6 +45,15 @@ def compare_rrset_list(list1, list2):
             return False
     return True
 
+# These are used for TSIG + TC tests
+LONG_TXT1 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde";
+
+LONG_TXT2 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456";
+
+LONG_TXT3 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01";
+
+LONG_TXT4 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0";
+
 # a complete message taken from cpp tests, for testing towire and totext
 def create_message():
     message_render = Message(Message.RENDER)
@@ -62,16 +72,12 @@ def create_message():
     message_render.add_rrset(Message.SECTION_ANSWER, rrset)
     return message_render
 
-def strip_mutable_tsig_data(data):
-    # Unfortunately we cannot easily compare TSIG RR because we can't tweak
-    # current time.  As a work around this helper function strips off the time
-    # dependent part of TSIG RDATA, i.e., the MAC (assuming HMAC-MD5) and
-    # Time Signed.
-    return data[0:-32] + data[-26:-22] + data[-6:]
-
 class MessageTest(unittest.TestCase):
 
     def setUp(self):
+        # make sure we don't use faked time unless explicitly do so in tests
+        fix_current_time(None)
+
         self.p = Message(Message.PARSE)
         self.r = Message(Message.RENDER)
 
@@ -90,6 +96,10 @@ class MessageTest(unittest.TestCase):
         self.tsig_key = TSIGKey("www.example.com:SFuWd/q99SzF8Yzd1QbB9g==")
         self.tsig_ctx = TSIGContext(self.tsig_key)
 
+    def tearDown(self):
+        # reset any faked current time setting (it would affect other tests)
+        fix_current_time(None)
+
     def test_init(self):
         self.assertRaises(TypeError, Message, -1)
         self.assertRaises(TypeError, Message, 3)
@@ -285,33 +295,112 @@ class MessageTest(unittest.TestCase):
         self.assertRaises(InvalidMessageOperation, self.r.to_wire,
                           MessageRenderer())
 
-    def __common_tsigquery_setup(self):
+    def __common_tsigmessage_setup(self, flags=[Message.HEADERFLAG_RD],
+                                   rrtype=RRType("A"), answer_data=None):
         self.r.set_opcode(Opcode.QUERY())
         self.r.set_rcode(Rcode.NOERROR())
-        self.r.set_header_flag(Message.HEADERFLAG_RD)
+        for flag in flags:
+            self.r.set_header_flag(flag)
+        if answer_data is not None:
+            rrset = RRset(Name("www.example.com"), RRClass("IN"),
+                          rrtype, RRTTL(86400))
+            for rdata in answer_data:
+                rrset.add_rdata(Rdata(rrtype, RRClass("IN"), rdata))
+            self.r.add_rrset(Message.SECTION_ANSWER, rrset)
         self.r.add_question(Question(Name("www.example.com"),
-                                     RRClass("IN"), RRType("A")))
+                                     RRClass("IN"), rrtype))
 
     def __common_tsig_checks(self, expected_file):
         renderer = MessageRenderer()
         self.r.to_wire(renderer, self.tsig_ctx)
-        actual_wire = strip_mutable_tsig_data(renderer.get_data())
-        expected_wire = strip_mutable_tsig_data(read_wire_data(expected_file))
-        self.assertEqual(expected_wire, actual_wire)
+        self.assertEqual(read_wire_data(expected_file), renderer.get_data())
 
     def test_to_wire_with_tsig(self):
+        fix_current_time(0x4da8877a)
         self.r.set_qid(0x2d65)
-        self.__common_tsigquery_setup()
+        self.__common_tsigmessage_setup()
         self.__common_tsig_checks("message_toWire2.wire")
 
     def test_to_wire_with_edns_tsig(self):
+        fix_current_time(0x4db60d1f)
         self.r.set_qid(0x6cd)
-        self.__common_tsigquery_setup()
+        self.__common_tsigmessage_setup()
         edns = EDNS()
         edns.set_udp_size(4096)
         self.r.set_edns(edns)
         self.__common_tsig_checks("message_toWire3.wire")
 
+    def test_to_wire_tsig_truncation(self):
+        fix_current_time(0x4e179212)
+        data = factoryFromFile(self.p, "message_fromWire17.wire")
+        self.assertEqual(TSIGError.NOERROR,
+                         self.tsig_ctx.verify(self.p.get_tsig_record(), data))
+        self.r.set_qid(0x22c2)
+        self.__common_tsigmessage_setup([Message.HEADERFLAG_QR,
+                                         Message.HEADERFLAG_AA,
+                                         Message.HEADERFLAG_RD],
+                                        RRType("TXT"),
+                                        [LONG_TXT1, LONG_TXT2])
+        self.__common_tsig_checks("message_toWire4.wire")
+
+    def test_to_wire_tsig_truncation2(self):
+        fix_current_time(0x4e179212)
+        data = factoryFromFile(self.p, "message_fromWire17.wire")
+        self.assertEqual(TSIGError.NOERROR,
+                         self.tsig_ctx.verify(self.p.get_tsig_record(), data))
+        self.r.set_qid(0x22c2)
+        self.__common_tsigmessage_setup([Message.HEADERFLAG_QR,
+                                         Message.HEADERFLAG_AA,
+                                         Message.HEADERFLAG_RD],
+                                        RRType("TXT"),
+                                        [LONG_TXT1, LONG_TXT3])
+        self.__common_tsig_checks("message_toWire4.wire")
+
+    def test_to_wire_tsig_truncation3(self):
+        self.r.set_opcode(Opcode.QUERY())
+        self.r.set_rcode(Rcode.NOERROR())
+        for i in range(1, 68):
+            self.r.add_question(Question(Name("www.example.com"),
+                                         RRClass("IN"), RRType(i)))
+        renderer = MessageRenderer()
+        self.r.to_wire(renderer, self.tsig_ctx)
+
+        self.p.from_wire(renderer.get_data())
+        self.assertTrue(self.p.get_header_flag(Message.HEADERFLAG_TC))
+        self.assertEqual(66, self.p.get_rr_count(Message.SECTION_QUESTION))
+        self.assertNotEqual(None, self.p.get_tsig_record())
+
+    def test_to_wire_tsig_no_truncation(self):
+        fix_current_time(0x4e17b38d)
+        data = factoryFromFile(self.p, "message_fromWire18.wire")
+        self.assertEqual(TSIGError.NOERROR,
+                         self.tsig_ctx.verify(self.p.get_tsig_record(), data))
+        self.r.set_qid(0xd6e2)
+        self.__common_tsigmessage_setup([Message.HEADERFLAG_QR,
+                                         Message.HEADERFLAG_AA,
+                                         Message.HEADERFLAG_RD],
+                                        RRType("TXT"),
+                                        [LONG_TXT1, LONG_TXT4])
+        self.__common_tsig_checks("message_toWire5.wire")
+
+    def test_to_wire_tsig_length_errors(self):
+        renderer = MessageRenderer()
+        renderer.set_length_limit(84) # 84 = expected TSIG length - 1
+        self.__common_tsigmessage_setup()
+        self.assertRaises(TSIGContextError,
+                          self.r.to_wire, renderer, self.tsig_ctx)
+
+        renderer.clear()
+        self.r.clear(Message.RENDER)
+        renderer.set_length_limit(86) # 86 = expected TSIG length + 1
+        self.__common_tsigmessage_setup()
+        self.assertRaises(TSIGContextError,
+                          self.r.to_wire, renderer, self.tsig_ctx)
+
+        # skip the last test of the corresponding C++ test: it requires
+        # subclassing MessageRenderer, which is (currently) not possible
+        # for python.  In any case, it's very unlikely to happen in practice.
+
     def test_to_text(self):
         message_render = create_message()
         

src/lib/dns/python/tests/question_python_test.py

@@ -74,7 +74,6 @@ class QuestionTest(unittest.TestCase):
         self.assertEqual("foo.example.com. IN NS\n", str(self.test_question1))
         self.assertEqual("bar.example.com. CH A\n", self.test_question2.to_text())
     
-    
     def test_to_wire_buffer(self):
         obuffer = bytes()
         obuffer = self.test_question1.to_wire(obuffer)
@@ -82,7 +81,6 @@ class QuestionTest(unittest.TestCase):
         wiredata = read_wire_data("question_toWire1")
         self.assertEqual(obuffer, wiredata)
     
-    
     def test_to_wire_renderer(self):
         renderer = MessageRenderer()
         self.test_question1.to_wire(renderer)
@@ -91,5 +89,13 @@ class QuestionTest(unittest.TestCase):
         self.assertEqual(renderer.get_data(), wiredata)
         self.assertRaises(TypeError, self.test_question1.to_wire, 1)
 
+    def test_to_wire_truncated(self):
+        renderer = MessageRenderer()
+        renderer.set_length_limit(self.example_name1.get_length())
+        self.assertFalse(renderer.is_truncated())
+        self.test_question1.to_wire(renderer)
+        self.assertTrue(renderer.is_truncated())
+        self.assertEqual(0, renderer.get_length())
+
 if __name__ == '__main__':
     unittest.main()

src/lib/dns/question.cc

@@ -57,10 +57,19 @@ Question::toWire(OutputBuffer& buffer) const {
 
 unsigned int
 Question::toWire(AbstractMessageRenderer& renderer) const {
+    const size_t pos0 = renderer.getLength();
+
     renderer.writeName(name_);
     rrtype_.toWire(renderer);
     rrclass_.toWire(renderer);
 
+    // Make sure the renderer has a room for the question
+    if (renderer.getLength() > renderer.getLengthLimit()) {
+        renderer.trim(renderer.getLength() - pos0);
+        renderer.setTruncated();
+        return (0);
+    }
+
     return (1);                 // number of "entries"
 }
 

src/lib/dns/question.h

@@ -201,23 +201,23 @@ public:
     /// class description).
     ///
     /// The owner name will be compressed if possible, although it's an
-    /// unlikely event in practice because the %Question section a DNS
+    /// unlikely event in practice because the Question section a DNS
     /// message normally doesn't contain multiple question entries and
     /// it's located right after the Header section.
     /// Nevertheless, \c renderer records the information of the owner name
     /// so that it can be pointed by other RRs in other sections (which is
     /// more likely to happen).
     ///
-    /// In theory, an attempt to render a Question may cause truncation
-    /// (when the Question section contains a large number of entries),
-    /// but this implementation doesn't catch that situation.
-    /// It would make the code unnecessarily complicated (though perhaps
-    /// slightly) for almost impossible case in practice.
-    /// An upper layer will handle the pathological case as a general error.
+    /// It could be possible, though very rare in practice, that
+    /// an attempt to render a Question may cause truncation
+    /// (when the Question section contains a large number of entries).
+    /// In such a case this method avoid the rendering and indicate the
+    /// truncation in the \c renderer.  This method returns 0 in this case.
     ///
     /// \param renderer DNS message rendering context that encapsulates the
     /// output buffer and name compression information.
-    /// \return 1
+    ///
+    /// \return 1 on success; 0 if it causes truncation
     unsigned int toWire(AbstractMessageRenderer& renderer) const;
 
     /// \brief Render the Question in the wire format without name compression.

src/lib/dns/tests/message_unittest.cc

@@ -62,7 +62,6 @@ using namespace isc::dns::rdata;
 //
 
 const uint16_t Message::DEFAULT_MAX_UDPSIZE;
-const Name test_name("test.example.com");
 
 namespace isc {
 namespace util {
@@ -79,7 +78,8 @@ const uint16_t TSIGContext::DEFAULT_FUDGE;
 namespace {
 class MessageTest : public ::testing::Test {
 protected:
-    MessageTest() : obuffer(0), renderer(obuffer),
+    MessageTest() : test_name("test.example.com"), obuffer(0),
+                    renderer(obuffer),
                     message_parse(Message::PARSE),
                     message_render(Message::RENDER),
                     bogus_section(static_cast<Message::Section>(
@@ -103,8 +103,9 @@ protected:
                                              "FAKEFAKEFAKEFAKE"));
         rrset_aaaa->addRRsig(rrset_rrsig);
     }
-    
+
     static Question factoryFromFile(const char* datafile);
+    const Name test_name;
     OutputBuffer obuffer;
     MessageRenderer renderer;
     Message message_parse;
@@ -114,17 +115,18 @@ protected:
     RRsetPtr rrset_aaaa;        // AAAA RRset with one RDATA with RRSIG
     RRsetPtr rrset_rrsig;       // RRSIG for the AAAA RRset
     TSIGContext tsig_ctx;
+    vector<unsigned char> received_data;
     vector<unsigned char> expected_data;
 
-    static void factoryFromFile(Message& message, const char* datafile);
+    void factoryFromFile(Message& message, const char* datafile);
 };
 
 void
 MessageTest::factoryFromFile(Message& message, const char* datafile) {
-    std::vector<unsigned char> data;
-    UnitTestUtil::readWireData(datafile, data);
+    received_data.clear();
+    UnitTestUtil::readWireData(datafile, received_data);
 
-    InputBuffer buffer(&data[0], data.size());
+    InputBuffer buffer(&received_data[0], received_data.size());
     message.fromWire(buffer);
 }
 
@@ -618,15 +620,43 @@ testGetTime() {
     return (NOW);
 }
 
+// bit-wise constant flags to configure DNS header flags for test
+// messages.
+const unsigned int QR_FLAG = 0x1;
+const unsigned int AA_FLAG = 0x2;
+const unsigned int RD_FLAG = 0x4;
+
 void
 commonTSIGToWireCheck(Message& message, MessageRenderer& renderer,
-                      TSIGContext& tsig_ctx, const char* const expected_file)
+                      TSIGContext& tsig_ctx, const char* const expected_file,
+                      unsigned int message_flags = RD_FLAG,
+                      RRType qtype = RRType::A(),
+                      const vector<const char*>* answer_data = NULL)
 {
     message.setOpcode(Opcode::QUERY());
     message.setRcode(Rcode::NOERROR());
-    message.setHeaderFlag(Message::HEADERFLAG_RD, true);
+    if ((message_flags & QR_FLAG) != 0) {
+        message.setHeaderFlag(Message::HEADERFLAG_QR);
+    }
+    if ((message_flags & AA_FLAG) != 0) {
+        message.setHeaderFlag(Message::HEADERFLAG_AA);
+    }
+    if ((message_flags & RD_FLAG) != 0) {
+        message.setHeaderFlag(Message::HEADERFLAG_RD);
+    }
     message.addQuestion(Question(Name("www.example.com"), RRClass::IN(),
-                                 RRType::A()));
+                                 qtype));
+
+    if (answer_data != NULL) {
+        RRsetPtr ans_rrset(new RRset(Name("www.example.com"), RRClass::IN(),
+                                     qtype, RRTTL(86400)));
+        for (vector<const char*>::const_iterator it = answer_data->begin();
+             it != answer_data->end();
+             ++it) {
+            ans_rrset->addRdata(createRdata(qtype, RRClass::IN(), *it));
+        }
+        message.addRRset(Message::SECTION_ANSWER, ans_rrset);
+    }
 
     message.toWire(renderer, tsig_ctx);
     vector<unsigned char> expected_data;
@@ -670,6 +700,182 @@ TEST_F(MessageTest, toWireWithEDNSAndTSIG) {
     }
 }
 
+// Some of the following tests involve truncation.  We use the query name
+// "www.example.com" and some TXT question/answers.  The length of the
+// header and question will be 33 bytes.  If we also try to include a
+// TSIG of the same key name (not compressed) with HMAC-MD5, the TSIG RR
+// will be 85 bytes.
+
+// A long TXT RDATA.  With a fully compressed owner name, the corresponding
+// RR will be 268 bytes.
+const char* const long_txt1 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde";
+
+// With a fully compressed owner name, the corresponding RR will be 212 bytes.
+// It should result in truncation even without TSIG (33 + 268 + 212 = 513)
+const char* const long_txt2 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456";
+
+// With a fully compressed owner name, the corresponding RR will be 127 bytes.
+// So, it can fit in the standard 512 bytes with txt1 and without TSIG, but
+// adding a TSIG would result in truncation (33 + 268 + 127 + 85 = 513)
+const char* const long_txt3 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01";
+
+// This is 1 byte shorter than txt3, which will result in a possible longest
+// message containing answer RRs and TSIG.
+const char* const long_txt4 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0";
+
+// Example output generated by
+// "dig -y www.example.com:SFuWd/q99SzF8Yzd1QbB9g== www.example.com txt
+// QID: 0x22c2
+// Time Signed: 0x00004e179212
+TEST_F(MessageTest, toWireTSIGTruncation) {
+    isc::util::detail::gettimeFunction = testGetTime<0x4e179212>;
+
+    // Verify a validly signed query so that we can use the TSIG context
+
+    factoryFromFile(message_parse, "message_fromWire17.wire");
+    EXPECT_EQ(TSIGError::NOERROR(),
+              tsig_ctx.verify(message_parse.getTSIGRecord(),
+                              &received_data[0], received_data.size()));
+
+    message_render.setQid(0x22c2);
+    vector<const char*> answer_data;
+    answer_data.push_back(long_txt1);
+    answer_data.push_back(long_txt2);
+    {
+        SCOPED_TRACE("Message sign with TSIG and TC bit on");
+        commonTSIGToWireCheck(message_render, renderer, tsig_ctx,
+                              "message_toWire4.wire",
+                              QR_FLAG|AA_FLAG|RD_FLAG,
+                              RRType::TXT(), &answer_data);
+    }
+}
+
+TEST_F(MessageTest, toWireTSIGTruncation2) {
+    // Similar to the previous test, but without TSIG it wouldn't cause
+    // truncation.
+    isc::util::detail::gettimeFunction = testGetTime<0x4e179212>;
+    factoryFromFile(message_parse, "message_fromWire17.wire");
+    EXPECT_EQ(TSIGError::NOERROR(),
+              tsig_ctx.verify(message_parse.getTSIGRecord(),
+                              &received_data[0], received_data.size()));
+
+    message_render.setQid(0x22c2);
+    vector<const char*> answer_data;
+    answer_data.push_back(long_txt1);
+    answer_data.push_back(long_txt3);
+    {
+        SCOPED_TRACE("Message sign with TSIG and TC bit on (2)");
+        commonTSIGToWireCheck(message_render, renderer, tsig_ctx,
+                              "message_toWire4.wire",
+                              QR_FLAG|AA_FLAG|RD_FLAG,
+                              RRType::TXT(), &answer_data);
+    }
+}
+
+TEST_F(MessageTest, toWireTSIGTruncation3) {
+    // Similar to previous ones, but truncation occurs due to too many
+    // Questions (very unusual, but not necessarily illegal).
+
+    // We are going to create a message starting with a standard
+    // header (12 bytes) and multiple questions in the Question
+    // section of the same owner name (changing the RRType, just so
+    // that it would be the form that would be accepted by the BIND 9
+    // parser).  The first Question is 21 bytes in length, and the subsequent
+    // ones are 6 bytes.  We'll also use a TSIG whose size is 85 bytes.
+    // Up to 66 questions can fit in the standard 512-byte buffer
+    // (12 + 21 + 6 * 65 + 85 = 508).  If we try to add one more it would
+    // result in truncation.
+    message_render.setOpcode(Opcode::QUERY());
+    message_render.setRcode(Rcode::NOERROR());
+    for (int i = 1; i <= 67; ++i) {
+        message_render.addQuestion(Question(Name("www.example.com"),
+                                            RRClass::IN(), RRType(i)));
+    }
+    message_render.toWire(renderer, tsig_ctx);
+
+    // Check the rendered data by parsing it.  We only check it has the
+    // TC bit on, has the correct number of questions, and has a TSIG RR.
+    // Checking the signature wouldn't be necessary for this rare case
+    // scenario.
+    InputBuffer buffer(renderer.getData(), renderer.getLength());
+    message_parse.fromWire(buffer);
+    EXPECT_TRUE(message_parse.getHeaderFlag(Message::HEADERFLAG_TC));
+    // Note that the number of questions are 66, not 67 as we tried to add.
+    EXPECT_EQ(66, message_parse.getRRCount(Message::SECTION_QUESTION));
+    EXPECT_TRUE(message_parse.getTSIGRecord() != NULL);
+}
+
+TEST_F(MessageTest, toWireTSIGNoTruncation) {
+    // A boundary case that shouldn't cause truncation: the resulting
+    // response message with a TSIG will be 512 bytes long.
+    isc::util::detail::gettimeFunction = testGetTime<0x4e17b38d>;
+    factoryFromFile(message_parse, "message_fromWire18.wire");
+    EXPECT_EQ(TSIGError::NOERROR(),
+              tsig_ctx.verify(message_parse.getTSIGRecord(),
+                              &received_data[0], received_data.size()));
+
+    message_render.setQid(0xd6e2);
+    vector<const char*> answer_data;
+    answer_data.push_back(long_txt1);
+    answer_data.push_back(long_txt4);
+    {
+        SCOPED_TRACE("Message sign with TSIG, no truncation");
+        commonTSIGToWireCheck(message_render, renderer, tsig_ctx,
+                              "message_toWire5.wire",
+                              QR_FLAG|AA_FLAG|RD_FLAG,
+                              RRType::TXT(), &answer_data);
+    }
+}
+
+// This is a buggy renderer for testing.  It behaves like the straightforward
+// MessageRenderer, but once it has some data, its setLengthLimit() ignores
+// the given parameter and resets the limit to the current length, making
+// subsequent insertion result in truncation, which would make TSIG RR
+// rendering fail unexpectedly in the test that follows.
+class BadRenderer : public MessageRenderer {
+public:
+    BadRenderer(isc::util::OutputBuffer& buffer) :
+        MessageRenderer(buffer)
+    {}
+    virtual void setLengthLimit(size_t len) {
+        if (getLength() > 0) {
+            MessageRenderer::setLengthLimit(getLength());
+        } else {
+            MessageRenderer::setLengthLimit(len);
+        }
+    }
+};
+
+TEST_F(MessageTest, toWireTSIGLengthErrors) {
+    // specify an unusual short limit that wouldn't be able to hold
+    // the TSIG.
+    renderer.setLengthLimit(tsig_ctx.getTSIGLength() - 1);
+    // Use commonTSIGToWireCheck() only to call toWire() with otherwise valid
+    // conditions.  The checks inside it don't matter because we expect an
+    // exception before any of the checks.
+    EXPECT_THROW(commonTSIGToWireCheck(message_render, renderer, tsig_ctx,
+                                       "message_toWire2.wire"),
+                 InvalidParameter);
+
+    // This one is large enough for TSIG, but the remaining limit isn't
+    // even enough for the Header section.
+    renderer.clear();
+    message_render.clear(Message::RENDER);
+    renderer.setLengthLimit(tsig_ctx.getTSIGLength() + 1);
+    EXPECT_THROW(commonTSIGToWireCheck(message_render, renderer, tsig_ctx,
+                                       "message_toWire2.wire"),
+                 InvalidParameter);
+
+    // Trying to render a message with TSIG using a buggy renderer.
+    obuffer.clear();
+    BadRenderer bad_renderer(obuffer);
+    bad_renderer.setLengthLimit(512);
+    message_render.clear(Message::RENDER);
+    EXPECT_THROW(commonTSIGToWireCheck(message_render, bad_renderer, tsig_ctx,
+                                       "message_toWire2.wire"),
+                 Unexpected);
+}
+
 TEST_F(MessageTest, toWireWithoutOpcode) {
     message_render.setRcode(Rcode::NOERROR());
     EXPECT_THROW(message_render.toWire(renderer), InvalidMessageOperation);

src/lib/dns/tests/question_unittest.cc

@@ -106,6 +106,22 @@ TEST_F(QuestionTest, toWireRenderer) {
                         obuffer.getLength(), &wiredata[0], wiredata.size());
 }
 
+TEST_F(QuestionTest, toWireTruncated) {
+    // If the available length in the renderer is too small, it would require
+    // truncation.  This won't happen in normal cases, but protocol wise it
+    // could still happen if and when we support some (possibly future) opcode
+    // that allows multiple questions.
+
+    // Set the length limit to the qname length so that the whole question
+    // would request truncated
+    renderer.setLengthLimit(example_name1.getLength());
+
+    EXPECT_FALSE(renderer.isTruncated()); // check pre-render condition
+    EXPECT_EQ(0, test_question1.toWire(renderer));
+    EXPECT_TRUE(renderer.isTruncated());
+    EXPECT_EQ(0, renderer.getLength()); // renderer shouldn't have any data
+}
+
 // test operator<<.  We simply confirm it appends the result of toText().
 TEST_F(QuestionTest, LeftShiftOperator) {
     ostringstream oss;

src/lib/dns/tests/testdata/Makefile.am

@@ -5,8 +5,10 @@ BUILT_SOURCES += edns_toWire4.wire
 BUILT_SOURCES += message_fromWire10.wire message_fromWire11.wire
 BUILT_SOURCES += message_fromWire12.wire message_fromWire13.wire
 BUILT_SOURCES += message_fromWire14.wire message_fromWire15.wire
-BUILT_SOURCES += message_fromWire16.wire
+BUILT_SOURCES += message_fromWire16.wire message_fromWire17.wire
+BUILT_SOURCES += message_fromWire18.wire
 BUILT_SOURCES += message_toWire2.wire message_toWire3.wire
+BUILT_SOURCES += message_toWire4.wire message_toWire5.wire
 BUILT_SOURCES += message_toText1.wire message_toText2.wire
 BUILT_SOURCES += message_toText3.wire
 BUILT_SOURCES += name_toWire5.wire name_toWire6.wire
@@ -59,7 +61,9 @@ EXTRA_DIST += message_fromWire9 message_fromWire10.spec
 EXTRA_DIST += message_fromWire11.spec message_fromWire12.spec
 EXTRA_DIST += message_fromWire13.spec message_fromWire14.spec
 EXTRA_DIST += message_fromWire15.spec message_fromWire16.spec
+EXTRA_DIST += message_fromWire17.spec message_fromWire18.spec
 EXTRA_DIST += message_toWire1 message_toWire2.spec message_toWire3.spec
+EXTRA_DIST += message_toWire4.spec message_toWire5.spec
 EXTRA_DIST += message_toText1.txt message_toText1.spec
 EXTRA_DIST += message_toText2.txt message_toText2.spec
 EXTRA_DIST += message_toText3.txt message_toText3.spec

src/lib/dns/tests/testdata/gen-wiredata.py.in

@@ -307,8 +307,8 @@ class SOA(RR):
                                                 self.retry, self.expire,
                                                 self.minimum))
 
-class TXT:
-    rdlen = -1                  # auto-calculate
+class TXT(RR):
+    rdlen = None                # auto-calculate
     nstring = 1                 # number of character-strings
     stringlen = -1              # default string length, auto-calculate
     string = 'Test String'      # default string
@@ -330,11 +330,9 @@ class TXT:
                 stringlen_list.append(self.stringlen)
             if stringlen_list[-1] < 0:
                 stringlen_list[-1] = int(len(wirestring_list[-1]) / 2)
-        rdlen = self.rdlen
-        if rdlen < 0:
-            rdlen = int(len(''.join(wirestring_list)) / 2) + self.nstring
-        f.write('\n# TXT RDATA (RDLEN=%d)\n' % rdlen)
-        f.write('%04x\n' % rdlen);
+        if self.rdlen is None:
+            self.rdlen = int(len(''.join(wirestring_list)) / 2) + self.nstring
+        self.dump_header(f, self.rdlen)
         for i in range(0, self.nstring):
             f.write('# String Len=%d, String=\"%s\"\n' %
                     (stringlen_list[i], string_list[i]))

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

@@ -0,0 +1,22 @@
+#
+# A simple DNS query message with TSIG signed
+#
+
+[custom]
+sections: header:question:tsig
+[header]
+id: 0x22c2
+rd: 1
+arcount: 1
+[question]
+name: www.example.com
+rrtype: TXT
+[tsig]
+as_rr: True
+# TSIG QNAME won't be compressed
+rr_name: www.example.com
+algorithm: hmac-md5
+time_signed: 0x4e179212
+mac_size: 16
+mac: 0x8214b04634e32323d651ac60b08e6388
+original_id: 0x22c2

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

@@ -0,0 +1,23 @@
+#
+# Another simple DNS query message with TSIG signed.  Only ID and time signed
+# (and MAC as a result) are different.
+#
+
+[custom]
+sections: header:question:tsig
+[header]
+id: 0xd6e2
+rd: 1
+arcount: 1
+[question]
+name: www.example.com
+rrtype: TXT
+[tsig]
+as_rr: True
+# TSIG QNAME won't be compressed
+rr_name: www.example.com
+algorithm: hmac-md5
+time_signed: 0x4e17b38d
+mac_size: 16
+mac: 0x903b5b194a799b03a37718820c2404f2
+original_id: 0xd6e2

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

@@ -0,0 +1,27 @@
+#
+# Truncated DNS response with TSIG signed
+# This is expected to be a response to "fromWire17"
+#
+
+[custom]
+sections: header:question:tsig
+[header]
+id: 0x22c2
+rd: 1
+qr: 1
+aa: 1
+# It's "truncated":
+tc: 1
+arcount: 1
+[question]
+name: www.example.com
+rrtype: TXT
+[tsig]
+as_rr: True
+# TSIG QNAME won't be compressed
+rr_name: www.example.com
+algorithm: hmac-md5
+time_signed: 0x4e179212
+mac_size: 16
+mac: 0x88adc3811d1d6bec7c684438906fc694
+original_id: 0x22c2

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

@@ -0,0 +1,36 @@
+#
+# A longest possible (without EDNS) DNS response with TSIG, i.e. totatl
+# length should be 512 bytes.
+#
+
+[custom]
+sections: header:question:txt/1:txt/2:tsig
+[header]
+id: 0xd6e2
+rd: 1
+qr: 1
+aa: 1
+ancount: 2
+arcount: 1
+[question]
+name: www.example.com
+rrtype: TXT
+[txt/1]
+as_rr: True
+# QNAME is fully compressed
+rr_name: ptr=12
+string: 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
+[txt/2]
+as_rr: True
+# QNAME is fully compressed
+rr_name: ptr=12
+string: 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0
+[tsig]
+as_rr: True
+# TSIG QNAME won't be compressed
+rr_name: www.example.com
+algorithm: hmac-md5
+time_signed: 0x4e17b38d
+mac_size: 16
+mac: 0xbe2ba477373d2496891e2fda240ee4ec
+original_id: 0xd6e2

src/lib/dns/tests/tsig_unittest.cc

@@ -927,4 +927,76 @@ TEST_F(TSIGTest, tooShortMAC) {
     }
 }
 
+TEST_F(TSIGTest, getTSIGLength) {
+    // Check for the most common case with various algorithms
+    // See the comment in TSIGContext::getTSIGLength() for calculation and
+    // parameter notation.
+    // The key name (www.example.com) is the same for most cases, where n1=17
+
+    // hmac-md5.sig-alg.reg.int.: n2=26, x=16
+    EXPECT_EQ(85, tsig_ctx->getTSIGLength());
+
+    // hmac-sha1: n2=11, x=20
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name, TSIGKey::HMACSHA1_NAME(),
+                                           &dummy_data[0], 20)));
+    EXPECT_EQ(74, tsig_ctx->getTSIGLength());
+
+    // hmac-sha256: n2=13, x=32
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name,
+                                           TSIGKey::HMACSHA256_NAME(),
+                                           &dummy_data[0], 32)));
+    EXPECT_EQ(88, tsig_ctx->getTSIGLength());
+
+    // hmac-sha224: n2=13, x=28
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name,
+                                           TSIGKey::HMACSHA224_NAME(),
+                                           &dummy_data[0], 28)));
+    EXPECT_EQ(84, tsig_ctx->getTSIGLength());
+
+    // hmac-sha384: n2=13, x=48
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name,
+                                           TSIGKey::HMACSHA384_NAME(),
+                                           &dummy_data[0], 48)));
+    EXPECT_EQ(104, tsig_ctx->getTSIGLength());
+
+    // hmac-sha512: n2=13, x=64
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name,
+                                           TSIGKey::HMACSHA512_NAME(),
+                                           &dummy_data[0], 64)));
+    EXPECT_EQ(120, tsig_ctx->getTSIGLength());
+
+    // bad key case: n1=len(badkey.example.com)=20, n2=26, x=0
+    tsig_ctx.reset(new TSIGContext(badkey_name, TSIGKey::HMACMD5_NAME(),
+                                   keyring));
+    EXPECT_EQ(72, tsig_ctx->getTSIGLength());
+
+    // bad sig case: n1=17, n2=26, x=0
+    isc::util::detail::gettimeFunction = testGetTime<0x4da8877a>;
+    createMessageFromFile("message_toWire2.wire");
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name, TSIGKey::HMACMD5_NAME(),
+                                           &dummy_data[0],
+                                           dummy_data.size())));
+    {
+        SCOPED_TRACE("Verify resulting in BADSIG");
+        commonVerifyChecks(*tsig_ctx, message.getTSIGRecord(),
+                           &received_data[0], received_data.size(),
+                           TSIGError::BAD_SIG(), TSIGContext::RECEIVED_REQUEST);
+    }
+    EXPECT_EQ(69, tsig_ctx->getTSIGLength());
+
+    // bad time case: n1=17, n2=26, x=16, y=6
+    isc::util::detail::gettimeFunction = testGetTime<0x4da8877a - 1000>;
+    tsig_ctx.reset(new TSIGContext(TSIGKey(test_name, TSIGKey::HMACMD5_NAME(),
+                                           &dummy_data[0],
+                                           dummy_data.size())));
+    {
+        SCOPED_TRACE("Verify resulting in BADTIME");
+        commonVerifyChecks(*tsig_ctx, message.getTSIGRecord(),
+                           &received_data[0], received_data.size(),
+                           TSIGError::BAD_TIME(),
+                           TSIGContext::RECEIVED_REQUEST);
+    }
+    EXPECT_EQ(91, tsig_ctx->getTSIGLength());
+}
+
 } // end namespace

src/lib/dns/tsig.cc

@@ -58,10 +58,32 @@ getTSIGTime() {
 }
 
 struct TSIGContext::TSIGContextImpl {
-    TSIGContextImpl(const TSIGKey& key) :
-        state_(INIT), key_(key), error_(Rcode::NOERROR()),
-        previous_timesigned_(0)
-    {}
+    TSIGContextImpl(const TSIGKey& key,
+                    TSIGError error = TSIGError::NOERROR()) :
+        state_(INIT), key_(key), error_(error),
+        previous_timesigned_(0), digest_len_(0)
+    {
+        if (error == TSIGError::NOERROR()) {
+            // In normal (NOERROR) case, the key should be valid, and we
+            // should be able to pre-create a corresponding HMAC object,
+            // which will be likely to be used for sign or verify later.
+            // We do this in the constructor so that we can know the expected
+            // digest length in advance.  The creation should normally succeed,
+            // but the key information could be still broken, which could
+            // trigger an exception inside the cryptolink module.  We ignore
+            // it at this moment; a subsequent sign/verify operation will try
+            // to create the HMAC, which would also fail.
+            try {
+                hmac_.reset(CryptoLink::getCryptoLink().createHMAC(
+                                key_.getSecret(), key_.getSecretLength(),
+                                key_.getAlgorithm()),
+                            deleteHMAC);
+            } catch (const Exception&) {
+                return;
+            }
+            digest_len_ = hmac_->getOutputLength();
+        }
+    }
 
     // This helper method is used from verify().  It's expected to be called
     // just before verify() returns.  It updates internal state based on
@@ -85,6 +107,23 @@ struct TSIGContext::TSIGContextImpl {
         return (error);
     }
 
+    // A shortcut method to create an HMAC object for sign/verify.  If one
+    // has been successfully created in the constructor, return it; otherwise
+    // create a new one and return it.  In the former case, the ownership is
+    // transferred to the caller; the stored HMAC will be reset after the
+    // call.
+    HMACPtr createHMAC() {
+        if (hmac_) {
+            HMACPtr ret = HMACPtr();
+            ret.swap(hmac_);
+            return (ret);
+        }
+        return (HMACPtr(CryptoLink::getCryptoLink().createHMAC(
+                            key_.getSecret(), key_.getSecretLength(),
+                            key_.getAlgorithm()),
+                        deleteHMAC));
+    }
+
     // The following three are helper methods to compute the digest for
     // TSIG sign/verify in order to unify the common code logic for sign()
     // and verify() and to keep these callers concise.
@@ -111,6 +150,8 @@ struct TSIGContext::TSIGContextImpl {
     vector<uint8_t> previous_digest_;
     TSIGError error_;
     uint64_t previous_timesigned_; // only meaningful for response with BADTIME
+    size_t digest_len_;
+    HMACPtr hmac_;
 };
 
 void
@@ -221,8 +262,7 @@ TSIGContext::TSIGContext(const Name& key_name, const Name& algorithm_name,
         // be used in subsequent response with a TSIG indicating a BADKEY
         // error.
         impl_ = new TSIGContextImpl(TSIGKey(key_name, algorithm_name,
-                                            NULL, 0));
-        impl_->error_ = TSIGError::BAD_KEY();
+                                            NULL, 0), TSIGError::BAD_KEY());
     } else {
         impl_ = new TSIGContextImpl(*result.key);
     }
@@ -232,6 +272,45 @@ TSIGContext::~TSIGContext() {
     delete impl_;
 }
 
+size_t
+TSIGContext::getTSIGLength() const {
+    //
+    // The space required for an TSIG record is:
+    //
+    //	n1 bytes for the (key) name
+    //	2 bytes for the type
+    //	2 bytes for the class
+    //	4 bytes for the ttl
+    //	2 bytes for the rdlength
+    //	n2 bytes for the algorithm name
+    //	6 bytes for the time signed
+    //	2 bytes for the fudge
+    //	2 bytes for the MAC size
+    //	x bytes for the MAC
+    //	2 bytes for the original id
+    //	2 bytes for the error
+    //	2 bytes for the other data length
+    //	y bytes for the other data (at most)
+    // ---------------------------------
+    //     26 + n1 + n2 + x + y bytes
+    //
+
+    // Normally the digest length ("x") is the length of the underlying
+    // hash output.  If a key related error occurred, however, the
+    // corresponding TSIG will be "unsigned", and the digest length will be 0.
+    const size_t digest_len =
+        (impl_->error_ == TSIGError::BAD_KEY() ||
+         impl_->error_ == TSIGError::BAD_SIG()) ? 0 : impl_->digest_len_;
+
+    // Other Len ("y") is normally 0; if BAD_TIME error occurred, the
+    // subsequent TSIG will contain 48 bits of the server current time.
+    const size_t other_len = (impl_->error_ == TSIGError::BAD_TIME()) ? 6 : 0;
+
+    return (26 + impl_->key_.getKeyName().getLength() +
+            impl_->key_.getAlgorithmName().getLength() +
+            digest_len + other_len);
+}
+
 TSIGContext::State
 TSIGContext::getState() const {
     return (impl_->state_);
@@ -276,11 +355,7 @@ TSIGContext::sign(const uint16_t qid, const void* const data,
         return (tsig);
     }
 
-    HMACPtr hmac(CryptoLink::getCryptoLink().createHMAC(
-                     impl_->key_.getSecret(),
-                     impl_->key_.getSecretLength(),
-                     impl_->key_.getAlgorithm()),
-                 deleteHMAC);
+    HMACPtr hmac(impl_->createHMAC());
 
     // If the context has previous MAC (either the Request MAC or its own
     // previous MAC), digest it.
@@ -406,11 +481,7 @@ TSIGContext::verify(const TSIGRecord* const record, const void* const data,
         return (impl_->postVerifyUpdate(error, NULL, 0));
     }
 
-    HMACPtr hmac(CryptoLink::getCryptoLink().createHMAC(
-                     impl_->key_.getSecret(),
-                     impl_->key_.getSecretLength(),
-                     impl_->key_.getAlgorithm()),
-                 deleteHMAC);
+    HMACPtr hmac(impl_->createHMAC());
 
     // If the context has previous MAC (either the Request MAC or its own
     // previous MAC), digest it.

src/lib/dns/tsig.h

@@ -353,6 +353,27 @@ public:
     TSIGError verify(const TSIGRecord* const record, const void* const data,
                      const size_t data_len);
 
+    /// Return the expected length of TSIG RR after \c sign()
+    ///
+    /// This method returns the length of the TSIG RR that would be
+    /// produced as a result of \c sign() with the state of the context
+    /// at the time of the call.  The expected length can be decided
+    /// from the key and the algorithm (which determines the MAC size if
+    /// included) and the recorded TSIG error.  Specifically, if a key
+    /// related error has been identified, the MAC will be excluded; if
+    /// a time error has occurred, the TSIG will include "other data".
+    ///
+    /// This method is provided mainly for the convenience of the Message
+    /// class, which needs to know the expected TSIG length in rendering a
+    /// signed DNS message so that it can handle truncated messages with TSIG
+    /// correctly.  Normal applications wouldn't need this method.  The Python
+    /// binding for this method won't be provided for the same reason.
+    ///
+    /// \exception None
+    ///
+    /// \return The expected TISG RR length in bytes
+    size_t getTSIGLength() const;
+
     /// Return the current state of the context
     ///
     /// \note