Commit 27fbe6f4 authored by Thomas Markwalder's avatar Thomas Markwalder

[#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
parent 9260b6d9
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
......
......@@ -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,
......
......@@ -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))) \
......
......@@ -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);
}
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment