From: Thomas Markwalder <tmark@isc.org>
Date: Wed, 24 Jul 2019 19:00:33 +0000 (-0400)
Subject: [#730,!2] Addressed review comments
X-Git-Tag: Kea-1.6.0~41^2~24
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=27fbe6f4bda684105a19674fd841ea6ee9f08fa0;p=thirdparty%2Fkea.git

[#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 f99a7d06e7..abc3cd2835 100644
--- 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 ea224a4f45..5e705bed38 100644
--- 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 4566c38d00..3600cde5c9 100644
--- 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 119f761a3a..c5423d2e20 100644
--- 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);
     }
 }