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

diff --git a/ChangeLog b/ChangeLog
index 12ee964495c33816ad6b277ab5b02c0800484659..b83203c6d7abb124a5716ccb8218bca9b52cee97 100644 (file)
--- a/ChangeLog
+++ b/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

diff --git a/doc/sphinx/arm/hooks-legal-log.rst b/doc/sphinx/arm/hooks-legal-log.rst
index 149d1c0accb3ab451487cc6757f6c609595c3722..a36e3beb3773aa689d20bdeb860f7b270adb08c6 100644 (file)
--- a/doc/sphinx/arm/hooks-legal-log.rst
+++ b/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.

diff --git a/doc/sphinx/arm/hooks-run-script.rst b/doc/sphinx/arm/hooks-run-script.rst
index b8912b7974dd776682a048b22e67d50427439ec0..31f8ad15745148ac5d5f21d83e553084c65a4d7c 100644 (file)
--- a/doc/sphinx/arm/hooks-run-script.rst
+++ b/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.

diff --git a/doc/sphinx/arm/security.rst b/doc/sphinx/arm/security.rst
index aa4a055a50c1a007c2b1123eefa73c93dfd888f0..8817da71078f1d317887d73a009b9243aeab973b 100644 (file)
--- a/doc/sphinx/arm/security.rst
+++ b/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:
 

diff --git a/src/hooks/dhcp/forensic_log/rotating_file.cc b/src/hooks/dhcp/forensic_log/rotating_file.cc
index 010bc91fa9fcd9fabe5b64bd90bd18025b183d25..4f9b553fe01f525e1f1756bf859ff15bca16f24e 100644 (file)
--- a/src/hooks/dhcp/forensic_log/rotating_file.cc
+++ b/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());

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 798a0ece9724b7f8e6ae99c6f1f3d4ec743a6e86..14348190190f0ce49bd26792264a8e0215362c8a 100644 (file)
--- a/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc
+++ b/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.

diff --git a/src/hooks/dhcp/forensic_log/tests/test_utils.h b/src/hooks/dhcp/forensic_log/tests/test_utils.h
index 8b1335334b2dcd1c4fc116e587b3014b79bebfaa..8e380bd43e242fe3076c538e189c195bbbf71af4 100644 (file)
--- a/src/hooks/dhcp/forensic_log/tests/test_utils.h
+++ b/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

diff --git a/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc b/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc
index 3679d40e01e61d939d6d213e90f3e4ef31f09a91..d68d176a140099d5fc45a90d58e8d78a2fcee561 100644 (file)
--- a/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc
+++ b/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

diff --git a/src/hooks/dhcp/run_script/libloadtests/meson.build b/src/hooks/dhcp/run_script/libloadtests/meson.build
index 59b53c350cc51d3f294fd8cd04498aa9dbad3ce7..1cc805d19b142c31c1e638861552981fa4465f04 100644 (file)
--- a/src/hooks/dhcp/run_script/libloadtests/meson.build
+++ b/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"',
     ],

diff --git a/src/hooks/dhcp/run_script/run_script.h b/src/hooks/dhcp/run_script/run_script.h
index 5ab6d2ab3f20379c4df368c70b4f0dd295b53929..201f0c56c12e9cde5be40b41f30aa0c02404439c 100644 (file)
--- a/src/hooks/dhcp/run_script/run_script.h
+++ b/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;
     }
 

diff --git a/src/hooks/dhcp/run_script/tests/meson.build b/src/hooks/dhcp/run_script/tests/meson.build
index 55f1296d042430fccbf67d8574d5b30bc91a3b06..c69064f4375c24adbb72da49feb0a7f4fd21b08d 100644 (file)
--- a/src/hooks/dhcp/run_script/tests/meson.build
+++ b/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"',

diff --git a/src/hooks/dhcp/run_script/tests/run_script_unittests.cc b/src/hooks/dhcp/run_script/tests/run_script_unittests.cc
index 66a700da62b8a557da396a7b2619f6d6c65bacf9..d6acccf25861b2ed9f5a114d13951936ba7f3f6f 100644 (file)
--- a/src/hooks/dhcp/run_script/tests/run_script_unittests.cc
+++ b/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();
     }

diff --git a/src/lib/hooks/hooks_parser.cc b/src/lib/hooks/hooks_parser.cc
index 26553c2cb84f55bd69de8429158f1b574f563b83..26beef75ca2e329230dae6e8ac5802cd76bdaa75 100644 (file)
--- a/src/lib/hooks/hooks_parser.cc
+++ b/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) {

diff --git a/src/lib/hooks/hooks_parser.h b/src/lib/hooks/hooks_parser.h
index 41e79b5eea36a085c4e48e8bc45718c49c6eee22..dfe0c8b835b16c7161c11821188805689bd31b7e 100644 (file)
--- a/src/lib/hooks/hooks_parser.h
+++ b/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
 

diff --git a/src/lib/hooks/meson.build b/src/lib/hooks/meson.build
index 69f5198b62d4f8e6676650d0cabb8983ca02988b..c3d214fcded3cb2c06998ab394afb7ef70f2f8e7 100644 (file)
--- a/src/lib/hooks/meson.build
+++ b/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,