Commit 9260b6d9 authored by Thomas Markwalder's avatar Thomas Markwalder

[#730,!2] Corrected assertion failure on malformed hostname from kea-dhcp4

src/lib/exceptions/isc_assert.h
    New file that defines isc_assert_throw()

src/lib/exceptions/Makefile.am
    added isc_asssert.h

src/lib/exceptions/tests/exceptions_unittest.cc
    TEST(IscThrowAssert, checkMessage) - new test

src/lib/dns/labelsequence.cc
src/lib/dns/name.cc
    Replaced assert() calls with isc_throw_assert() calls

src/lib/dns/tests/name_unittest.cc
    TEST_F(NameTest, unexpectedParseError) - new unit test
    for hostname option content based on fuzz test failure

src/lib/dhcp_ddns/ncr_msg.cc
    Removed unnecessary include of dns/name.h

src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::processHostnameOption() - added try catch
    around OptionDataTypeUtil::getLabelCount() call

src/bin/dhcp4/dhcp4_messages.*
    added new log message DHCP4_CLIENT_HOSTNAME_MALFORMED

src/bin/dhcp4/tests/fqdn_unittest.cc
    TEST_F(NameDhcpv4SrvTest, serverUpdateMalformedHostname) -
    renamed test and augmented it to include the packet of death
    produced by fuzz testing.
parent 7fe5b82a
// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Tue Jul 16 2019 11:03
// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Wed Jul 10 2019 15:10
#include <cstddef>
#include <log/message_types.h>
......@@ -23,6 +23,7 @@ extern const isc::log::MessageID DHCP4_CLIENTID_IGNORED_FOR_LEASES = "DHCP4_CLIE
extern const isc::log::MessageID DHCP4_CLIENT_FQDN_DATA = "DHCP4_CLIENT_FQDN_DATA";
extern const isc::log::MessageID DHCP4_CLIENT_FQDN_PROCESS = "DHCP4_CLIENT_FQDN_PROCESS";
extern const isc::log::MessageID DHCP4_CLIENT_HOSTNAME_DATA = "DHCP4_CLIENT_HOSTNAME_DATA";
extern const isc::log::MessageID DHCP4_CLIENT_HOSTNAME_MALFORMED = "DHCP4_CLIENT_HOSTNAME_MALFORMED";
extern const isc::log::MessageID DHCP4_CLIENT_HOSTNAME_PROCESS = "DHCP4_CLIENT_HOSTNAME_PROCESS";
extern const isc::log::MessageID DHCP4_CLIENT_NAME_PROC_FAIL = "DHCP4_CLIENT_NAME_PROC_FAIL";
extern const isc::log::MessageID DHCP4_COMMAND_RECEIVED = "DHCP4_COMMAND_RECEIVED";
......@@ -159,6 +160,7 @@ const char* values[] = {
"DHCP4_CLIENT_FQDN_DATA", "%1: Client sent FQDN option: %2",
"DHCP4_CLIENT_FQDN_PROCESS", "%1: processing Client FQDN option",
"DHCP4_CLIENT_HOSTNAME_DATA", "%1: client sent Hostname option: %2",
"DHCP4_CLIENT_HOSTNAME_MALFORMED", "%1: client hostname option malformed: %2",
"DHCP4_CLIENT_HOSTNAME_PROCESS", "%1: processing client's Hostname option",
"DHCP4_CLIENT_NAME_PROC_FAIL", "%1: failed to process the fqdn or hostname sent by a client: %2",
"DHCP4_COMMAND_RECEIVED", "received command %1, arguments: %2",
......
// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Tue Jul 16 2019 11:03
// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Wed Jul 10 2019 15:10
#ifndef DHCP4_MESSAGES_H
#define DHCP4_MESSAGES_H
......@@ -24,6 +24,7 @@ extern const isc::log::MessageID DHCP4_CLIENTID_IGNORED_FOR_LEASES;
extern const isc::log::MessageID DHCP4_CLIENT_FQDN_DATA;
extern const isc::log::MessageID DHCP4_CLIENT_FQDN_PROCESS;
extern const isc::log::MessageID DHCP4_CLIENT_HOSTNAME_DATA;
extern const isc::log::MessageID DHCP4_CLIENT_HOSTNAME_MALFORMED;
extern const isc::log::MessageID DHCP4_CLIENT_HOSTNAME_PROCESS;
extern const isc::log::MessageID DHCP4_CLIENT_NAME_PROC_FAIL;
extern const isc::log::MessageID DHCP4_COMMAND_RECEIVED;
......
......@@ -112,6 +112,13 @@ client and transaction identification information. The second argument
specifies the hostname carried in the Hostname option sent by the
client.
% DHCP4_CLIENT_HOSTNAME_MALFORMED %1: client hostname option malformed: %2
This debug message is issued when the DHCP server was unable to process the
the hostname option sent by the client because the content is malformed.
The first argument includes the client and transaction identification
information. The second argument should contain a description of the data
error.
% DHCP4_CLIENT_HOSTNAME_PROCESS %1: processing client's Hostname option
This debug message is issued when the server starts processing the Hostname
option sent in the client's query. The argument includes the client and
......
......@@ -1817,7 +1817,19 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) {
.arg(opt_hostname->getValue());
std::string hostname = isc::util::str::trim(opt_hostname->getValue());
unsigned int label_count = OptionDataTypeUtil::getLabelCount(hostname);
unsigned int label_count;
try {
// Parsing into labels can throw on malformed content so we're
// going to explicitly catch that here.
label_count = OptionDataTypeUtil::getLabelCount(hostname);
} catch (const std::exception& exc) {
LOG_DEBUG(ddns4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_CLIENT_HOSTNAME_MALFORMED)
.arg(ex.getQuery()->getLabel())
.arg(exc.what());
return;
}
// The hostname option sent by the client should be at least 1 octet long.
// If it isn't we ignore this option. (Per RFC 2131, section 3.14)
/// @todo It would be more liberal to accept this and let it fall into
......
......@@ -851,14 +851,42 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateHostname) {
}
// Test that the server skips processing of a wrong Hostname option.
TEST_F(NameDhcpv4SrvTest, serverUpdateWrongHostname) {
// Test that the server skips processing of a mal-formed Hostname options.
TEST_F(NameDhcpv4SrvTest, serverUpdateMalformedHostname) {
Pkt4Ptr query;
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.
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,
0x00,0x00,0x00,0x19,0x0c,0x4e,0x01,0x00,0x07,0x08,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x35,0x01,0x05,0x3a,0x04,0x00,0x00,0x07,0x08,0x3b,0x04,0x00,
0x00,0x2e,0x3b,0x04,0x00,0x19,0x2e,0x00,0x00,0x00,0x0a,0x00,0x12,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x0b,0x82,0x01,0xfc,0x42,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x35,0x01,0x05,0x3a,0x04,0x00,0x00,0x07,0x08,0x3b,0x04,
0x00,0x00,0x2e,0x3b,0x04,0x00,0x19,0x2e,0x56,0x00,0x00,0x0a,0x00,0x12,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x0b,0x82,0x01,0xfc,0x42,0x00,0x00,0x00,0x00,0x19,0x0c,
0x4e,0x01,0x05,0x3a,0x04,0xde,0x00,0x07,0x08,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x35,0x01,0x05,0x3a,0x07,0x08,0x3b,0x04,0x00,0x00,0x2e,0x3b,0x04,
0x00,0x19,0x2e,0x56,0x40,0x00,0x00,0x00,0x00,0x00,0x0a,0x00,0x12,0x00,0x00,0x00,
0x00,0x00,0x19,0x00,0x0b,0x82,0x01,0xfc,0x42,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0xfc,0xff,0xff,0xff,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x35,0x01,0x05,0xff,0xff,0x05,0x00,0x07,0x08,0x3b,0x04,
0x00,0x00,0x2e,0x3b
};
std::string badnamestr(badname.begin(), badname.end());
ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST,
badnamestr));
ASSERT_NO_THROW(hostname = processHostname(query));
EXPECT_FALSE(hostname);
}
// Test that the server does not see an empty Hostname option.
......
......@@ -7,7 +7,6 @@
#include <config.h>
#include <dhcp_ddns/ncr_msg.h>
#include <dns/name.h>
#include <asiolink/io_address.h>
#include <asiolink/io_error.h>
#include <cryptolink/cryptolink.h>
......
// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -9,6 +9,7 @@
#include <dns/labelsequence.h>
#include <dns/name_internal.h>
#include <exceptions/exceptions.h>
#include <exceptions/isc_assert.h>
#include <boost/functional/hash.hpp>
......@@ -116,7 +117,7 @@ LabelSequence::serialize(void* buf, size_t buf_len) const {
}
const size_t offsets_len = getLabelCount();
assert(offsets_len < 256); // should be in the 8-bit range
isc_throw_assert(offsets_len < 256); // should be in the 8-bit range
// Overridden check. Buffer shouldn't overwrap the offset of name data
// regions.
......@@ -134,7 +135,7 @@ LabelSequence::serialize(void* buf, size_t buf_len) const {
std::memcpy(bp, &data_[offsets_[first_label_]], ndata_len);
bp += ndata_len;
assert(bp - reinterpret_cast<const uint8_t*>(buf) == expected_size);
isc_throw_assert(bp - reinterpret_cast<const uint8_t*>(buf) == expected_size);
}
bool
......@@ -189,7 +190,7 @@ LabelSequence::compare(const LabelSequence& other,
// We don't support any extended label types including now-obsolete
// bitstring labels.
assert(count1 <= Name::MAX_LABELLEN && count2 <= Name::MAX_LABELLEN);
isc_throw_assert(count1 <= Name::MAX_LABELLEN && count2 <= Name::MAX_LABELLEN);
const int cdiff = static_cast<int>(count1) - static_cast<int>(count2);
unsigned int count = (cdiff < 0) ? count1 : count2;
......@@ -309,7 +310,7 @@ LabelSequence::toRawText(bool omit_final_dot) const {
}
if (count <= Name::MAX_LABELLEN) {
assert(np_end - np >= count);
isc_throw_assert(np_end - np >= count);
if (!result.empty()) {
// just after a non-empty label. add a separating dot.
......@@ -326,8 +327,8 @@ LabelSequence::toRawText(bool omit_final_dot) const {
}
// We should be at the end of the data and have consumed all labels.
assert(np == np_end);
assert(labels == 0);
isc_throw_assert(np == np_end);
isc_throw_assert(labels == 0);
return (result);
}
......@@ -363,7 +364,7 @@ LabelSequence::toText(bool omit_final_dot) const {
}
if (count <= Name::MAX_LABELLEN) {
assert(np_end - np >= count);
isc_throw_assert(np_end - np >= count);
if (!result.empty()) {
// just after a non-empty label. add a separating dot.
......@@ -404,8 +405,8 @@ LabelSequence::toText(bool omit_final_dot) const {
}
// We should be at the end of the data and have consumed all labels.
assert(np == np_end);
assert(labels == 0);
isc_throw_assert(np == np_end);
isc_throw_assert(labels == 0);
return (result);
}
......
......@@ -7,13 +7,13 @@
#include <config.h>
#include <cctype>
#include <cassert>
#include <iterator>
#include <functional>
#include <vector>
#include <iostream>
#include <algorithm>
#include <exceptions/isc_assert.h>
#include <util/buffer.h>
#include <dns/exceptions.h>
#include <dns/name.h>
......@@ -189,7 +189,7 @@ stringParse(Iterator s, Iterator send, bool downcase, Offsets& offsets,
break;
}
state = ft_ordinary;
assert(ndata.size() < Name::MAX_WIRE);
isc_throw_assert(ndata.size() < Name::MAX_WIRE);
// FALLTHROUGH
case ft_ordinary:
if (c == '.') {
......@@ -261,7 +261,7 @@ stringParse(Iterator s, Iterator send, bool downcase, Offsets& offsets,
break;
default:
// impossible case
assert(false);
isc_throw_assert(false);
}
}
......@@ -271,14 +271,14 @@ stringParse(Iterator s, Iterator send, bool downcase, Offsets& offsets,
"name is too long for termination in " <<
string(orig_s, send));
}
assert(s == send);
isc_throw_assert(s == send);
if (state != ft_ordinary) {
isc_throw(IncompleteName,
"incomplete textual name in " <<
(empty ? "<empty>" : string(orig_s, send)));
}
if (state == ft_ordinary) {
assert(count != 0);
isc_throw_assert(count != 0);
ndata.at(offsets.back()) = count;
offsets.push_back(ndata.size());
......@@ -304,7 +304,7 @@ Name::Name(const std::string &namestring, bool downcase) {
// And get the output
labelcount_ = offsets.size();
assert(labelcount_ > 0 && labelcount_ <= Name::MAX_LABELS);
isc_throw_assert(labelcount_ > 0 && labelcount_ <= Name::MAX_LABELS);
ndata_.assign(ndata.data(), ndata.size());
length_ = ndata_.size();
offsets_.assign(offsets.begin(), offsets.end());
......@@ -338,7 +338,7 @@ Name::Name(const char* namedata, size_t data_len, const Name* origin,
// Get the output
labelcount_ = offsets.size();
assert(labelcount_ > 0 && labelcount_ <= Name::MAX_LABELS);
isc_throw_assert(labelcount_ > 0 && labelcount_ <= Name::MAX_LABELS);
ndata_.assign(ndata.data(), ndata.size());
length_ = ndata_.size();
offsets_.assign(offsets.begin(), offsets.end());
......@@ -479,7 +479,7 @@ Name::Name(InputBuffer& buffer, bool downcase) {
state = fw_start;
break;
default:
assert(false);
isc_throw_assert(false);
}
}
......@@ -576,8 +576,8 @@ Name::isWildcard() const {
Name
Name::concatenate(const Name& suffix) const {
assert(length_ > 0 && suffix.length_ > 0);
assert(labelcount_ > 0 && suffix.labelcount_ > 0);
isc_throw_assert(length_ > 0 && suffix.length_ > 0);
isc_throw_assert(labelcount_ > 0 && suffix.labelcount_ > 0);
unsigned int length = length_ + suffix.length_ - 1;
if (length > Name::MAX_WIRE) {
......@@ -589,7 +589,7 @@ Name::concatenate(const Name& suffix) const {
retname.ndata_.assign(ndata_, 0, length_ - 1);
retname.ndata_.insert(retname.ndata_.end(),
suffix.ndata_.begin(), suffix.ndata_.end());
assert(retname.ndata_.size() == length);
isc_throw_assert(retname.ndata_.size() == length);
retname.length_ = length;
//
......@@ -598,13 +598,13 @@ Name::concatenate(const Name& suffix) const {
// suffix name with the additional offset of the length of the prefix.
//
unsigned int labels = labelcount_ + suffix.labelcount_ - 1;
assert(labels <= Name::MAX_LABELS);
isc_throw_assert(labels <= Name::MAX_LABELS);
retname.offsets_.reserve(labels);
retname.offsets_.assign(&offsets_[0], &offsets_[0] + labelcount_ - 1);
transform(suffix.offsets_.begin(), suffix.offsets_.end(),
back_inserter(retname.offsets_),
bind2nd(plus<char>(), length_ - 1));
assert(retname.offsets_.size() == labels);
isc_throw_assert(retname.offsets_.size() == labels);
retname.labelcount_ = labels;
return (retname);
......@@ -671,7 +671,7 @@ Name::split(const unsigned int first, const unsigned int n) const {
retname.length_ = retname.ndata_.size();
retname.labelcount_ = retname.offsets_.size();
assert(retname.labelcount_ == newlabels);
isc_throw_assert(retname.labelcount_ == newlabels);
return (retname);
}
......@@ -699,8 +699,8 @@ Name::downcase() {
// we assume a valid name, and do abort() if the assumption fails
// rather than throwing an exception.
unsigned int count = ndata_.at(pos++);
assert(count <= MAX_LABELLEN);
assert(nlen >= count);
isc_throw_assert(count <= MAX_LABELLEN);
isc_throw_assert(nlen >= count);
while (count > 0) {
ndata_.at(pos) =
......
......@@ -289,6 +289,35 @@ TEST_F(NameTest, fromText) {
EXPECT_EQ(Name::MAX_LABELS, maxlabels.getLabelCount());
}
// The following test uses a name data that was produced by
// fuz testing and causes an unexpected condition in stringParser.
// Formerly this condition was trapped by an assert, but for
// robustness it has been replaced by a throw.
TEST_F(NameTest, unexpectedParseError) {
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,
0x00,0x00,0x00,0x19,0x0c,0x4e,0x01,0x00,0x07,0x08,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x35,0x01,0x05,0x3a,0x04,0x00,0x00,0x07,0x08,0x3b,0x04,0x00,
0x00,0x2e,0x3b,0x04,0x00,0x19,0x2e,0x00,0x00,0x00,0x0a,0x00,0x12,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x0b,0x82,0x01,0xfc,0x42,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x35,0x01,0x05,0x3a,0x04,0x00,0x00,0x07,0x08,0x3b,0x04,
0x00,0x00,0x2e,0x3b,0x04,0x00,0x19,0x2e,0x56,0x00,0x00,0x0a,0x00,0x12,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x0b,0x82,0x01,0xfc,0x42,0x00,0x00,0x00,0x00,0x19,0x0c,
0x4e,0x01,0x05,0x3a,0x04,0xde,0x00,0x07,0x08,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x35,0x01,0x05,0x3a,0x07,0x08,0x3b,0x04,0x00,0x00,0x2e,0x3b,0x04,
0x00,0x19,0x2e,0x56,0x40,0x00,0x00,0x00,0x00,0x00,0x0a,0x00,0x12,0x00,0x00,0x00,
0x00,0x00,0x19,0x00,0x0b,0x82,0x01,0xfc,0x42,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0xfc,0xff,0xff,0xff,0x00,0x00,0x00,0x00,0x00,0x00,0x00,
0x00,0x00,0x00,0x00,0x00,0x35,0x01,0x05,0xff,0xff,0x05,0x00,0x07,0x08,0x3b,0x04,
0x00,0x00,0x2e,0x3b
};
std::string badnamestr(badname.begin(), badname.end());
EXPECT_THROW(Name(badnamestr, false), Unexpected);
}
// on the rest while we prepare it.
// Check the @ syntax is accepted and it just copies the origin.
TEST_F(NameTest, copyOrigin) {
......
......@@ -6,9 +6,11 @@ AM_CXXFLAGS=$(KEA_CXXFLAGS)
lib_LTLIBRARIES = libkea-exceptions.la
libkea_exceptions_la_SOURCES = exceptions.h exceptions.cc
libkea_exceptions_la_SOURCES += isc_assert.h
libkea_exceptions_la_LDFLAGS = -no-undefined -version-info 2:0:2
CLEANFILES = *.gcno *.gcda
libkea_exceptions_includedir = $(pkgincludedir)/exceptions
libkea_exceptions_include_HEADERS = exceptions.h
libkea_exceptions_include_HEADERS += isc_assert.h
// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.
#ifndef ISC_ASSERT_H
#define ISC_ASSERT_H
#include <exceptions/exceptions.h>
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.
#define isc_throw_assert(expr) \
{\
if(!(static_cast<bool>(expr))) \
{\
isc_throw(isc::Unexpected, __FILE__ << ":" << __LINE__ << " (" << #expr << ") failed");\
}\
}
}
#endif // ISC_ASSERT_H
// Copyright (C) 2009-2015,2017 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2009-2019 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
......@@ -10,6 +10,7 @@
#include <string>
#include <exceptions/exceptions.h>
#include <exceptions/isc_assert.h>
#include <sstream>
#include <gtest/gtest.h>
......@@ -95,4 +96,19 @@ TEST_F(ExceptionTest, message) {
}
}
// 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));
try {
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);
}
}
}
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