mir/kea
2
0
Fork 0
mirror of https://gitlab.isc.org/isc-projects/kea synced 2025-08-31 14:05:33 +00:00
Code Issues Releases Wiki Activity

[#3848] Warn on invalid paths when security disabled

Browse Source 
Warn but still use invalid paths when security is
disabled.
This commit is contained in:
Thomas Markwalder
2025-06-09 13:08:08 -04:00
parent 048b1e9b1a
commit b5aeb99f98
38 changed files with 564 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc
View File

@@ -17,7 +17,9 @@
#include <hooks/hooks_parser.h>
#include <legal_log_log.h>
#include <rotating_file.h>
#include <util/filesystem.h>
#include <testutils/env_var_wrapper.h>
#include <testutils/log_utils.h>
#include <gtest/gtest.h>
@@ -30,6 +32,8 @@ using namespace isc::dhcp;
using namespace isc::hooks;
using namespace isc::legal_log;
using namespace isc::test;
using namespace isc::dhcp::test;
using namespace isc::util::file;
using namespace std;
namespace {
@@ -40,7 +44,8 @@ const DbLogger::MessageMap legal_log_db_message_map = {
DbLogger legal_log_db_logger(legal_log_logger, legal_log_db_message_map);
/// @brief Test fixture
struct LegalLogMgrTest : ::testing::Test {
class LegalLogMgrTest : public LogContentTest {
public:
/// @brief Constructor.
LegalLogMgrTest() : legal_log_dir_env_var_("KEA_LEGAL_LOG_DIR") {
HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR);
@@ -66,6 +71,7 @@ struct LegalLogMgrTest : ::testing::Test {
/// @brief Removes files that may be left over from previous tests
void reset() {
PathChecker::enableEnforcement(true);
std::ostringstream stream;
stream << "rm " << TEST_DATA_BUILDDIR << "/" << "*-legal"
<< ".*.txt 2>/dev/null";
@@ -184,7 +190,7 @@ TEST_F(LegalLogMgrTest, pathValidation) {
std::ostringstream os;
os << "invalid path specified: '/tmp', supported path is '"
<< LegalLogMgr::getLogPath() << "'";
ASSERT_THROW_MSG(LegalLogMgr::parseFile(params, map), BadValue, os.str());
ASSERT_THROW_MSG(LegalLogMgr::parseFile(params, map), SecurityError, os.str());
}
// Verify env variable path override.
@@ -212,7 +218,28 @@ TEST_F(LegalLogMgrTest, pathEnvVarOverride) {
std::ostringstream os;
os << "invalid path specified: '/bogus', supported path is '"
<< LegalLogMgr::getLogPath() << "'";
ASSERT_THROW_MSG(LegalLogMgr::parseFile(params, map), BadValue, os.str());
ASSERT_THROW_MSG(LegalLogMgr::parseFile(params, map), SecurityError, os.str());
}
// Verify path validation when security is disabled.
TEST_F(LegalLogMgrTest, pathValidationSecurityDisabled) {
PathChecker::enableEnforcement(false);
isc::db::DatabaseConnection::ParameterMap map;
EXPECT_NO_THROW(LegalLogMgr::parseConfig(ConstElementPtr(), map));
EXPECT_NO_THROW(LegalLogMgrFactory::addBackend(map));
EXPECT_TRUE(LegalLogMgrFactory::instance());
// Invalid path should be OK but we should get warning.
ElementPtr params = Element::createMap();
params->set("path", Element::create("/tmp"));
ASSERT_NO_THROW_LOG(LegalLogMgr::parseFile(params, map));
std::ostringstream os;
os << "LEGAL_LOG_PATH_SECURITY_WARNING Forensic log path specified is NOT SECURE:"
<< " invalid path specified: '/tmp', supported path is '"
<< LegalLogMgr::getLogPath() << "'";
EXPECT_EQ(1, countFile(os.str()));
}
// Verify that parsing extra parameters for rotate file works

6
src/hooks/dhcp/host_cache/host_cache.cc
View File

@@ -13,6 +13,7 @@
#include <database/db_exceptions.h>
#include <dhcpsrv/cfgmgr.h>
#include <util/encode/encode.h>
#include <util/filesystem.h>
#include <util/multi_threading_mgr.h>
#include <util/str.h>
#include <string>
@@ -26,6 +27,7 @@ using namespace isc::data;
using namespace isc::db;
using namespace isc::dhcp;
using namespace isc::util;
using namespace isc::util::file;
namespace isc {
namespace host_cache {
@@ -753,6 +755,10 @@ HostCache::cacheWriteHandler(hooks::CalloutHandle& handle) {
try {
filename = CfgMgr::instance().validatePath(cmd_args_->stringValue());
} catch (const SecurityWarn& ex) {
LOG_WARN(host_cache_logger, HOST_CACHE_PATH_SECURITY_WARNING)
.arg(ex.what());
filename = cmd_args_->stringValue();
} catch (const std::exception& ex) {
isc_throw(BadValue, "parameter is invalid: " << ex.what());
}

2
src/hooks/dhcp/host_cache/host_cache_messages.cc
View File

@@ -42,6 +42,7 @@ extern const isc::log::MessageID HOST_CACHE_GET_ONE_SUBNET_ID_ADDRESS6_HOST = "H
extern const isc::log::MessageID HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER = "HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER";
extern const isc::log::MessageID HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER_HOST = "HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER_HOST";
extern const isc::log::MessageID HOST_CACHE_INIT_OK = "HOST_CACHE_INIT_OK";
extern const isc::log::MessageID HOST_CACHE_PATH_SECURITY_WARNING = "HOST_CACHE_PATH_SECURITY_WARNING";
} // namespace host_cache
} // namespace isc
@@ -84,6 +85,7 @@ const char* values[] = {
"HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER", "get one host with %1 reservation for subnet id %2, identified by %3",
"HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER_HOST", "using subnet id %1 and identifier %2, found host: %3",
"HOST_CACHE_INIT_OK", "loading Host Cache hooks library successful",
"HOST_CACHE_PATH_SECURITY_WARNING", "Cache file path specified is NOT SECURE: %1",
NULL
};

1
src/hooks/dhcp/host_cache/host_cache_messages.h
View File

@@ -43,6 +43,7 @@ extern const isc::log::MessageID HOST_CACHE_GET_ONE_SUBNET_ID_ADDRESS6_HOST;
extern const isc::log::MessageID HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER;
extern const isc::log::MessageID HOST_CACHE_GET_ONE_SUBNET_ID_IDENTIFIER_HOST;
extern const isc::log::MessageID HOST_CACHE_INIT_OK;
extern const isc::log::MessageID HOST_CACHE_PATH_SECURITY_WARNING;
} // namespace host_cache
} // namespace isc

6
src/hooks/dhcp/host_cache/host_cache_messages.mes
View File

@@ -158,3 +158,9 @@ subnet id and specific host identifier.
% HOST_CACHE_INIT_OK loading Host Cache hooks library successful
This info message indicates that the Host Cache hooks library has been
loaded successfully. Enjoy!
% HOST_CACHE_PATH_SECURITY_WARNING Cache file path specified is NOT SECURE: %1
This warning message is issued when security enforcement is
disabled and the host cache file path specified does not comply
with the supported path. The server will still use the specified
path but is warning that doing so may pose a security risk.

39
src/hooks/dhcp/host_cache/tests/command_unittests.cc
View File

@@ -14,6 +14,7 @@
#include <dhcpsrv/cfgmgr.h>
#include <testutils/env_var_wrapper.h>
#include <testutils/multi_threading_utils.h>
#include <testutils/log_utils.h>
#include <gtest/gtest.h>
#include <fstream>
#include <functional>
@@ -28,16 +29,19 @@ using namespace isc::hooks;
using namespace isc::host_cache;
using namespace isc::test;
using namespace isc::util;
using namespace isc::dhcp::test;
using namespace std;
namespace ph = std::placeholders;
namespace {
/// @brief Test fixture for testing commands for the host-cache library
class CommandTest : public ::testing::Test {
//class CommandTest : public ::testing::Test {
class CommandTest : public LogContentTest {
public:
/// @brief Constructor
CommandTest() {
file::PathChecker::enableEnforcement(true);
// Save the pre-test data dir and set it to the test directory.
CfgMgr::instance().clear();
original_datadir_ = CfgMgr::instance().getDataDir();
@@ -54,6 +58,7 @@ public:
// Revert to original data directory.
CfgMgr::instance().getDataDir(true, original_datadir_);
file::PathChecker::enableEnforcement(true);
}
/// @brief Creates a sample IPv4 host
@@ -283,6 +288,9 @@ public:
/// @brief Test cache-write command.
void testWriteCommand();
/// @brief Test cache-write command with security disabled.
void testWriteCommandSecurityWarning();
/// @brief Test cache-load command.
void testLoadCommand();
@@ -758,6 +766,31 @@ CommandTest::testWriteCommand() {
checkCommand(write_handler, badpath_cmd, 1, 1, exp_error);
}
// Verifies that cache-write really writes the expected file.
void
CommandTest::testWriteCommandSecurityWarning() {
// Prepare
handlerType write_handler = std::bind(&writeHandler, hcptr_, ph::_1);
file::PathChecker::enableEnforcement(false);
string badpath = "/tmp/kea-host-cache-test.txt";
string write_cmd =
"{ \"command\": \"cache-write\", \"arguments\": \"" + badpath + "\" }";
// Insert an entry.
EXPECT_EQ(0, hcptr_->insert(createHost4(), false));
EXPECT_EQ(1, hcptr_->size());
// Write file
checkCommand(write_handler, write_cmd, 0, 0,
"1 entries dumped to '" + badpath + "'.");
std::ostringstream oss;
oss << "HOST_CACHE_PATH_SECURITY_WARNING Cache file path specified"
<< " is NOT SECURE: invalid path specified: '/tmp', supported path is '"
<< CfgMgr::instance().getDataDir() << "'";
EXPECT_EQ(1, countFile(oss.str()));
}
// Verifies that cache-load can load a dump file.
void
CommandTest::testLoadCommand() {
@@ -1142,6 +1175,10 @@ TEST_F(CommandTest, write) {
testWriteCommand();
}
TEST_F(CommandTest, writeSecurityWarning) {
testWriteCommandSecurityWarning();
}
TEST_F(CommandTest, writeMultiThreading) {
MultiThreadingTest mt(true);
testWriteCommand();

6
src/hooks/dhcp/lease_cmds/lease_cmds.cc
View File

@@ -28,6 +28,7 @@
#include <lease_cmds_log.h>
#include <stats/stats_mgr.h>
#include <util/encode/encode.h>
#include <util/filesystem.h>
#include <util/multi_threading_mgr.h>
#include <boost/scoped_ptr.hpp>
@@ -43,6 +44,7 @@ using namespace isc::asiolink;
using namespace isc::hooks;
using namespace isc::stats;
using namespace isc::util;
using namespace isc::util::file;
using namespace isc::log;
using namespace std;
@@ -2755,6 +2757,10 @@ LeaseCmdsImpl::leaseWriteHandler(CalloutHandle& handle) {
std::string filename;
try {
filename = CfgMgr::instance().validatePath(file->stringValue());
} catch (const SecurityWarn& ex) {
LOG_WARN(lease_cmds_logger, LEASE_CMDS_PATH_SECURITY_WARNING)
.arg(ex.what());
filename = file->stringValue();
} catch (const std::exception& ex) {
isc_throw(BadValue, "'filename' parameter is invalid: " << ex.what());
}

2
src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc
View File

@@ -26,6 +26,7 @@ extern const isc::log::MessageID LEASE_CMDS_LEASES6_COMMITTED_CONFLICT = "LEASE_
extern const isc::log::MessageID LEASE_CMDS_LEASES6_COMMITTED_FAILED = "LEASE_CMDS_LEASES6_COMMITTED_FAILED";
extern const isc::log::MessageID LEASE_CMDS_LEASES6_COMMITTED_LEASE_ERROR = "LEASE_CMDS_LEASES6_COMMITTED_LEASE_ERROR";
extern const isc::log::MessageID LEASE_CMDS_LOAD_ERROR = "LEASE_CMDS_LOAD_ERROR";
extern const isc::log::MessageID LEASE_CMDS_PATH_SECURITY_WARNING = "LEASE_CMDS_PATH_SECURITY_WARNING";
extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4 = "LEASE_CMDS_RESEND_DDNS4";
extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4_FAILED = "LEASE_CMDS_RESEND_DDNS4_FAILED";
extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS6 = "LEASE_CMDS_RESEND_DDNS6";
@@ -66,6 +67,7 @@ const char* values[] = {
"LEASE_CMDS_LEASES6_COMMITTED_FAILED", "reason: %1",
"LEASE_CMDS_LEASES6_COMMITTED_LEASE_ERROR", "evaluating binding-variables for lease: %1 for: %2, reason: %3",
"LEASE_CMDS_LOAD_ERROR", "loading Lease Commands hooks library failed: %1",
"LEASE_CMDS_PATH_SECURITY_WARNING", "lease file path specified is NOT SECURE: %1",
"LEASE_CMDS_RESEND_DDNS4", "lease4-resend-ddns command successful: %1",
"LEASE_CMDS_RESEND_DDNS4_FAILED", "lease4-resend-ddns command failed: %1",
"LEASE_CMDS_RESEND_DDNS6", "lease6-resend-ddns command successful: %1",

1
src/hooks/dhcp/lease_cmds/lease_cmds_messages.h
View File

@@ -27,6 +27,7 @@ extern const isc::log::MessageID LEASE_CMDS_LEASES6_COMMITTED_CONFLICT;
extern const isc::log::MessageID LEASE_CMDS_LEASES6_COMMITTED_FAILED;
extern const isc::log::MessageID LEASE_CMDS_LEASES6_COMMITTED_LEASE_ERROR;
extern const isc::log::MessageID LEASE_CMDS_LOAD_ERROR;
extern const isc::log::MessageID LEASE_CMDS_PATH_SECURITY_WARNING;
extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4;
extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS4_FAILED;
extern const isc::log::MessageID LEASE_CMDS_RESEND_DDNS6;

7
src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes
View File

@@ -166,3 +166,10 @@ are logged.
% LEASE_CMDS_WIPE6_FAILED lease6-wipe command failed (parameters: %1, reason: %2)
The lease6-wipe command has failed. Both the reason as well as the
parameters passed are logged.
% LEASE_CMDS_PATH_SECURITY_WARNING lease file path specified is NOT SECURE: %1
This warning message is issued when security enforcement is disabled
and the path portion of the `filename` parameter of the lease4-write
or lease6-write command does not comply with the supported path. The
server will still use the specified path but is warning that doing so
may pose a security risk.

38
src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc
View File

@@ -19,6 +19,7 @@
#include <lease_cmds.h>
#include <lease_cmds_unittest.h>
#include <stats/stats_mgr.h>
#include <util/filesystem.h>
#include <testutils/user_context_utils.h>
#include <testutils/multi_threading_utils.h>
#include <testutils/gtest_utils.h>
@@ -37,6 +38,7 @@ using namespace isc::dhcp;
using namespace isc::dhcp_ddns;
using namespace isc::asiolink;
using namespace isc::stats;
using namespace isc::util::file;
using namespace isc::test;
using namespace isc::lease_cmds;
@@ -53,6 +55,9 @@ public:
setFamily(AF_INET);
}
/// @brief Destructor.
virtual ~Lease4CmdsTest(){};
/// @brief Checks if specified response contains IPv4 lease
///
/// @param lease Element tree that represents a lease
@@ -447,6 +452,10 @@ public:
/// @brief Check that lease4-write works as expected.
void testLease4Write();
/// @brief Check that lease4-write works as expected when security
/// is disabled.
void testLease4WriteSecurityWarn();
};
void Lease4CmdsTest::testLease4AddMissingParams() {
@@ -3496,6 +3505,31 @@ void Lease4CmdsTest::testLease4Write() {
testCommand(txt, CONTROL_RESULT_ERROR, os.str());
}
void Lease4CmdsTest::testLease4WriteSecurityWarn() {
PathChecker::enableEnforcement(false);
// Initialize lease manager (false = v4, false = don't add leases)
initLeaseMgr(false, false);
// Filename must use supported path.
std::string cmd =
"{\n"
" \"command\": \"lease4-write\",\n"
" \"arguments\": {"
" \"filename\": \"/tmp/kea-lease-write-test.txt\"\n"
" }\n"
"}";
std::ostringstream os;
os << "LEASE_CMDS_PATH_SECURITY_WARNING lease file path specified is NOT SECURE:"
<< " invalid path specified: '/tmp', supported path is '"
<< CfgMgr::instance().getDataDir() << "'";
testCommand(cmd, CONTROL_RESULT_SUCCESS,
"IPv4 lease database into '/tmp/kea-lease-write-test.txt'.");
EXPECT_EQ(1, countFile(os.str()));
}
TEST_F(Lease4CmdsTest, lease4AddMissingParams) {
testLease4AddMissingParams();
}
@@ -4224,4 +4258,8 @@ TEST_F(Lease4CmdsTest, lease4WriteMultiThreading) {
testLease4Write();
}
TEST_F(Lease4CmdsTest, lease4WriteSecurityWarn) {
testLease4WriteSecurityWarn();
}
} // end of anonymous namespace

6
src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds_unittest.h
View File

@@ -18,8 +18,10 @@
#include <cc/data.h>
#include <process/daemon.h>
#include <stats/stats_mgr.h>
#include <util/filesystem.h>
#include <testutils/user_context_utils.h>
#include <testutils/multi_threading_utils.h>
#include <testutils/log_utils.h>
#include <gtest/gtest.h>
@@ -40,7 +42,7 @@ constexpr uint32_t HIGH_VALID_LIFETIME = 0xFFFFFFFE;
constexpr time_t DEC_2030_TIME = 1923222072;
/// @brief Test fixture for testing loading and unloading the flex-id library
class LibLoadTest : public ::testing::Test {
class LibLoadTest : public isc::dhcp::test::LogContentTest {
public:
/// @brief Constructor
LibLoadTest(std::string lib_filename)
@@ -282,6 +284,7 @@ public:
enableD2();
lmptr_ = 0;
isc::stats::StatsMgr::instance().removeAll();
isc::util::file::PathChecker::enableEnforcement(true);
}
/// @brief Destructor
@@ -295,6 +298,7 @@ public:
unloadLibs();
lmptr_ = 0;
isc::stats::StatsMgr::instance().removeAll();
isc::util::file::PathChecker::enableEnforcement(true);
}
/// @brief Creates an IPv4 lease

5
src/hooks/dhcp/lease_cmds/libloadtests/meson.build
View File

@@ -2,6 +2,10 @@ if not TESTS_OPT.enabled()
subdir_done()
endif
libs_testutils = [
kea_testutils_lib
]
dhcp_lease_cmds_libload_tests = executable(
'dhcp-lease-cmds-libload-tests',
'lease_cmds4_unittest.cc',
@@ -15,6 +19,7 @@ dhcp_lease_cmds_libload_tests = executable(
dependencies: [GTEST_DEP, CRYPTO_DEP],
include_directories: [include_directories('.'), include_directories('..')] + INCLUDES,
link_with: LIBS_BUILT_SO_FAR,
link_with: [libs_testutils] + LIBS_BUILT_SO_FAR,
)
test(
'dhcp-lease-cmds-libload-tests',

2
src/lib/config/config_messages.cc
View File

@@ -31,6 +31,7 @@ extern const isc::log::MessageID COMMAND_SOCKET_READ = "COMMAND_SOCKET_READ";
extern const isc::log::MessageID COMMAND_SOCKET_READ_FAIL = "COMMAND_SOCKET_READ_FAIL";
extern const isc::log::MessageID COMMAND_SOCKET_WRITE = "COMMAND_SOCKET_WRITE";
extern const isc::log::MessageID COMMAND_SOCKET_WRITE_FAIL = "COMMAND_SOCKET_WRITE_FAIL";
extern const isc::log::MessageID COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING = "COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING";
extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLEAR_ERROR = "COMMAND_WATCH_SOCKET_CLEAR_ERROR";
extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLOSE_ERROR = "COMMAND_WATCH_SOCKET_CLOSE_ERROR";
extern const isc::log::MessageID COMMAND_WATCH_SOCKET_MARK_READY_ERROR = "COMMAND_WATCH_SOCKET_MARK_READY_ERROR";
@@ -71,6 +72,7 @@ const char* values[] = {
"COMMAND_SOCKET_READ_FAIL", "Encountered error %1 while reading from command socket %2",
"COMMAND_SOCKET_WRITE", "Sent response of %1 bytes (%2 bytes left to send) over command socket %3",
"COMMAND_SOCKET_WRITE_FAIL", "Error while writing to command socket %1 : %2",
"COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING", "unix socket path is NOT SECURE: %1",
"COMMAND_WATCH_SOCKET_CLEAR_ERROR", "watch socket failed to clear: %1",
"COMMAND_WATCH_SOCKET_CLOSE_ERROR", "watch socket failed to close: %1",
"COMMAND_WATCH_SOCKET_MARK_READY_ERROR", "watch socket failed to mark ready: %1",

1
src/lib/config/config_messages.h
View File

@@ -32,6 +32,7 @@ extern const isc::log::MessageID COMMAND_SOCKET_READ;
extern const isc::log::MessageID COMMAND_SOCKET_READ_FAIL;
extern const isc::log::MessageID COMMAND_SOCKET_WRITE;
extern const isc::log::MessageID COMMAND_SOCKET_WRITE_FAIL;
extern const isc::log::MessageID COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING;
extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLEAR_ERROR;
extern const isc::log::MessageID COMMAND_WATCH_SOCKET_CLOSE_ERROR;
extern const isc::log::MessageID COMMAND_WATCH_SOCKET_MARK_READY_ERROR;

6
src/lib/config/config_messages.mes
View File

@@ -180,3 +180,9 @@ control commands.
% HTTP_COMMAND_MGR_SERVICE_STOPPING Server is stopping %1 service %2
This informational message indicates that the server has stopped
HTTP/HTTPS service. When known the address and port are displayed.
% COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING unix socket path is NOT SECURE: %1
This warning message is issued when security enforcement is disabled
and the path specified for a control channel unix socket-name does
not comply with the supported path. The server will still use the
specified path but is warning that doing so may pose a security risk.

23
src/lib/config/tests/unix_command_config_unittests.cc
View File

@@ -13,6 +13,7 @@
#include <testutils/gtest_utils.h>
#include <testutils/test_to_element.h>
#include <testutils/env_var_wrapper.h>
#include <testutils/log_utils.h>
using namespace isc;
using namespace isc::asiolink;
@@ -22,12 +23,14 @@ using namespace isc::dhcp;
using namespace isc::http;
using namespace isc::test;
using namespace isc::util;
using namespace isc::dhcp::test;
using namespace std;
namespace {
/// @brief Test fixture for UNIX control socket configuration.
class UnixCommandConfigTest : public ::testing::Test {
//class UnixCommandConfigTest : public ::testing::Test {
class UnixCommandConfigTest : public LogContentTest {
public:
/// @brief Constructor.
UnixCommandConfigTest() : unix_config_() {
@@ -142,4 +145,22 @@ TEST_F(UnixCommandConfigTest, errors) {
}
}
// This test verifies security warning of invalid
// socket paths.
TEST_F(UnixCommandConfigTest, securityEnforcmentFalse) {
file::PathChecker::enableEnforcement(false);
std::string config = R"( { "socket-name": "/tmp/mysocket" } )";
ElementPtr json;
ASSERT_NO_THROW(json = Element::fromJSON(config));
ASSERT_NO_THROW_LOG(unix_config_.reset(new UnixCommandConfig(json)));
std::ostringstream oss;
oss << "COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING unix socket path is NOT SECURE:"
<< " invalid path specified: '/tmp', supported path is '"
<< UnixCommandConfig::getSocketPath() << "'";
EXPECT_EQ(1, countFile(oss.str()));
}
} // end of anonymous namespace

12
src/lib/config/unix_command_config.cc
View File

@@ -9,6 +9,7 @@
#include <asiolink/io_asio_socket.h>
#include <cc/dhcp_config_error.h>
#include <config/command_mgr.h>
#include <config/config_log.h>
#include <config/unix_command_config.h>
#include <util/filesystem.h>
#include <limits>
@@ -116,7 +117,16 @@ UnixCommandConfig::validatePath(const std::string socket_path) {
getSocketPath();
}
auto valid_path = socket_path_checker_->validatePath(socket_path);
std::string valid_path;
try {
valid_path = socket_path_checker_->validatePath(socket_path);
} catch (const SecurityWarn& ex) {
LOG_WARN(command_logger, COMMAND_UNIX_SOCKET_PATH_SECURITY_WARNING)
.arg(ex.what());
// Skip checking permissions.
return(socket_path);
}
if (!socket_path_checker_->pathHasPermissions(socket_path_perms_)) {
isc_throw (DhcpConfigError,
"socket path:" << socket_path_checker_->getPath()

4
src/lib/dhcpsrv/dhcpsrv_messages.cc
View File

@@ -138,6 +138,7 @@ extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_UNREGISTER_TIMER_FAILED = "
extern const isc::log::MessageID DHCPSRV_MEMFILE_NEEDS_DOWNGRADING = "DHCPSRV_MEMFILE_NEEDS_DOWNGRADING";
extern const isc::log::MessageID DHCPSRV_MEMFILE_NEEDS_UPGRADING = "DHCPSRV_MEMFILE_NEEDS_UPGRADING";
extern const isc::log::MessageID DHCPSRV_MEMFILE_NO_STORAGE = "DHCPSRV_MEMFILE_NO_STORAGE";
extern const isc::log::MessageID DHCPSRV_MEMFILE_PATH_SECURITY_WARNING = "DHCPSRV_MEMFILE_PATH_SECURITY_WARNING";
extern const isc::log::MessageID DHCPSRV_MEMFILE_READ_HWADDR_FAIL = "DHCPSRV_MEMFILE_READ_HWADDR_FAIL";
extern const isc::log::MessageID DHCPSRV_MEMFILE_ROLLBACK = "DHCPSRV_MEMFILE_ROLLBACK";
extern const isc::log::MessageID DHCPSRV_MEMFILE_UPDATE_ADDR4 = "DHCPSRV_MEMFILE_UPDATE_ADDR4";
@@ -176,6 +177,7 @@ extern const isc::log::MessageID DHCPSRV_TIMERMGR_STOP_TIMER = "DHCPSRV_TIMERMGR
extern const isc::log::MessageID DHCPSRV_TIMERMGR_UNREGISTER_ALL_TIMERS = "DHCPSRV_TIMERMGR_UNREGISTER_ALL_TIMERS";
extern const isc::log::MessageID DHCPSRV_TIMERMGR_UNREGISTER_TIMER = "DHCPSRV_TIMERMGR_UNREGISTER_TIMER";
extern const isc::log::MessageID DHCPSRV_UNKNOWN_DB = "DHCPSRV_UNKNOWN_DB";
extern const isc::log::MessageID LEGAL_LOG_PATH_SECURITY_WARNING = "LEGAL_LOG_PATH_SECURITY_WARNING";
} // namespace dhcp
} // namespace isc
@@ -314,6 +316,7 @@ const char* values[] = {
"DHCPSRV_MEMFILE_NEEDS_DOWNGRADING", "version of lease file: %1 schema is later than version %2",
"DHCPSRV_MEMFILE_NEEDS_UPGRADING", "version of lease file: %1 schema is earlier than version %2",
"DHCPSRV_MEMFILE_NO_STORAGE", "running in non-persistent mode, leases will be lost after restart",
"DHCPSRV_MEMFILE_PATH_SECURITY_WARNING", "Lease file path specified is NOT SECURE: %1",
"DHCPSRV_MEMFILE_READ_HWADDR_FAIL", "failed to read hardware address from lease file: %1",
"DHCPSRV_MEMFILE_ROLLBACK", "rolling back memory file database",
"DHCPSRV_MEMFILE_UPDATE_ADDR4", "updating IPv4 lease for address %1",
@@ -352,6 +355,7 @@ const char* values[] = {
"DHCPSRV_TIMERMGR_UNREGISTER_ALL_TIMERS", "unregistering all timers",
"DHCPSRV_TIMERMGR_UNREGISTER_TIMER", "unregistering timer: %1",
"DHCPSRV_UNKNOWN_DB", "unknown database type: %1",
"LEGAL_LOG_PATH_SECURITY_WARNING", "Forensic log path specified is NOT SECURE: %1",
NULL
};

2
src/lib/dhcpsrv/dhcpsrv_messages.h
View File

@@ -139,6 +139,7 @@ extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_UNREGISTER_TIMER_FAILED;
extern const isc::log::MessageID DHCPSRV_MEMFILE_NEEDS_DOWNGRADING;
extern const isc::log::MessageID DHCPSRV_MEMFILE_NEEDS_UPGRADING;
extern const isc::log::MessageID DHCPSRV_MEMFILE_NO_STORAGE;
extern const isc::log::MessageID DHCPSRV_MEMFILE_PATH_SECURITY_WARNING;
extern const isc::log::MessageID DHCPSRV_MEMFILE_READ_HWADDR_FAIL;
extern const isc::log::MessageID DHCPSRV_MEMFILE_ROLLBACK;
extern const isc::log::MessageID DHCPSRV_MEMFILE_UPDATE_ADDR4;
@@ -177,6 +178,7 @@ extern const isc::log::MessageID DHCPSRV_TIMERMGR_STOP_TIMER;
extern const isc::log::MessageID DHCPSRV_TIMERMGR_UNREGISTER_ALL_TIMERS;
extern const isc::log::MessageID DHCPSRV_TIMERMGR_UNREGISTER_TIMER;
extern const isc::log::MessageID DHCPSRV_UNKNOWN_DB;
extern const isc::log::MessageID LEGAL_LOG_PATH_SECURITY_WARNING;
} // namespace dhcp
} // namespace isc

13
src/lib/dhcpsrv/dhcpsrv_messages.mes
View File

@@ -1002,3 +1002,16 @@ included in the message.
% DHCPSRV_UNKNOWN_DB unknown database type: %1
The database access string specified a database type (given in the
message) that is unknown to the software. This is a configuration error.
% DHCPSRV_MEMFILE_PATH_SECURITY_WARNING Lease file path specified is NOT SECURE: %1
This warning message is issued when security enforcement is
disabled and the lease file path specified for does not comply
with the supported path. The server will still use the specified
path but is warning that doing so may pose a security risk.
% LEGAL_LOG_PATH_SECURITY_WARNING Forensic log path specified is NOT SECURE: %1
This warning message is issued when security enforcement is
disabled and the path specified for forensic logging output
does not comply with the supported path. The server will
still use the specified path but is warning that doing so may
pose a security risk.

12
src/lib/dhcpsrv/legal_log_mgr.cc
View File

@@ -11,6 +11,7 @@
#include <database/database_connection.h>
#include <dhcpsrv/cfgmgr.h>
#include <dhcpsrv/dhcpsrv_log.h>
#include <eval/eval_context.h>
#include <util/reconnect_ctl.h>
#include <util/filesystem.h>
@@ -32,6 +33,7 @@ using namespace isc::dhcp;
using namespace isc::eval;
using namespace isc::hooks;
using namespace isc::util;
using namespace isc::util::file;
using namespace std;
namespace {
@@ -173,8 +175,14 @@ LegalLogMgr::parseFile(const ConstElementPtr& parameters, DatabaseConnection::Pa
ConstElementPtr const value(parameters->get(key));
if (value) {
if (key == std::string("path")) {
auto valid_path = validatePath(value->stringValue());
file_parameters.emplace(key, valid_path);
try {
auto valid_path = validatePath(value->stringValue());
file_parameters.emplace(key, valid_path);
} catch (const SecurityWarn& ex) {
LOG_WARN(dhcpsrv_logger, LEGAL_LOG_PATH_SECURITY_WARNING)
.arg(ex.what());
file_parameters.emplace(key, value->stringValue());
}
}
file_parameters.emplace(key, value->stringValue());

11
src/lib/dhcpsrv/memfile_lease_mgr.cc
View File

@@ -18,6 +18,7 @@
#include <stats/stats_mgr.h>
#include <util/multi_threading_mgr.h>
#include <util/pid_file.h>
#include <util/filesystem.h>
#include <boost/foreach.hpp>
#include <cstdio>
@@ -45,6 +46,7 @@ using namespace isc::data;
using namespace isc::db;
using namespace isc::util;
using namespace isc::stats;
using namespace isc::util::file;
namespace isc {
namespace dhcp {
@@ -2321,8 +2323,13 @@ Memfile_LeaseMgr::initLeaseFilePath(Universe u) {
return (getDefaultLeaseFilePath(u));
}
// If path is invalid this will throw.
lease_file = CfgMgr::instance().validatePath(lease_file);
try {
lease_file = CfgMgr::instance().validatePath(lease_file);
} catch (const SecurityWarn& ex) {
LOG_WARN(dhcpsrv_logger, DHCPSRV_MEMFILE_PATH_SECURITY_WARNING)
.arg(ex.what());
}
return (lease_file);
}

73
src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
View File

@@ -27,6 +27,8 @@
#include <util/range_utilities.h>
#include <util/stopwatch.h>
#include <testutils/env_var_wrapper.h>
#include <util/filesystem.h>
#include <testutils/log_utils.h>
#include <gtest/gtest.h>
@@ -139,6 +141,7 @@ public:
setExtendedInfoSanityCheck(CfgConsistency::EXTENDED_INFO_CHECK_FIX);
MultiThreadingMgr::instance().setMode(false);
file::PathChecker::enableEnforcement(true);
}
/// @brief Reopens the connection to the backend.
@@ -176,6 +179,7 @@ public:
// Revert to original data directory.
CfgMgr::instance().getDataDir(true, original_datadir_);
file::PathChecker::enableEnforcement(true);
}
/// @brief Remove files being products of Lease File Cleanup.
@@ -477,7 +481,7 @@ TEST_F(MemfileLeaseMgrTest, defaultDataDir) {
<< CfgMgr::instance().getDataDir() << "'";
ASSERT_THROW_MSG(lease_mgr.reset(new Memfile_LeaseMgr(pmap)),
BadValue, os.str());
file::SecurityError, os.str());
}
/// @brief Verifies that the supported path may be overridden with
@@ -4509,4 +4513,71 @@ TEST_F(MemfileLeaseMgrTest, bigStats) {
testBigStats();
}
/// @brief Test fixture which allows log content to be tested.
class MemfileLeaseMgrLogTest : public LogContentTest {
public:
/// @brief memfile lease mgr test constructor
///
/// Creates memfile and stores it in lmptr_ pointer
MemfileLeaseMgrLogTest() :
data_dir_env_var_("KEA_DHCP_DATA_DIR") {
// Reset the env variable.
data_dir_env_var_.setValue();
// Save the pre-test data dir and set it to the test directory.
CfgMgr::instance().clear();
original_datadir_ = CfgMgr::instance().getDataDir();
CfgMgr::instance().getDataDir(true, TEST_DATA_BUILDDIR);
MultiThreadingMgr::instance().setMode(false);
file::PathChecker::enableEnforcement(true);
}
/// @brief destructor
///
/// destroys lease manager backend.
virtual ~MemfileLeaseMgrLogTest() {
LeaseMgrFactory::destroy();
// Disable multi-threading.
MultiThreadingMgr::instance().setMode(false);
// Revert to original data directory.
CfgMgr::instance().getDataDir(true, original_datadir_);
file::PathChecker::enableEnforcement(true);
}
/// @brief RAII wrapper for KEA_DHCP_DATA_DIR env variable.
EnvVarWrapper data_dir_env_var_;
/// @brief Stores the pre-test DHCP data directory.
std::string original_datadir_;
};
/// @brief Verifies that a warning is logged when given
/// an invalid path and enforcement is disabled.
TEST_F(MemfileLeaseMgrLogTest, warnWhenSecurityDisabled) {
ASSERT_TRUE(data_dir_env_var_.getValue().empty());
file::PathChecker::enableEnforcement(false);
DatabaseConnection::ParameterMap pmap;
pmap["universe"] = "6";
pmap["persist"] = "true";
pmap["lfc-interval"] = "0";
pmap["name"] = "/tmp/leasefile6_1.csv";
boost::scoped_ptr<Memfile_LeaseMgr> lease_mgr;
ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap)));
EXPECT_EQ(lease_mgr->getLeaseFilePath(Memfile_LeaseMgr::V6),
"/tmp/leasefile6_1.csv");
std::ostringstream oss;
oss << "DHCPSRV_MEMFILE_PATH_SECURITY_WARNING Lease file path specified is NOT SECURE:"
<< " invalid path specified: '/tmp', supported path is '"
<< CfgMgr::instance().getDataDir() << "'";
EXPECT_EQ(1, countFile(oss.str()));
}
} // namespace

2
src/lib/hooks/hooks_messages.cc
View File

@@ -19,6 +19,7 @@ extern const isc::log::MessageID HOOKS_CALLOUT_REGISTRATION = "HOOKS_CALLOUT_REG
extern const isc::log::MessageID HOOKS_CLOSE_ERROR = "HOOKS_CLOSE_ERROR";
extern const isc::log::MessageID HOOKS_HOOK_LIST_RESET = "HOOKS_HOOK_LIST_RESET";
extern const isc::log::MessageID HOOKS_INCORRECT_VERSION = "HOOKS_INCORRECT_VERSION";
extern const isc::log::MessageID HOOKS_LIBPATH_SECURITY_WARNING = "HOOKS_LIBPATH_SECURITY_WARNING";
extern const isc::log::MessageID HOOKS_LIBRARY_CLOSED = "HOOKS_LIBRARY_CLOSED";
extern const isc::log::MessageID HOOKS_LIBRARY_LOADED = "HOOKS_LIBRARY_LOADED";
extern const isc::log::MessageID HOOKS_LIBRARY_LOADING = "HOOKS_LIBRARY_LOADING";
@@ -61,6 +62,7 @@ const char* values[] = {
"HOOKS_CLOSE_ERROR", "failed to close hook library %1: %2",
"HOOKS_HOOK_LIST_RESET", "the list of hooks has been reset",
"HOOKS_INCORRECT_VERSION", "hook library %1 is at version %2, require version %3",
"HOOKS_LIBPATH_SECURITY_WARNING", "Library path specified is NOT SECURE: %1",
"HOOKS_LIBRARY_CLOSED", "hooks library %1 successfully closed",
"HOOKS_LIBRARY_LOADED", "hooks library %1 successfully loaded",
"HOOKS_LIBRARY_LOADING", "loading hooks library %1",

1
src/lib/hooks/hooks_messages.h
View File

@@ -20,6 +20,7 @@ extern const isc::log::MessageID HOOKS_CALLOUT_REGISTRATION;
extern const isc::log::MessageID HOOKS_CLOSE_ERROR;
extern const isc::log::MessageID HOOKS_HOOK_LIST_RESET;
extern const isc::log::MessageID HOOKS_INCORRECT_VERSION;
extern const isc::log::MessageID HOOKS_LIBPATH_SECURITY_WARNING;
extern const isc::log::MessageID HOOKS_LIBRARY_CLOSED;
extern const isc::log::MessageID HOOKS_LIBRARY_LOADED;
extern const isc::log::MessageID HOOKS_LIBRARY_LOADING;

7
src/lib/hooks/hooks_messages.mes
View File

@@ -215,3 +215,10 @@ in a hook library during the unload process, called, and returned success.
This error message is issued if the version() function in the specified
hooks library was called and generated an exception. The library is
considered unusable and will not be loaded.
% HOOKS_LIBPATH_SECURITY_WARNING Library path specified is NOT SECURE: %1
This warning message is issued when security enforcement is
disabled and the library path specified for a given hook library
does not comply with the supported path. The server will still load
the hook library but is warning that doing so may pose a security
risk.

9
src/lib/hooks/hooks_parser.cc
View File

@@ -9,6 +9,7 @@
#include <cc/data.h>
#include <cc/dhcp_config_error.h>
#include <hooks/hooks_parser.h>
#include <hooks/hooks_log.h>
#include <boost/algorithm/string.hpp>
#include <util/filesystem.h>
#include <util/str.h>
@@ -71,7 +72,13 @@ HooksLibrariesParser::validatePath(const std::string libpath) {
getHooksPath();
}
return (hooks_path_checker_->validatePath(libpath));
try {
return (hooks_path_checker_->validatePath(libpath));
} catch (const SecurityWarn& ex) {
LOG_WARN(hooks_logger, HOOKS_LIBPATH_SECURITY_WARNING)
.arg(ex.what());
return (libpath);
}
}
// @todo use the flat style, split into list and item

42
src/lib/hooks/tests/hooks_manager_unittest.cc
View File

@@ -12,6 +12,7 @@
#include <hooks/server_hooks.h>
#include <util/filesystem.h>
#include <testutils/gtest_utils.h>
#include <testutils/log_utils.h>
#include <hooks/tests/common_test_class.h>
#define TEST_ASYNC_CALLOUT
@@ -32,6 +33,7 @@ using namespace isc;
using namespace isc::hooks;
using namespace isc::data;
using namespace isc::util::file;
using namespace isc::dhcp::test;
using namespace std;
namespace {
@@ -1083,7 +1085,7 @@ TEST_F(HooksManagerTest, UnloadBeforeUnpark) {
}
/// @brief Test fixture for hooks parsing.
class HooksParserTest : public ::testing::Test {
class HooksParserTest : public LogContentTest {
public:
/// @brief Constructor
HooksParserTest() {
@@ -1098,7 +1100,7 @@ public:
}
/// @brief Destructor
~HooksParserTest() {
virtual ~HooksParserTest() {
// Restore the original environment path.
if (!original_path_.empty()) {
setenv("KEA_HOOKS_PATH", original_path_.c_str(), 1);
@@ -1139,6 +1141,7 @@ TEST_F(HooksParserTest, validatePathEnforcePath) {
std::string lib_path_;
std::string exp_path_;
std::string exp_error_;
bool exp_security_err_;
};
std::list<Scenario> scenarios = {
@@ -1147,28 +1150,32 @@ TEST_F(HooksParserTest, validatePathEnforcePath) {
__LINE__,
"/var/lib/bs/mylib.so",
"",
string("invalid path specified: '/var/lib/bs', supported path is '" + def_path + "'")
string("invalid path specified: '/var/lib/bs', supported path is '" + def_path + "'"),
true
},
{
// No file name.
__LINE__,
def_path + "/",
"",
string ("path: '" + def_path + "/' has no filename")
string ("path: '" + def_path + "/' has no filename"),
false
},
{
// File name only is valid.
__LINE__,
"mylib.so",
def_path + "/mylib.so",
""
"",
false
},
{
// Valid full path.
__LINE__,
def_path + "/mylib.so",
def_path + "/mylib.so",
""
"",
false
},
{
// Invalid relative path.
@@ -1176,7 +1183,8 @@ TEST_F(HooksParserTest, validatePathEnforcePath) {
"../kea/mylib.so",
"",
string("invalid path specified: '../kea', supported path is '" +
def_path + "'")
def_path + "'"),
true
}
};
@@ -1190,9 +1198,15 @@ TEST_F(HooksParserTest, validatePathEnforcePath) {
HooksLibrariesParser::validatePath(scenario.lib_path_));
EXPECT_EQ(validated_path, scenario.exp_path_);
} else {
ASSERT_THROW_MSG(validated_path =
HooksLibrariesParser::validatePath(scenario.lib_path_),
BadValue, scenario.exp_error_);
if (scenario.exp_security_err_) {
ASSERT_THROW_MSG(validated_path =
HooksLibrariesParser::validatePath(scenario.lib_path_),
SecurityError, scenario.exp_error_);
} else {
ASSERT_THROW_MSG(validated_path =
HooksLibrariesParser::validatePath(scenario.lib_path_),
BadValue, scenario.exp_error_);
}
}
}
}
@@ -1255,6 +1269,12 @@ TEST_F(HooksParserTest, validatePathEnforcePathFalse) {
BadValue, scenario.exp_error_);
}
}
std::ostringstream oss;
oss << "HOOKS_LIBPATH_SECURITY_WARNING Library path specified is NOT SECURE:"
<< " invalid path specified: '/var/lib/bs', supported path is '"
<< def_path << "'";
EXPECT_EQ(1, countFile(oss.str()));
}
// Verifies output of HooksConfig::toElement().
@@ -1288,4 +1308,4 @@ TEST(HooksConfig, toElementTest) {
EXPECT_EQ(data::prettyPrint(cfg.toElement()), exp_cfg);
}
} // Anonymous namespace
} // Anonymous namespae

6
src/lib/hooks/tests/meson.build
View File

@@ -15,6 +15,10 @@ configure_file(
configuration: kea_hooks_conf_data,
)
libs_testutils = [
kea_testutils_lib
]
nvl = shared_library(
'nvl',
'no_version_library.cc',
@@ -112,7 +116,7 @@ kea_hooks_tests = executable(
dependencies: [GTEST_DEP],
cpp_args: [f'-DDEFAULT_HOOKS_PATH="@DEFAULT_HOOKS_PATH@"'],
include_directories: [include_directories('.')] + INCLUDES,
link_with: [kea_util_unittests_lib] + LIBS_BUILT_SO_FAR,
link_with: [kea_util_unittests_lib, libs_testutils] + LIBS_BUILT_SO_FAR,
)
test(
'kea-hooks-tests',

8
src/lib/process/log_parser.cc
View File

@@ -151,7 +151,13 @@ LogConfigParser::validatePath(const std::string logpath) {
getLogPath();
}
return (log_path_checker_->validatePath(logpath));
try {
return (log_path_checker_->validatePath(logpath));
} catch (const SecurityWarn& ex) {
LOG_WARN(dctl_logger, DCTL_LOG_PATH_SECURITY_WARNING)
.arg(ex.what());
return (logpath);
}
}
void LogConfigParser::parseOutputOptions(std::vector<LoggingDestination>& destination,

2
src/lib/process/process_messages.cc
View File

@@ -21,6 +21,7 @@ extern const isc::log::MessageID DCTL_DEPRECATED_OUTPUT_OPTIONS = "DCTL_DEPRECAT
extern const isc::log::MessageID DCTL_DEVELOPMENT_VERSION = "DCTL_DEVELOPMENT_VERSION";
extern const isc::log::MessageID DCTL_INIT_PROCESS = "DCTL_INIT_PROCESS";
extern const isc::log::MessageID DCTL_INIT_PROCESS_FAIL = "DCTL_INIT_PROCESS_FAIL";
extern const isc::log::MessageID DCTL_LOG_PATH_SECURITY_WARNING = "DCTL_LOG_PATH_SECURITY_WARNING";
extern const isc::log::MessageID DCTL_NOT_RUNNING = "DCTL_NOT_RUNNING";
extern const isc::log::MessageID DCTL_OPEN_CONFIG_DB = "DCTL_OPEN_CONFIG_DB";
extern const isc::log::MessageID DCTL_PARSER_FAIL = "DCTL_PARSER_FAIL";
@@ -54,6 +55,7 @@ const char* values[] = {
"DCTL_DEVELOPMENT_VERSION", "This software is a development branch of Kea. It is not recommended for production use.",
"DCTL_INIT_PROCESS", "%1 initializing the application",
"DCTL_INIT_PROCESS_FAIL", "%1 application initialization failed: %2",
"DCTL_LOG_PATH_SECURITY_WARNING", "Log output path specified is NOT SECURE: %1",
"DCTL_NOT_RUNNING", "%1 application instance is not running",
"DCTL_OPEN_CONFIG_DB", "Opening configuration database: %1",
"DCTL_PARSER_FAIL", "Parser error: %1",

1
src/lib/process/process_messages.h
View File

@@ -22,6 +22,7 @@ extern const isc::log::MessageID DCTL_DEPRECATED_OUTPUT_OPTIONS;
extern const isc::log::MessageID DCTL_DEVELOPMENT_VERSION;
extern const isc::log::MessageID DCTL_INIT_PROCESS;
extern const isc::log::MessageID DCTL_INIT_PROCESS_FAIL;
extern const isc::log::MessageID DCTL_LOG_PATH_SECURITY_WARNING;
extern const isc::log::MessageID DCTL_NOT_RUNNING;
extern const isc::log::MessageID DCTL_OPEN_CONFIG_DB;
extern const isc::log::MessageID DCTL_PARSER_FAIL;

7
src/lib/process/process_messages.mes
View File

@@ -145,3 +145,10 @@ This is a debug message indicating that the application received an
unsupported signal. This is a programming error indicating that the
application has registered to receive the signal but no associated
processing logic has been added.
% DCTL_LOG_PATH_SECURITY_WARNING Log output path specified is NOT SECURE: %1
This warning message is issued when security enforcement is
disabled and the output path specified for a given logger does
not comply with the supported path. The server will still
use the specified path but is warning that doing so may pose a
security risk.

89
src/lib/process/tests/log_parser_unittests.cc
View File

@@ -9,17 +9,21 @@
#include <cc/data.h>
#include <process/log_parser.h>
#include <process/process_messages.h>
#include <process/daemon.h>
#include <exceptions/exceptions.h>
#include <log/logger_support.h>
#include <process/d_log.h>
#include <util/filesystem.h>
#include <testutils/gtest_utils.h>
#include <testutils/io_utils.h>
#include <testutils/log_utils.h>
#include <gtest/gtest.h>
using namespace isc;
using namespace isc::process;
using namespace isc::data;
using namespace isc::util;
namespace {
@@ -29,8 +33,8 @@ namespace {
/// each test. Strictly speaking this only resets the testing root logger (which
/// has the name "kea") but as the only other logger mentioned here ("wombat")
/// is not used elsewhere, that is sufficient.
class LoggingTest : public ::testing::Test {
public:
class LoggingTest : public dhcp::test::LogContentTest {
public:
/// @brief Constructor
LoggingTest() {
resetLogPath();
@@ -77,6 +81,7 @@ class LoggingTest : public ::testing::Test {
/// @brief Resets the log path to default.
void resetLogPath() {
LogConfigParser::getLogPath(true);
file::PathChecker::enableEnforcement(true);
}
/// @brief Name of the log file
@@ -708,6 +713,86 @@ TEST_F(LoggingTest, syslogPlusFacility) {
EXPECT_EQ("syslog:daemon" , storage->getLoggingInfo()[0].destinations_[0].output_);
}
// Verifies that an invalid output path results in an
// error security enforcement is enabled.
TEST_F(LoggingTest, enforceSecurityTrue) {
const char* config_txt =
"{ \"loggers\": ["
" {"
" \"name\": \"kea\","
" \"output-options\": ["
" {"
" \"output\": \"/tmp/myfile.log\""
" }"
" ],"
" \"severity\": \"INFO\""
" }"
"]}";
ConfigPtr storage(new ConfigBase());
LogConfigParser parser(storage);
// We need to parse properly formed JSON and then extract
// "loggers" element from it. For some reason fromJSON is
// throwing at opening square bracket
ConstElementPtr config = Element::fromJSON(config_txt);
config = config->get("loggers");
std::ostringstream oss;
oss << "invalid path in `output`: invalid path specified: '/tmp', supported path is '"
<< LogConfigParser::getLogPath()
<< "' (<string>:1:82)";
ASSERT_THROW_MSG(parser.parseConfiguration(config), BadValue,
oss.str());
}
// Verifies that an invalid output path results in a
// warning when security enforcement is disabled.
TEST_F(LoggingTest, enforceSecurityFalse) {
file::PathChecker::enableEnforcement(false);
const char* config_txt =
"{ \"loggers\": ["
" {"
" \"name\": \"kea\","
" \"output-options\": ["
" {"
" \"output\": \"/tmp/myfile.log\""
" }"
" ],"
" \"severity\": \"INFO\""
" }"
"]}";
ConfigPtr storage(new ConfigBase());
LogConfigParser parser(storage);
// We need to parse properly formed JSON and then extract
// "loggers" element from it. For some reason fromJSON is
// throwing at opening square bracket
ConstElementPtr config = Element::fromJSON(config_txt);
config = config->get("loggers");
ASSERT_NO_THROW_LOG(parser.parseConfiguration(config));
ASSERT_EQ(1, storage->getLoggingInfo().size());
EXPECT_EQ("kea", storage->getLoggingInfo()[0].name_);
EXPECT_EQ(isc::log::INFO, storage->getLoggingInfo()[0].severity_);
ASSERT_EQ(1, storage->getLoggingInfo()[0].destinations_.size());
EXPECT_EQ("/tmp/myfile.log" , storage->getLoggingInfo()[0].destinations_[0].output_);
std::ostringstream oss;
oss << "DCTL_LOG_PATH_SECURITY_WARNING Log output path specified is NOT SECURE:"
<< " invalid path specified: '/tmp', supported path is '"
<< LogConfigParser::getLogPath() << "'";
EXPECT_EQ(1, countFile(oss.str()));
}
/// @todo Add tests for malformed logging configuration
/// @todo There is no easy way to test applyConfiguration() and defaultLogging().

37
src/lib/util/filesystem.cc
View File

@@ -284,17 +284,19 @@ PathChecker::validatePath(const std::string input_path_str,
auto parent_path = input_path.parentPath();
auto parent_dir = input_path.parentDirectory();
if (!parent_dir.empty()) {
if (!enforce_path) {
// Security set to lax, let it fly.
return (input_path_str);
}
// We only allow absolute path equal to default. Catch an invalid path.
if ((parent_path != path_) || (parent_dir == "/")) {
isc_throw(BadValue, "invalid path specified: '"
<< (parent_path.empty() ? "/" : parent_path)
<< "', supported path is '"
<< path_ << "'");
std::ostringstream oss;
oss << "invalid path specified: '"
<< (parent_path.empty() ? "/" : parent_path)
<< "', supported path is '"
<< path_ << "'";
if (enforce_path) {
isc_throw(SecurityError, oss.str());
} else {
isc_throw(SecurityWarn, oss.str());
}
}
}
@@ -306,10 +308,6 @@ std::string
PathChecker::validateDirectory(const std::string input_path_str,
bool enforce_path /* = PathChecker::shouldEnforceSecurity() */) const {
std::string input_copy = trim(input_path_str);
if (!enforce_path) {
return(input_copy);
}
// We only allow absolute path equal to default. Catch an invalid path.
if (!input_path_str.empty()) {
std::string input_copy = input_path_str;
@@ -318,9 +316,16 @@ PathChecker::validateDirectory(const std::string input_path_str,
}
if (input_copy != path_) {
isc_throw(BadValue, "invalid path specified: '"
<< input_path_str << "', supported path is '"
<< path_ << "'");
std::ostringstream oss;
oss << "invalid path specified: '"
<< input_path_str << "', supported path is '"
<< path_ << "'";
if (enforce_path) {
isc_throw(SecurityError, oss.str());
} else {
isc_throw(SecurityWarn, oss.str());
}
}
}

37
src/lib/util/filesystem.h
View File

@@ -7,6 +7,7 @@
#ifndef KEA_UTIL_FILESYSTEM_H
#define KEA_UTIL_FILESYSTEM_H
#include <exceptions/exceptions.h>
#include <sys/stat.h>
#include <string>
#include <boost/shared_ptr.hpp>
@@ -15,6 +16,25 @@ namespace isc {
namespace util {
namespace file {
/// @brief A generic exception that is thrown if a parameter given
/// violates security check but enforcement is lax.
class SecurityWarn : public Exception {
public:
SecurityWarn(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
/// @brief A generic exception that is thrown if a parameter given
/// violates security and enforcement is true.
class SecurityError : public Exception {
public:
SecurityError(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
/// @brief A generic exception that is thrown if a parameter given
/// violates security check but enfordement is lax.
/// @brief Get the content of a regular file.
///
/// @param file_name The file name.
@@ -216,13 +236,14 @@ public:
/// returns valid path using the supported path and the input path name.
///
/// @param input_path_str file path to validate.
/// @param enforce_path enables validation against the supported path. If false
/// verifies only that the path contains a file name.
/// @param enforce_path If true throw SecurityError when validation against the
/// supported path fails, if false throw SecurityWarn.
///
/// @return validated path as a string (supported path + input file name)
///
/// @throw BadValue if the input path does not include a file name or if the
/// it the parent path does not path the supported path.
/// @throw BadValue if the input path does not include a file name.
/// SecurityError if the parent path does not path the supported path and
/// security is being enforced, SecurityWarn if it is not being enforced.
std::string validatePath(const std::string input_path_str,
bool enforce_path = shouldEnforceSecurity()) const;
@@ -236,13 +257,13 @@ public:
/// the supported path. Otherwise it throws an error.
///
/// @param input_path_str file path to validate.
/// @param enforce_path enables validation against the supported path. If
/// it simply returns the supported path.
/// @param enforce_path If true throw SecurityError when validation against the
/// supported path fails, if false throw SecurityWarn.
///
/// @return validated path
///
/// @throw BadValue if the input directory does not match the supported
/// path.
/// @throw SecurityError if the path does not match the supported path and
/// security is being enforced, SecurityWarn if it is not being enforced.
std::string validateDirectory(const std::string input_path_str,
bool enforce_path = shouldEnforceSecurity()) const;

86
src/lib/util/tests/filesystem_unittests.cc
View File

@@ -305,6 +305,7 @@ TEST_F(PathCheckerTest, validatePathEnforcePath) {
std::string lib_path_;
std::string exp_path_;
std::string exp_error_;
bool exp_security_err_;
};
std::list<Scenario> scenarios = {
@@ -313,42 +314,48 @@ TEST_F(PathCheckerTest, validatePathEnforcePath) {
__LINE__,
"/mylib.so",
"",
string("invalid path specified: '/', supported path is '" + def_path + "'")
string("invalid path specified: '/', supported path is '" + def_path + "'"),
true
},
{
// Invalid parent path.
__LINE__,
"/var/lib/bs/mylib.so",
"",
string("invalid path specified: '/var/lib/bs', supported path is '" + def_path + "'")
string("invalid path specified: '/var/lib/bs', supported path is '" + def_path + "'"),
true
},
{
// No file name.
__LINE__,
def_path + "/",
"",
string ("path: '" + def_path + "/' has no filename")
string ("path: '" + def_path + "/' has no filename"),
false
},
{
// File name only is valid.
__LINE__,
"mylib.so",
def_path + "/mylib.so",
""
"",
false
},
{
// Valid full path.
__LINE__,
def_path + "/mylib.so",
def_path + "/mylib.so",
""
"",
false
},
{
// White space for file name.
__LINE__,
" ",
"",
string("path: '' has no filename")
string("path: '' has no filename"),
false
},
{
// Invalid relative path.
@@ -356,7 +363,8 @@ TEST_F(PathCheckerTest, validatePathEnforcePath) {
"../kea/mylib.so",
"",
string("invalid path specified: '../kea', supported path is '" +
def_path + "'")
def_path + "'"),
true
}
};
@@ -372,9 +380,15 @@ TEST_F(PathCheckerTest, validatePathEnforcePath) {
checker.validatePath(scenario.lib_path_));
EXPECT_EQ(validated_path, scenario.exp_path_);
} else {
ASSERT_THROW_MSG(validated_path =
checker.validatePath(scenario.lib_path_),
BadValue, scenario.exp_error_);
if (scenario.exp_security_err_) {
ASSERT_THROW_MSG(validated_path =
checker.validatePath(scenario.lib_path_),
SecurityError, scenario.exp_error_);
} else {
ASSERT_THROW_MSG(validated_path =
checker.validatePath(scenario.lib_path_),
BadValue, scenario.exp_error_);
}
}
}
}
@@ -387,6 +401,7 @@ TEST_F(PathCheckerTest, validatePathEnforcePathFalse) {
std::string lib_path_;
std::string exp_path_;
std::string exp_error_;
bool exp_security_warn_;
};
std::list<Scenario> scenarios = {
@@ -395,42 +410,49 @@ TEST_F(PathCheckerTest, validatePathEnforcePathFalse) {
__LINE__,
"/mylib.so",
"/mylib.so",
"",
string("invalid path specified: '/', supported path is '" + def_path + "'"),
true
},
{
// Invalid parent path but shouldn't care.
// Invalid parent path.
__LINE__,
"/var/lib/bs/mylib.so",
"/var/lib/bs/mylib.so",
""
string("invalid path specified: '/var/lib/bs', supported path is '"
+ def_path + "'"),
true
},
{
// No file name.
__LINE__,
def_path + "/",
"",
string ("path: '" + def_path + "/' has no filename")
string ("path: '" + def_path + "/' has no filename"),
false
},
{
// File name only is valid.
__LINE__,
"mylib.so",
def_path + "/mylib.so",
""
"",
false
},
{
// Valid full path.
__LINE__,
def_path + "/mylib.so",
def_path + "/mylib.so",
""
"",
false
},
{
// White space for file name.
__LINE__,
" ",
"",
string("path: '' has no filename")
string("path: '' has no filename"),
false
}
};
@@ -450,9 +472,15 @@ TEST_F(PathCheckerTest, validatePathEnforcePathFalse) {
checker.validatePath(scenario.lib_path_));
EXPECT_EQ(validated_path, scenario.exp_path_);
} else {
ASSERT_THROW_MSG(validated_path =
checker.validatePath(scenario.lib_path_),
BadValue, scenario.exp_error_);
if (scenario.exp_security_warn_) {
ASSERT_THROW_MSG(validated_path =
checker.validatePath(scenario.lib_path_),
SecurityWarn, scenario.exp_error_);
} else {
ASSERT_THROW_MSG(validated_path =
checker.validatePath(scenario.lib_path_),
BadValue, scenario.exp_error_);
}
}
}
}
@@ -527,7 +555,7 @@ TEST_F(PathCheckerTest, validateDirectoryEnforcePath) {
} else {
ASSERT_THROW_MSG(validated_path =
checker.validateDirectory(scenario.lib_path_),
BadValue, scenario.exp_error_);
SecurityError, scenario.exp_error_);
}
}
}
@@ -547,21 +575,21 @@ TEST_F(PathCheckerTest, validateDirectoryEnforcePathFalse) {
// Invalid parent path but shouldn't care.
__LINE__,
"/var/lib/bs/",
"/var/lib/bs/",
""
"",
string("invalid path specified: '/var/lib/bs/', supported path is '" + def_path + "'")
},
{
// File name only is valid.
// File name only is invalid.
__LINE__,
"mylib.so",
"mylib.so",
""
"",
string("invalid path specified: 'mylib.so', supported path is '" + def_path + "'")
},
{
// Valid full path.
__LINE__,
def_path + "/",
def_path + "/",
def_path,
""
},
{
@@ -569,7 +597,7 @@ TEST_F(PathCheckerTest, validateDirectoryEnforcePathFalse) {
__LINE__,
" ",
"",
""
string("invalid path specified: ' ', supported path is '" + def_path + "'")
}
};
@@ -591,7 +619,7 @@ TEST_F(PathCheckerTest, validateDirectoryEnforcePathFalse) {
} else {
ASSERT_THROW_MSG(validated_path =
checker.validateDirectory(scenario.lib_path_),
BadValue, scenario.exp_error_);
SecurityWarn, scenario.exp_error_);
}
}
}