[#730,!2] Addressed review comments

ChangeLog - added an entry

src/bin/dhcp4/tests/fqdn_unittest.cc
    TEST_F(NameDhcpv4SrvTest, serverUpdateMalformedHostname) - added
    commentary

src/lib/exceptions/isc_assert.h
    commentary changes

src/lib/exceptions/tests/exceptions_unittest.cc
    TEST(IscThrowAssert, checkMessage) - replace use of explicit line number

diff --git a/ChangeLog b/ChangeLog
index f99a7d06e77b553cb28f74efd2a7b73fd4d76380..abc3cd283573c9258ff4ed14f56efea8c3a836bf 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+1652.  [security]              tmark
+       Replaced asserts with exception throws to catch parser errors
+       that can occur handling malformed hostname name and FQDN options.
+       CVE:2019-6473
+       (Gitlab #730,!2-p git TBD)
+
 1651.  [security]              tmark
        Added logic to kea-dhcp6 to catch values for client or
        server DUIDs that exceed 128 bytes to inbound packet

diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc
index ea224a4f451964b81990c20aade049293704622c..5e705bed385d125f1edfa35e91d9395b4f2277ee 100644 (file)
--- a/src/bin/dhcp4/tests/fqdn_unittest.cc
+++ b/src/bin/dhcp4/tests/fqdn_unittest.cc
@@ -852,16 +852,21 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateHostname) {
 }
 
 // Test that the server skips processing of a mal-formed Hostname options.
+// - First scenario the hostname has an empty label
+// - Second scenario the hostname option causes an internal parsing error
+// in dns::Name(). The content was created by fuzz testing.
 TEST_F(NameDhcpv4SrvTest, serverUpdateMalformedHostname) {
     Pkt4Ptr query;
+
+    // Hostname should not be able to have an emtpy label.
     ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST,
                                                     "abc..example.com"));
     OptionStringPtr hostname;
     ASSERT_NO_THROW(hostname = processHostname(query));
     EXPECT_FALSE(hostname);
 
-    // The following vector matches a malformed hostname data produced by
-    // that caused the server to assert.
+    // The following vector matches malformed hostname data produced by
+    // fuzz testing that causes an internal error in dns::Name parsing logic.
     std::vector<uint8_t> badname  {
         0xff,0xff,0x7f,0x00,0x00,0x00,0x7f,0x00,0x00,0x00,0x00,
         0x00,0x00,0x04,0x63,0x82,0x53,0x63,0x35,0x01,0x01,0x3d,0x07,0x01,0x00,0x00,0x00,

diff --git a/src/lib/exceptions/isc_assert.h b/src/lib/exceptions/isc_assert.h
index 4566c38d006a10b9310ff776cd145267654d92f1..3600cde5c9bdffc62ecab1d796ae9c0310d5749c 100644 (file)
--- a/src/lib/exceptions/isc_assert.h
+++ b/src/lib/exceptions/isc_assert.h
@@ -12,8 +12,8 @@
 namespace isc {
 
 /// @brief Replacement for assert() that throws if the expression is false.
-/// It exists because some of the original code has asserts and we'd rather
-/// they throw than crash the server or be compiled out.  
+/// It exists because some of the original code has asserts and we prefer to
+/// throw rather than crash the server or be compile out asserts.
 #define isc_throw_assert(expr) \
 {\
     if(!(static_cast<bool>(expr))) \

diff --git a/src/lib/exceptions/tests/exceptions_unittest.cc b/src/lib/exceptions/tests/exceptions_unittest.cc
index 119f761a3aab0e5c93eca6c48b6c743812717e57..c5423d2e205c26bed2ff61a29f57a71276ee544c 100644 (file)
--- a/src/lib/exceptions/tests/exceptions_unittest.cc
+++ b/src/lib/exceptions/tests/exceptions_unittest.cc
@@ -96,18 +96,21 @@ TEST_F(ExceptionTest, message) {
     }
 }
 
-// Sanity check that ISC_THROW_ASSERT macro operates correctly.
+// Sanity check that 'isc_throw_assert' macro operates correctly.
 TEST(IscThrowAssert, checkMessage) {
     int m = 5;
     int n = 7;
 
     ASSERT_NO_THROW(isc_throw_assert(m == m));
 
+    int line_no;
     try {
-        isc_throw_assert(m == n);
+        line_no = __LINE__; isc_throw_assert(m == n);
     } catch (const std::exception& ex) {
         std::string msg = ex.what();
-        EXPECT_EQ("exceptions_unittest.cc:107 (m == n) failed", msg);
+        std::ostringstream os;
+        os << __FILE__  << ":" << line_no << " (m == n) failed";
+        EXPECT_EQ(os.str(), msg);
     }
 }