[#4647] Always treat "." and ".." as invalid

/changelog_unreleased/4647-validatepath-allows-lease-file-path-outside-dhcp-data-dir
    - reworded to be more user friendly

/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
    updated test

/src/lib/util/filesystem.cc
    PathChecker::validatePath() - treat "." and ".." as always invalid

/src/lib/util/tests/filesystem_unittests.cc
    updated test

diff --git a/changelog_unreleased/4647-validatepath-allows-lease-file-path-outside-dhcp-data-dir b/changelog_unreleased/4647-validatepath-allows-lease-file-path-outside-dhcp-data-dir
index 9621a06ca60b19e70f19ebc7ed1bea372516121a..bf801211fb0263c60a49b16ee309a4f018e92836 100644 (file)
--- a/changelog_unreleased/4647-validatepath-allows-lease-file-path-outside-dhcp-data-dir
+++ b/changelog_unreleased/4647-validatepath-allows-lease-file-path-outside-dhcp-data-dir
@@ -1,6 +1,4 @@
-[bug]          wlodek
-       PathChecker::validatePath() now rejects "." and ".." file names,
-       which previously bypassed parent-directory checks and could resolve
-       paths outside the configured data directory (e.g. memfile lease
-       database name "..").
+[bug]          wlodek, tmark
+       Path validation now rejects now rejects "." and ".." file names
+       as invalid (e.g. memfile lease database name "..").
        (Gitlab #4647)

diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
index 60fdee04503b779e2da4378d16da9142796462e8..b22228a9084046ec16d981a2685a829d0144fd7c 100644 (file)
--- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc
@@ -567,13 +567,25 @@ TEST_F(MemfileLeaseMgrTest, defaultDataDir) {
     ASSERT_THROW_MSG(lease_mgr.reset(new Memfile_LeaseMgr(pmap)),
                      file::SecurityError, os.str());
 
-    // Bare ".." must not resolve outside the data directory.
+    // Blank value should be rejected.
+    pmap["name"] = "   ";
+    ASSERT_THROW_MSG(lease_mgr.reset(new Memfile_LeaseMgr(pmap)),
+                     BadValue, "path: '' has no filename");
+
+    // Empty value should be rejected.
+    pmap["name"] = "";
+    ASSERT_THROW_MSG(lease_mgr.reset(new Memfile_LeaseMgr(pmap)),
+                     BadValue, "path: '' has no filename");
+
+    // Bare "." should be rejected.
+    pmap["name"] = ".";
+    ASSERT_THROW_MSG(lease_mgr.reset(new Memfile_LeaseMgr(pmap)),
+                     BadValue, "path: '.' has no filename");
+
+    // Bare ".." should be rejected.
     pmap["name"] = "..";
-    std::ostringstream os2;
-    os2 << "invalid path specified: '..', supported path is '"
-        << CfgMgr::instance().getDataDir() << "'";
     ASSERT_THROW_MSG(lease_mgr.reset(new Memfile_LeaseMgr(pmap)),
-                     file::SecurityError, os2.str());
+                     BadValue, "path: '..' has no filename");
 }
 
 /// @brief Verifies that the supported path may be overridden with

diff --git a/src/lib/util/filesystem.cc b/src/lib/util/filesystem.cc
index 07cf7a05376de22191888c4235896332d959c708..2b745aca3e88b3eea277e7a2437e4b4fe5db0ce9 100644 (file)
--- a/src/lib/util/filesystem.cc
+++ b/src/lib/util/filesystem.cc
@@ -293,28 +293,10 @@ PathChecker::validatePath(const std::string input_path_str,
                           bool enforce_path /* = PathChecker::shouldEnforceSecurity() */) const {
     Path input_path(trim(input_path_str));
     auto filename = input_path.filename();
-    if (filename.empty()) {
+    if (filename.empty() || (filename == ".") || (filename == "..")) {
         isc_throw(BadValue, "path: '" << input_path.str() << "' has no filename");
     }
 
-    // A bare "." or ".." has an empty parent directory, so the check below would
-    // otherwise accept it and append it to the supported path. For "..", that
-    // resolves outside the supported directory (e.g. /var/lib/kea/..). The same
-    // escape also occurs for "<supported-path>/..", whose parent matches.
-    if ((filename == ".") || (filename == "..")) {
-        std::ostringstream oss;
-        oss << "invalid path specified: '"
-            << filename
-            << "', supported path is '"
-            << path_ << "'";
-
-        if (enforce_path) {
-            isc_throw(SecurityError, oss.str());
-        } else {
-            isc_throw(SecurityWarn, oss.str());
-        }
-    }
-
     auto parent_path = input_path.parentPath();
     auto parent_dir = input_path.parentDirectory();
     if (!parent_dir.empty()) {

diff --git a/src/lib/util/tests/filesystem_unittests.cc b/src/lib/util/tests/filesystem_unittests.cc
index ebe35f039cfc6bfbcddc5e527ddec4cc2a258156..01c283fb3be0e88c3ec7edd34521e24ff7614d27 100644 (file)
--- a/src/lib/util/tests/filesystem_unittests.cc
+++ b/src/lib/util/tests/filesystem_unittests.cc
@@ -398,27 +398,24 @@ TEST_F(PathCheckerTest, validatePathEnforcePath) {
         __LINE__,
         "..",
         "",
-        string("invalid path specified: '..', supported path is '" +
-               def_path + "'"),
-        true
+        "path: '..' has no filename",
+        false
     },
     {
         // Bare "." refers to the supported directory itself.
         __LINE__,
         ".",
         "",
-        string("invalid path specified: '.', supported path is '" +
-               def_path + "'"),
-        true
+        "path: '.' has no filename",
+        false
     },
     {
         // Supported path plus ".." also escapes despite matching parent.
         __LINE__,
         def_path + "/..",
         "",
-        string("invalid path specified: '..', supported path is '" +
-               def_path + "'"),
-        true
+        string("path: '" + def_path + "/..' has no filename"),
+        false
     }
     };