From: Thomas Markwalder Date: Mon, 9 Jun 2025 17:08:08 +0000 (-0400) Subject: [#3848] Warn on invalid paths when security disabled X-Git-Tag: Kea-3.1.0~69 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=b5aeb99f98c8a0185e09751b0328bbc0241ba70e;p=thirdparty%2Fkea.git [#3848] Warn on invalid paths when security disabled Warn but still use invalid paths when security is disabled. --- diff --git a/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc b/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc index 1434819019..29f6e9d51e 100644 --- a/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc +++ b/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc @@ -17,7 +17,9 @@ #include #include #include +#include #include +#include #include @@ -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 diff --git a/src/hooks/dhcp/host_cache/host_cache.cc b/src/hooks/dhcp/host_cache/host_cache.cc index 3ceb9abbf9..3c2834e833 100644 --- a/src/hooks/dhcp/host_cache/host_cache.cc +++ b/src/hooks/dhcp/host_cache/host_cache.cc @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -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()); } diff --git a/src/hooks/dhcp/host_cache/host_cache_messages.cc b/src/hooks/dhcp/host_cache/host_cache_messages.cc index c1f2e58d28..6547937ff8 100644 --- a/src/hooks/dhcp/host_cache/host_cache_messages.cc +++ b/src/hooks/dhcp/host_cache/host_cache_messages.cc @@ -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 }; diff --git a/src/hooks/dhcp/host_cache/host_cache_messages.h b/src/hooks/dhcp/host_cache/host_cache_messages.h index ccfda0ead2..9b786decaa 100644 --- a/src/hooks/dhcp/host_cache/host_cache_messages.h +++ b/src/hooks/dhcp/host_cache/host_cache_messages.h @@ -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 diff --git a/src/hooks/dhcp/host_cache/host_cache_messages.mes b/src/hooks/dhcp/host_cache/host_cache_messages.mes index e31287ff25..afb3981b81 100644 --- a/src/hooks/dhcp/host_cache/host_cache_messages.mes +++ b/src/hooks/dhcp/host_cache/host_cache_messages.mes @@ -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. diff --git a/src/hooks/dhcp/host_cache/tests/command_unittests.cc b/src/hooks/dhcp/host_cache/tests/command_unittests.cc index 8c85414053..b9bb5513e2 100644 --- a/src/hooks/dhcp/host_cache/tests/command_unittests.cc +++ b/src/hooks/dhcp/host_cache/tests/command_unittests.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -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(); diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index a9ce781355..389cb40978 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -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()); } diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc index 862f186499..9deb7f3d61 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.cc @@ -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", diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h index e5f5baf9e4..c2baa81c8c 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.h @@ -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; diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes index ccf760fbf9..34e023454c 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes +++ b/src/hooks/dhcp/lease_cmds/lease_cmds_messages.mes @@ -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. diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc index a405da4d00..a4fb0a61d3 100644 --- a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -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 diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds_unittest.h b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds_unittest.h index 682edd0493..91e76eae83 100644 --- a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds_unittest.h +++ b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds_unittest.h @@ -18,8 +18,10 @@ #include #include #include +#include #include #include +#include #include @@ -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 diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/meson.build b/src/hooks/dhcp/lease_cmds/libloadtests/meson.build index 6cb9cf966a..2e6c095222 100644 --- a/src/hooks/dhcp/lease_cmds/libloadtests/meson.build +++ b/src/hooks/dhcp/lease_cmds/libloadtests/meson.build @@ -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', diff --git a/src/lib/config/config_messages.cc b/src/lib/config/config_messages.cc index 6152b0f1a6..bfc4fe9899 100644 --- a/src/lib/config/config_messages.cc +++ b/src/lib/config/config_messages.cc @@ -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", diff --git a/src/lib/config/config_messages.h b/src/lib/config/config_messages.h index 3619abdea2..890ea28053 100644 --- a/src/lib/config/config_messages.h +++ b/src/lib/config/config_messages.h @@ -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; diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes index 5c12011a0f..752083c167 100644 --- a/src/lib/config/config_messages.mes +++ b/src/lib/config/config_messages.mes @@ -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. diff --git a/src/lib/config/tests/unix_command_config_unittests.cc b/src/lib/config/tests/unix_command_config_unittests.cc index b0965c065d..a7e501ccb3 100644 --- a/src/lib/config/tests/unix_command_config_unittests.cc +++ b/src/lib/config/tests/unix_command_config_unittests.cc @@ -13,6 +13,7 @@ #include #include #include +#include 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 diff --git a/src/lib/config/unix_command_config.cc b/src/lib/config/unix_command_config.cc index c058510712..4b1e2153ad 100644 --- a/src/lib/config/unix_command_config.cc +++ b/src/lib/config/unix_command_config.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -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() diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.cc b/src/lib/dhcpsrv/dhcpsrv_messages.cc index f47bbfe464..50cbed4f79 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.cc +++ b/src/lib/dhcpsrv/dhcpsrv_messages.cc @@ -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 }; diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.h b/src/lib/dhcpsrv/dhcpsrv_messages.h index bc69ddb0ef..c0a1af1825 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.h +++ b/src/lib/dhcpsrv/dhcpsrv_messages.h @@ -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 diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index b49a8ae835..6e6dc5a72f 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -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. diff --git a/src/lib/dhcpsrv/legal_log_mgr.cc b/src/lib/dhcpsrv/legal_log_mgr.cc index d654e1cc5a..d7f649ba9b 100644 --- a/src/lib/dhcpsrv/legal_log_mgr.cc +++ b/src/lib/dhcpsrv/legal_log_mgr.cc @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -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()); diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index f706aa9710..3d6075fb9f 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -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); } diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 0cad62ab61..ec0286a0d0 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -27,6 +27,8 @@ #include #include #include +#include +#include #include @@ -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 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 diff --git a/src/lib/hooks/hooks_messages.cc b/src/lib/hooks/hooks_messages.cc index b06d563e0b..2ac1ac0285 100644 --- a/src/lib/hooks/hooks_messages.cc +++ b/src/lib/hooks/hooks_messages.cc @@ -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", diff --git a/src/lib/hooks/hooks_messages.h b/src/lib/hooks/hooks_messages.h index f79e0943fa..778bb77243 100644 --- a/src/lib/hooks/hooks_messages.h +++ b/src/lib/hooks/hooks_messages.h @@ -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; diff --git a/src/lib/hooks/hooks_messages.mes b/src/lib/hooks/hooks_messages.mes index 3812a60765..9cb191f176 100644 --- a/src/lib/hooks/hooks_messages.mes +++ b/src/lib/hooks/hooks_messages.mes @@ -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. diff --git a/src/lib/hooks/hooks_parser.cc b/src/lib/hooks/hooks_parser.cc index 26beef75ca..9c94509c7d 100644 --- a/src/lib/hooks/hooks_parser.cc +++ b/src/lib/hooks/hooks_parser.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -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 diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc index 1779da423a..7562a1bddf 100644 --- a/src/lib/hooks/tests/hooks_manager_unittest.cc +++ b/src/lib/hooks/tests/hooks_manager_unittest.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #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 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 diff --git a/src/lib/hooks/tests/meson.build b/src/lib/hooks/tests/meson.build index bc38fcac4b..6bcadfa4c1 100644 --- a/src/lib/hooks/tests/meson.build +++ b/src/lib/hooks/tests/meson.build @@ -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', diff --git a/src/lib/process/log_parser.cc b/src/lib/process/log_parser.cc index 944161abb8..e585932754 100644 --- a/src/lib/process/log_parser.cc +++ b/src/lib/process/log_parser.cc @@ -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& destination, diff --git a/src/lib/process/process_messages.cc b/src/lib/process/process_messages.cc index 3f013b1a0b..b0c6711620 100644 --- a/src/lib/process/process_messages.cc +++ b/src/lib/process/process_messages.cc @@ -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", diff --git a/src/lib/process/process_messages.h b/src/lib/process/process_messages.h index 652dc70410..df99a467f0 100644 --- a/src/lib/process/process_messages.h +++ b/src/lib/process/process_messages.h @@ -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; diff --git a/src/lib/process/process_messages.mes b/src/lib/process/process_messages.mes index 92c595bc76..1754f048c6 100644 --- a/src/lib/process/process_messages.mes +++ b/src/lib/process/process_messages.mes @@ -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. diff --git a/src/lib/process/tests/log_parser_unittests.cc b/src/lib/process/tests/log_parser_unittests.cc index 87eef1a599..08f88a1882 100644 --- a/src/lib/process/tests/log_parser_unittests.cc +++ b/src/lib/process/tests/log_parser_unittests.cc @@ -9,17 +9,21 @@ #include #include #include +#include #include #include #include +#include #include #include +#include #include 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() + << "' (: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(). diff --git a/src/lib/util/filesystem.cc b/src/lib/util/filesystem.cc index 7d29bdb93f..29d12a67d2 100644 --- a/src/lib/util/filesystem.cc +++ b/src/lib/util/filesystem.cc @@ -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()); + } } } diff --git a/src/lib/util/filesystem.h b/src/lib/util/filesystem.h index 7b93a61db1..587bdf366f 100644 --- a/src/lib/util/filesystem.h +++ b/src/lib/util/filesystem.h @@ -7,6 +7,7 @@ #ifndef KEA_UTIL_FILESYSTEM_H #define KEA_UTIL_FILESYSTEM_H +#include #include #include #include @@ -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; diff --git a/src/lib/util/tests/filesystem_unittests.cc b/src/lib/util/tests/filesystem_unittests.cc index e925dcad99..8164aed026 100644 --- a/src/lib/util/tests/filesystem_unittests.cc +++ b/src/lib/util/tests/filesystem_unittests.cc @@ -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 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 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_); } } }