[#1680] added check for execurtable permissions

diff --git a/src/hooks/dhcp/run_script/libloadtests/Makefile.am b/src/hooks/dhcp/run_script/libloadtests/Makefile.am
index 40de57219199e3a9ca88450413cccf08de42ee84..439045af15aca44cef04379a2e371fb86da81ad5 100644 (file)
--- a/src/hooks/dhcp/run_script/libloadtests/Makefile.am
+++ b/src/hooks/dhcp/run_script/libloadtests/Makefile.am
@@ -4,6 +4,7 @@ AM_CPPFLAGS = -I$(top_builddir)/src/lib -I$(top_srcdir)/src/lib
 AM_CPPFLAGS += -I$(top_builddir)/src/hooks/dhcp/run_script -I$(top_srcdir)/src/hooks/dhcp/run_script
 AM_CPPFLAGS += $(BOOST_INCLUDES)
 AM_CPPFLAGS += -DLIBRUN_SCRIPT_SO=\"$(abs_top_builddir)/src/hooks/dhcp/run_script/.libs/libdhcp_run_script.so\"
+AM_CPPFLAGS += -DRUN_SCRIPT_TEST_SH=\"$(abs_top_builddir)/src/hooks/dhcp/run_script/tests/run_script_test.sh\"
 AM_CPPFLAGS += -DINSTALL_PROG=\"$(abs_top_srcdir)/install-sh\"
 
 AM_CXXFLAGS = $(KEA_CXXFLAGS)

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 1947c48b7f6152e9d6008080144a230791f3ef5a..b924b8ddf713ddcce4b279214f47ecd98d9c0024 100644 (file)
--- a/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc
+++ b/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc
@@ -75,7 +75,7 @@ public:
 TEST_F(LibLoadTest, validLoad) {
     // Prepare parameters for the callout parameters library.
     ElementPtr params = Element::createMap();
-    ElementPtr name = Element::create("test_script.sh");
+    ElementPtr name = Element::create(RUN_SCRIPT_TEST_SH);
     params->set("name", name);
     ElementPtr sync = Element::create(false);
     params->set("sync", sync);
@@ -104,12 +104,16 @@ TEST_F(LibLoadTest, invalidLoad) {
     params->set("name", name);
     EXPECT_FALSE(loadLibs());
 
-    // Use valid name parameter type but use invalid sync parameter type.
+    // Use invalid name parameter.
     name = Element::create("script_name.sh");
     params->set("name", name);
+    EXPECT_FALSE(loadLibs());
+
+    // Use valid name parameter type but use invalid sync parameter type.
+    name = Element::create(RUN_SCRIPT_TEST_SH);
+    params->set("name", name);
     ElementPtr sync = Element::create("data");
     params->set("sync", sync);
-
     EXPECT_FALSE(loadLibs());
 }
 

diff --git a/src/hooks/dhcp/run_script/run_script.cc b/src/hooks/dhcp/run_script/run_script.cc
index a7748e95d86d7e06e50a12aaca818725221e4921..d9a4eab58863d36a8ea2a5be1d94e22addde2055 100644 (file)
--- a/src/hooks/dhcp/run_script/run_script.cc
+++ b/src/hooks/dhcp/run_script/run_script.cc
@@ -32,6 +32,15 @@ RunScriptImpl::configure(LibraryHandle& handle) {
     if (name->getType() != Element::string) {
         isc_throw(InvalidParameter, "The 'name' parameter must be a string");
     }
+    IOServicePtr io_service(new asiolink::IOService());
+    try {
+        ProcessSpawn process(io_service, name->stringValue());
+        if (!process.checkPermissions()) {
+            isc_throw(InvalidParameter, "The 'name' parameter must point to an executable");
+        }
+    } catch (const isc::Exception& ex) {
+        isc_throw(InvalidParameter, "Invalid 'name' parameter: " << ex.what());
+    }
     setName(name->stringValue());
     ConstElementPtr sync = handle.getParameter("sync");
     if (sync) {

diff --git a/src/lib/asiolink/process_spawn.cc b/src/lib/asiolink/process_spawn.cc
index 2f607182ec4bce9749dc2564fc9b7fa15306320c..a7ef6fe48fcccee933f631b8b9f4189d666a07bc 100644 (file)
--- a/src/lib/asiolink/process_spawn.cc
+++ b/src/lib/asiolink/process_spawn.cc
@@ -17,6 +17,7 @@
 #include <stdlib.h>
 #include <errno.h>
 #include <unistd.h>
+#include <sys/stat.h>
 #include <sys/wait.h>
 
 using namespace std;
@@ -131,6 +132,12 @@ public:
     /// @param pid A process pid.
     void clearState(const pid_t pid);
 
+    /// @brief Check executable permissions.
+    ///
+    /// @return true if file has executable permissions, false otherwise.
+    /// @throw ProcessSpawnError if file does not exist.
+    bool checkPermissions() const;
+
 private:
 
     /// @brief Copies the argument specified as a C++ string to the new
@@ -357,6 +364,18 @@ ProcessSpawn::ProcessSpawn(IOServicePtr io_service,
     : impl_(new ProcessSpawnImpl(io_service, executable, args, vars)) {
 }
 
+bool
+ProcessSpawnImpl::checkPermissions() const {
+    struct stat st;
+    if (stat(executable_.c_str(), &st)) {
+        isc_throw(ProcessSpawnError, "File not found: " << executable_);
+    }
+    if (st.st_mode & S_IEXEC) {
+        return (true);
+    }
+    return (false);
+}
+
 std::string
 ProcessSpawn::getCommandLine() const {
     return (impl_->getCommandLine());
@@ -387,5 +406,10 @@ ProcessSpawn::clearState(const pid_t pid) {
     return (impl_->clearState(pid));
 }
 
+bool
+ProcessSpawn::checkPermissions() const {
+    return (impl_->checkPermissions());
+}
+
 } // namespace asiolink
 } // namespace isc

diff --git a/src/lib/asiolink/process_spawn.h b/src/lib/asiolink/process_spawn.h
index ca72ce897547be94b48c631976d7a6b213a5f0b5..c810f50bd63007ed520eb243193b6277c85ab854 100644 (file)
--- a/src/lib/asiolink/process_spawn.h
+++ b/src/lib/asiolink/process_spawn.h
@@ -137,6 +137,12 @@ public:
     /// @param pid A process pid.
     void clearState(const pid_t pid);
 
+    /// @brief Check executable permissions.
+    ///
+    /// @return true if file has executable permissions, false otherwise.
+    /// @throw ProcessSpawnError if file does not exist.
+    bool checkPermissions() const;
+
 private:
 
     /// @brief A smart pointer to the implementation of this class.

diff --git a/src/lib/asiolink/tests/process_spawn_unittest.cc b/src/lib/asiolink/tests/process_spawn_unittest.cc
index 11bb26d7d65f086a152c3fef9978015a450e0d8a..b1fe9c0b008f5aa74bced2c55f2456590833f959 100644 (file)
--- a/src/lib/asiolink/tests/process_spawn_unittest.cc
+++ b/src/lib/asiolink/tests/process_spawn_unittest.cc
@@ -16,6 +16,8 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include <testutils/gtest_utils.h>
+
 namespace {
 
 using namespace isc;
@@ -343,4 +345,19 @@ TEST_F(ProcessSpawnTest, isRunning) {
     ASSERT_EQ(SIGCHLD, processed_signals_[0]);
 }
 
+// This test verifies that the checkPermissions function throws if the file does
+// not exist and returns true or false if the file is or it is not executable.
+TEST_F(ProcessSpawnTest, checkPermissions) {
+    ProcessSpawn no_process(io_service_, "no-file");
+    EXPECT_THROW_MSG(no_process.checkPermissions(), ProcessSpawnError,
+                     "File not found: no-file");
+
+    std::string name = TEST_SCRIPT_SH;
+    name += ".in";
+    ProcessSpawn invalid_process(io_service_, name);
+    ASSERT_FALSE(invalid_process.checkPermissions());
+    ProcessSpawn process(io_service_, TEST_SCRIPT_SH);
+    ASSERT_TRUE(process.checkPermissions());
+}
+
 } // end of anonymous namespace