[#3849] restrict location of configured scripts in loaded hooks

ChangeLog

@@ -1,3 +1,8 @@
+2353.	[func]		razvan
+	Restricted location of configured scripts in loaded hook
+	libraries.
+	(Gitlab #3849)
+
 2352.	[bug]		razvan
 	Fix error handling when detecting a global reservation for the
 	client and global reservatons are explicitly disabled in the

doc/sphinx/arm/hooks-legal-log.rst

@@ -165,6 +165,10 @@ script and are configured with the following settings:
 -  ``postrotate`` - an external executable or script called with the name of the
    file that was opened. Kea does not wait for the process to finish.
 
+These executables must be stored in the ``"[kea-install-dir]/share/kea/scripts/"``
+directory which can be overridden at startup by setting the environment variable
+``KEA_HOOK_SCRIPTS_PATH`` to a different path.
+
 Custom formatting can be enabled for logging information that can be extracted
 either from the client's request packet or from the server's response packet.
 Use with caution as this might affect server performance.

doc/sphinx/arm/hooks-run-script.rst

@@ -38,6 +38,10 @@ If the ``sync`` parameter is ``false``, then the script will launch and Kea
 will not wait for the execution to finish, causing all the OUT parameters of
 the script (including the next step) to be ignored.
 
+The external script must be stored in the ``"[kea-install-dir]/share/kea/scripts/"``
+directory which can be overridden at startup by setting the environment variable
+``KEA_HOOK_SCRIPTS_PATH`` to a different path.
+
 .. note::
 
    The script inherits all privileges from the server which calls it.

doc/sphinx/arm/security.rst

@@ -426,6 +426,8 @@ the following table:
 +-------------------------------------+---------------------------------------+-------------------------------+
 | Unix Sockets                        | ``var/run/kea``                       | ``KEA_CONTROL_SOCKET_DIR``    |
 +-------------------------------------+---------------------------------------+-------------------------------+
+| Scripts used by hook libraries      | ``share/kea/scripts/``                | ``KEA_HOOK_SCRIPTS_PATH``     |
++-------------------------------------+---------------------------------------+-------------------------------+
 
 .. note:
 

src/hooks/dhcp/forensic_log/rotating_file.cc

@@ -7,6 +7,7 @@
 #include <config.h>
 
 #include <asiolink/process_spawn.h>
+#include <hooks/hooks_parser.h>
 #include <legal_log_log.h>
 #include <rotating_file.h>
 #include <util/multi_threading_mgr.h>
@@ -25,6 +26,7 @@ using namespace isc::util;
 using namespace isc::dhcp;
 using namespace isc::data;
 using namespace isc::db;
+using namespace isc::hooks;
 using namespace std;
 
 namespace isc {
@@ -104,6 +106,7 @@ RotatingFile::apply(const DatabaseConnection::ParameterMap& parameters) {
 
     if (!prerotate_.empty()) {
         try {
+            HookLibraryScriptsChecker::validatePath(prerotate_);
             ProcessSpawn process(ProcessSpawn::ASYNC, prerotate_);
         } catch (const isc::Exception& ex) {
             isc_throw(LegalLogMgrError, "Invalid 'prerotate' parameter: " << ex.what());
@@ -112,6 +115,7 @@ RotatingFile::apply(const DatabaseConnection::ParameterMap& parameters) {
 
     if (!postrotate_.empty()) {
         try {
+            HookLibraryScriptsChecker::validatePath(postrotate_);
             ProcessSpawn process(ProcessSpawn::ASYNC, postrotate_);
         } catch (const isc::Exception& ex) {
             isc_throw(LegalLogMgrError, "Invalid 'postrotate' parameter: " << ex.what());

src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc

@@ -14,6 +14,7 @@
 #include <testutils/gtest_utils.h>
 #include <dhcpsrv/legal_log_mgr_factory.h>
 #include <dhcpsrv/legal_log_db_log.h>
+#include <hooks/hooks_parser.h>
 #include <legal_log_log.h>
 #include <rotating_file.h>
 #include <testutils/env_var_wrapper.h>
@@ -26,6 +27,7 @@ using namespace isc;
 using namespace isc::data;
 using namespace isc::db;
 using namespace isc::dhcp;
+using namespace isc::hooks;
 using namespace isc::legal_log;
 using namespace isc::test;
 using namespace std;
@@ -41,6 +43,7 @@ DbLogger legal_log_db_logger(legal_log_logger, legal_log_db_message_map);
 struct LegalLogMgrTest : ::testing::Test {
     /// @brief Constructor.
     LegalLogMgrTest() : legal_log_dir_env_var_("KEA_LEGAL_LOG_DIR") {
+        HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR);
     }
 
     /// @brief Destructor.

src/hooks/dhcp/forensic_log/tests/test_utils.h

@@ -10,6 +10,7 @@
 #include <exceptions/exceptions.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <hooks/callout_manager.h>
+#include <hooks/hooks_parser.h>
 #include <dhcpsrv/legal_log_mgr.h>
 #include <rotating_file.h>
 #include <util/reconnect_ctl.h>
@@ -147,6 +148,7 @@ public:
 
     /// @brief Constructor
     RotatingFileTest() : time_(RotatingFileTest::getTime()) {
+        HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR);
     }
 
     /// @brief Destructor

src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc

@@ -13,6 +13,7 @@
 #include <config.h>
 
 #include <dhcpsrv/testutils/lib_load_test_fixture.h>
+#include <hooks/hooks_parser.h>
 #include <testutils/gtest_utils.h>
 
 #include <gtest/gtest.h>
@@ -32,6 +33,7 @@ class RunScriptLibLoadTest : public isc::test::LibLoadTest {
 public:
     /// @brief Constructor
     RunScriptLibLoadTest() : LibLoadTest(LIBRUN_SCRIPT_SO) {
+        HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR);
     }
 
     /// @brief Destructor

src/hooks/dhcp/run_script/libloadtests/meson.build

@@ -7,6 +7,7 @@ dhcp_run_script_libload_tests = executable(
     'load_unload_unittests.cc',
     'run_unittests.cc',
     cpp_args: [
+        f'-DTEST_DATA_BUILDDIR="@current_build_dir@"',
         f'-DLIBRUN_SCRIPT_SO="@TOP_BUILD_DIR@/src/hooks/dhcp/run_script/libdhcp_run_script.so"',
         f'-DRUN_SCRIPT_TEST_SH="@TOP_BUILD_DIR@/src/hooks/dhcp/run_script/tests/run_script_test.sh"',
     ],

src/hooks/dhcp/run_script/run_script.h

@@ -18,6 +18,7 @@
 #include <dhcpsrv/lease.h>
 #include <dhcpsrv/subnet.h>
 #include <hooks/library_handle.h>
+#include <hooks/hooks_parser.h>
 #include <string>
 
 namespace isc {
@@ -217,6 +218,7 @@ public:
     ///
     /// @param name The name of the target script.
     void setName(const std::string& name) {
+        isc::hooks::HookLibraryScriptsChecker::validatePath(name);
         name_ = name;
     }
 

src/hooks/dhcp/run_script/tests/meson.build

@@ -16,6 +16,7 @@ dhcp_run_script_lib_tests = executable(
     'run_script_unittests.cc',
     'run_unittests.cc',
     cpp_args: [
+        f'-DTEST_DATA_BUILDDIR="@current_build_dir@"',
         f'-DRUN_SCRIPT_LIB_SO="@TOP_BUILD_DIR@/src/hooks/dhcp/run_script/libdhcp_run_script.so"',
         f'-DTEST_LOG_FILE="@current_build_dir@/test.log"',
         f'-DRUN_SCRIPT_TEST_SH="@current_build_dir@/run_script_test.sh"',

src/hooks/dhcp/run_script/tests/run_script_unittests.cc

@@ -824,6 +824,7 @@ public:
     /// @brief Constructor.
     RunScriptTest() :
         co_manager_(new CalloutManager(1)), io_service_(new IOService()) {
+        HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR);
         ProcessSpawn::setIOService(io_service_);
         clearLogFile();
     }

src/lib/hooks/hooks_parser.cc

@@ -27,8 +27,32 @@ namespace hooks {
 namespace {
     // Singleton PathChecker to set and hold valid hooks library path.
     PathCheckerPtr hooks_path_checker_;
+
+    // Singleton PathChecker to set and hold valid scripts path (scripts loaded by hook libraries).
+    PathCheckerPtr hook_scripts_path_checker_;
 };
 
+std::string
+HookLibraryScriptsChecker::getHookScriptsPath(bool reset /* = false */, const std::string explicit_path /* = "" */) {
+    if (!hook_scripts_path_checker_ || reset) {
+        hook_scripts_path_checker_.reset(new PathChecker(DEFAULT_HOOK_SCRIPTS_PATH, "KEA_HOOK_SCRIPTS_PATH"));
+        if (!explicit_path.empty()) {
+            hook_scripts_path_checker_->getPath(true, explicit_path);
+        }
+    }
+
+    return (hook_scripts_path_checker_->getPath());
+}
+
+std::string
+HookLibraryScriptsChecker::validatePath(const std::string libpath) {
+    if (!hook_scripts_path_checker_) {
+        getHookScriptsPath();
+    }
+
+    return (hook_scripts_path_checker_->validatePath(libpath));
+}
+
 std::string
 HooksLibrariesParser::getHooksPath(bool reset /* = false */, const std::string explicit_path /* = "" */) {
     if (!hooks_path_checker_ || reset) {

src/lib/hooks/hooks_parser.h

@@ -83,6 +83,33 @@ public:
                                     const std::string explicit_path = "");
 };
 
+class HookLibraryScriptsChecker {
+public:
+    /// @brief Validates a script path (script loaded by a hook) against the
+    /// supported path.
+    ///
+    /// @param libpath script path to validate.
+    ///
+    /// @return validated path
+    static std::string validatePath(const std::string libpath);
+
+    /// @brief Fetches the supported script path.
+    ///
+    /// The first call to this function with no arguments will set the default
+    /// hooks path to either the value of DEFAULT_HOOK_SCRIPTS_PATH or the environment
+    /// variable KEA_HOOK_SCRIPTS_PATH if it is defined.  Subsequent calls with no
+    /// arguments will simply return this value.
+    ///
+    /// @param reset recalculate when true, defaults to false. This is for
+    /// testing purposes only.
+    /// @param explicit_path set default hooks path to this value. This is
+    /// for testing purposes only.
+    ///
+    /// @return String containing the default script path.
+    static std::string getHookScriptsPath(bool reset = false,
+                                          const std::string explicit_path = "");
+};
+
 }  // namespace isc::hooks
 }  // namespace isc
 

src/lib/hooks/meson.build

@@ -13,7 +13,10 @@ kea_hooks_lib = shared_library(
     'library_manager.cc',
     'library_manager_collection.cc',
     'server_hooks.cc',
-    cpp_args: [f'-DDEFAULT_HOOKS_PATH="@DEFAULT_HOOKS_PATH@"'],
+    cpp_args: [
+        f'-DDEFAULT_HOOK_SCRIPTS_PATH="@PREFIX@/@DATADIR@/kea/scripts"',
+        f'-DDEFAULT_HOOKS_PATH="@DEFAULT_HOOKS_PATH@"'
+    ],
     include_directories: [include_directories('.')] + INCLUDES,
     install: true,
     install_dir: LIBDIR,