Commit 55dab872 authored by Thomas Markwalder's avatar Thomas Markwalder

[#1048] Enable host sanitizing by default

src/lib/util/strutil.cc
    StringSanitizerImpl::scrub() - modified non-USE_REGEX code to
    handle embedded nuls.

src/lib/util/tests/strutil_unittest.cc
    Updated tests

src/bin/dhcp4/tests/fqdn_unittest.cc
src/bin/dhcp4/tests/get_config_unittest.cc
src/bin/dhcp6/tests/fqdn_unittest.cc
src/bin/dhcp6/tests/get_config_unittest.cc
    updated testing

src/lib/dhcpsrv/parsers/simple_parser4.cc
src/lib/dhcpsrv/parsers/simple_parser6.cc
    Added global default values

doc/sphinx/arm/dhcp6-srv.rst
doc/sphinx/arm/dhcp4-srv.rst
    Updated sanitizing text
parent 24e7b45c
......@@ -3236,15 +3236,22 @@ and '-'. This may be accomplished with the following two parameters:
- ``hostname-char-set`` - a regular expression describing the invalid
character set. This can be any valid, regular expression using POSIX
extended expression syntax. For example, "[^A-Za-z0-9-]" would
replace any character other than the letters A through z, digits 0
through 9, and '-'. An empty string, the default value, disables
sanitization.
extended expression syntax. Embedded nuls (0x00) will always be
considered an invalid character to be replaced (or omitted).
- ``hostname-char-replacement`` - a string of zero or more characters
with which to replace each invalid character in the host name. The
default value is an empty string and will cause invalid characters to
be OMITTED rather than replaced.
with which to replace each invalid character in the host name. An empty
string and will cause invalid characters to be OMITTED rather than replaced.
.. note::
Starting with Kea 1.7.5, the default values are as follows:
"hostname-char-set": "[^A-Za-z0-9.-]",
"hostname-char-replacement": ""
This enables sanitizing and will omit any character that is not
a letter,digit, hyphen, dot or nul.
The following configuration will replace anything other than a letter,
digit, hyphen, or dot with the letter 'x':
......
......@@ -2964,15 +2964,22 @@ accomplished with the following two parameters:
- ``hostname-char-set`` - a regular expression describing the invalid
character set. This can be any valid, regular expression using POSIX
extended expression syntax. For example, "[^A-Za-z0-9-]" would
replace any character other than the letters A through z, digits 0
through 9, and '-'. An empty string, the default value, disables
sanitization.
extended expression syntax. Embedded nuls (0x00) will always be
considered an invalid character to be replaced (or omitted).
- ``hostname-char-replacement`` - a string of zero or more characters
with which to replace each invalid character in the client value. The
default value is an empty string and will cause invalid characters to
be OMITTED rather than replaced.
with which to replace each invalid character in the host name. An empty
string and will cause invalid characters to be OMITTED rather than replaced.
.. note::
Starting with Kea 1.7.5, the default values are as follows:
"hostname-char-set": "[^A-Za-z0-9.-]",
"hostname-char-replacement": ""
This enables sanitizing and will omit any character that is not
a letter,digit, hyphen, dot or nul.
The following configuration will replace anything other than a letter,
digit, hyphen, or dot with the letter 'x':
......
......@@ -15,6 +15,7 @@
#include <dhcpsrv/cfgmgr.h>
#include <dhcpsrv/lease_mgr.h>
#include <dhcpsrv/lease_mgr_factory.h>
#include <testutils/gtest_utils.h>
#include <gtest/gtest.h>
#include <boost/scoped_ptr.hpp>
......@@ -1844,6 +1845,74 @@ TEST_F(NameDhcpv4SrvTest, replaceClientNameModeTest) {
CLIENT_NAME_PRESENT, NAME_NOT_REPLACED);
}
// Verifies that default hostname-char-set sanitizes Hostname option
// values received from clients.
TEST_F(NameDhcpv4SrvTest, sanitizeHostDefault) {
Dhcp4Client client(Dhcp4Client::SELECTING);
// Configure DHCP server.
configure(CONFIGS[2], *client.getServer());
// Make sure that DDNS is not enabled.
ASSERT_FALSE(CfgMgr::instance().ddnsEnabled());
struct Scenario {
std::string description_;
std::string original_;
std::string sanitized_;
};
std::vector<Scenario> scenarios = {
{
"unqualified host name with invalid characters",
"one-&$_-host",
"one--host.fake-suffix.isc.org"
},
{
"qualified host name with invalid characters",
"two--host.other.org",
"two--host.other.org"
},
{
"unqualified host name with all valid characters",
"three-ok-host",
"three-ok-host.fake-suffix.isc.org"
},
{
"qualified host name with valid characters",
"four-ok-host.other.org",
"four-ok-host.other.org"
},
{
"qualified host name with nuls",
std::string("four-ok-host\000.other.org",23),
"four-ok-host.other.org"
}
};
Pkt4Ptr resp;
OptionStringPtr hostname;
for (auto scenario : scenarios) {
SCOPED_TRACE((scenario).description_);
{
// Set the hostname option.
ASSERT_NO_THROW(client.includeHostname((scenario).original_));
// Send the DHCPDISCOVER and make sure that the server responded.
ASSERT_NO_THROW(client.doDiscover());
resp = client.getContext().response_;
ASSERT_TRUE(resp);
ASSERT_EQ(DHCPOFFER, static_cast<int>(resp->getType()));
// Make sure the response hostname is what we expect.
hostname = boost::dynamic_pointer_cast<OptionString>(resp->getOption(DHO_HOST_NAME));
ASSERT_TRUE(hostname);
EXPECT_EQ((scenario).sanitized_, hostname->getValue());
}
}
}
// Verifies that setting hostname-char-set sanitizes Hostname option
// values received from clients.
TEST_F(NameDhcpv4SrvTest, sanitizeHost) {
......@@ -1882,6 +1951,11 @@ TEST_F(NameDhcpv4SrvTest, sanitizeHost) {
"qualified host name with valid characters",
"four-ok-host.other.org",
"four-ok-host.other.org"
},
{
"qualified host name with nuls",
std::string("four-ok-host\000.other.org",23),
"four-ok-hostx.other.org"
}
};
......@@ -1891,7 +1965,7 @@ TEST_F(NameDhcpv4SrvTest, sanitizeHost) {
SCOPED_TRACE((scenario).description_);
{
// Set the hostname option.
ASSERT_NO_THROW(client.includeHostname((scenario).original_));
ASSERT_NO_THROW_LOG(client.includeHostname((scenario).original_));
// Send the DHCPDISCOVER and make sure that the server responded.
ASSERT_NO_THROW(client.doDiscover());
......@@ -2080,6 +2154,12 @@ TEST_F(NameDhcpv4SrvTest, sanitizeFqdnGlobal) {
"four-ok-host.other.org",
Option4ClientFqdn::FULL,
"four-ok-host.other.org."
},
{
"qualified FQDN name with nuls",
std::string("four-ok-host.ot\000\000her.org", 24),
Option4ClientFqdn::FULL,
"four-ok-host.otxxher.org."
}
};
......
This diff is collapsed.
......@@ -1584,6 +1584,13 @@ TEST_F(FqdnDhcpv6SrvTest, sanitizeFqdn) {
"m%y*host",
Option6ClientFqdn::PARTIAL, Option6ClientFqdn::FLAG_S,
"mxyxhost.example.com.", false);
// Verify that a full FQDN with nul chars is cleaned.
testFqdn(DHCPV6_SOLICIT, Option6ClientFqdn::FLAG_S,
std::string("m\000yhost.exa\000mple.com", 20),
Option6ClientFqdn::FULL, Option6ClientFqdn::FLAG_S,
"mxyhost.exaxmple.com.", false);
}
// Verifies that scoped ddns-parameter handling.
......
This diff is collapsed.
......@@ -110,7 +110,9 @@ const SimpleDefaults SimpleParser4::GLOBAL4_DEFAULTS = {
{ "ddns-override-client-update", Element::boolean, "false" },
{ "ddns-replace-client-name", Element::string, "never" },
{ "ddns-generated-prefix", Element::string, "myhost" },
{ "ddns-qualifying-suffix", Element::string, "" }
{ "ddns-qualifying-suffix", Element::string, "" },
{ "hostname-char-set", Element::string, "[^A-Za-z0-9.-]" },
{ "hostname-char-replacement", Element::string, "" }
};
/// @brief This table defines all option definition parameters.
......
......@@ -105,7 +105,9 @@ const SimpleDefaults SimpleParser6::GLOBAL6_DEFAULTS = {
{ "ddns-override-client-update", Element::boolean, "false" },
{ "ddns-replace-client-name", Element::string, "never" },
{ "ddns-generated-prefix", Element::string, "myhost" },
{ "ddns-qualifying-suffix", Element::string, "" }
{ "ddns-qualifying-suffix", Element::string, "" },
{ "hostname-char-set", Element::string, "[^A-Za-z0-9.-]" },
{ "hostname-char-replacement", Element::string, "" }
};
/// @brief This table defines all option definition parameters.
......
// Copyright (C) 2011-2019 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2011-2020 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
......@@ -351,38 +351,53 @@ public:
return (result.str());
#else
// Iterate over original string, match by match.
const char* origStr = original.c_str();
const char* startFrom = origStr;
const char* endAt = origStr + strlen(origStr);
regmatch_t matches[2]; // n matches + 1
// In order to handle embedded nuls, we have to process it nul-terminated
// chunks. We iterator over the original data, doing pattern replacement
// on each chunk.
const char* orig_data = original.data();
const char* dead_end = orig_data + original.size();
const char* start_from = orig_data;
stringstream result;
while (startFrom < endAt) {
// Look for the next match
if (regexec(&scrub_exp_, startFrom, 1, matches, 0) == REG_NOMATCH) {
// No matches, so add in the remainder
result << startFrom;
break;
}
while (start_from < dead_end) {
// Iterate over original string, match by match.
regmatch_t matches[2]; // n matches + 1
const char* end_at = start_from + strlen(start_from);
while (start_from < end_at) {
// Look for the next match
if (regexec(&scrub_exp_, start_from, 1, matches, 0) == REG_NOMATCH) {
// No matches, so add in the remainder
result << start_from;
start_from = end_at + 1;
break;
}
// Shouldn't happen, but one never knows eh?
if (matches[0].rm_so == -1) {
isc_throw(isc::Unexpected, "matched but so is -1?");
}
// Shouldn't happen, but one never knows eh?
if (matches[0].rm_so == -1) {
isc_throw(isc::Unexpected, "matched but so is -1?");
}
// Add everything from starting point up to the current match
const char* matchAt = startFrom + matches[0].rm_so;
while (startFrom < matchAt) {
result << *startFrom;
++startFrom;
}
// Add everything from starting point up to the current match
const char* match_at = start_from + matches[0].rm_so;
while (start_from < match_at) {
result << *start_from;
++start_from;
}
// Add in the replacement
result << char_replacement_;
// Add in the replacement
result << char_replacement_;
// Move past the match.
++start_from;
}
// Move past the match.
++startFrom;
// if we have an embedded nul, replace it and continue
if (start_from < dead_end) {
// Add in the replacement
result << char_replacement_;
start_from = end_at + 1;
}
}
return (result.str());
......
......@@ -509,7 +509,6 @@ void sanitizeStringTest(
// Verifies StringSantizer class
TEST(StringUtilTest, stringSanitizer) {
// Bad regular expression should throw.
StringSanitizerPtr ss;
ASSERT_THROW (ss.reset(new StringSanitizer("[bogus-regex","")), BadValue);
......@@ -517,7 +516,6 @@ TEST(StringUtilTest, stringSanitizer) {
// List of invalid chars should work: (b,c,2 are invalid)
sanitizeStringTest("abc.123", "[b-c2]", "*",
"a**.1*3");
// Inverted list of valid chars should work: (b,c,2 are valid)
sanitizeStringTest("abc.123", "[^b-c2]", "*",
"*bc**2*");
......@@ -549,6 +547,10 @@ TEST(StringUtilTest, stringSanitizer) {
// Dots as valid chars work.
sanitizeStringTest("abc.123", "[^A-Za-z0-9_.]", "*",
"abc.123");
std::string withNulls("\000ab\000c.12\0003",10);
sanitizeStringTest(withNulls, "[^A-Za-z0-9_.]", "*",
"*ab*c.12*3");
}
// Verifies templated buffer iterator seekTrimmed() function
......
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