Add security check for directory symlinks
Currently only files symlink are verified during file operations.
This change is extending check to all directories in path.
Testing done:
- UTs are passing
Signed-off-by: Szymon Dompke <szymon.dompke@intel.com>
Change-Id: I1f30de94872d2a25597d3549224cd90aa8fab634
diff --git a/src/persistent_json_storage.cpp b/src/persistent_json_storage.cpp
index 0dc66ad..97326fe 100644
--- a/src/persistent_json_storage.cpp
+++ b/src/persistent_json_storage.cpp
@@ -2,11 +2,30 @@
#include <phosphor-logging/log.hpp>
+#include <format>
#include <fstream>
#include <stdexcept>
using namespace std::literals::string_literals;
+bool isAnySymlink(const std::filesystem::path& path)
+{
+ auto currentPath = path;
+ while (currentPath != path.root_path())
+ {
+ if (std::filesystem::is_symlink(currentPath))
+ {
+ std::string warningStr = std::format("{} is a symlink",
+ currentPath.string());
+ phosphor::logging::log<phosphor::logging::level::WARNING>(
+ warningStr.c_str());
+ return true;
+ }
+ currentPath = currentPath.parent_path();
+ }
+ return false;
+}
+
PersistentJsonStorage::PersistentJsonStorage(const DirectoryPath& directory) :
directory(directory)
{}
@@ -53,7 +72,7 @@
{
const auto path = join(directory, filePath);
- if (std::filesystem::is_symlink(path))
+ if (isAnySymlink(path))
{
return false;
}
@@ -114,7 +133,7 @@
for (const auto& p :
std::filesystem::recursive_directory_iterator(directory))
{
- if (p.is_regular_file() && !p.is_symlink())
+ if (p.is_regular_file() && !isAnySymlink(p.path()))
{
auto item = std::filesystem::relative(p.path(), directory);
result.emplace_back(std::move(item));
@@ -149,7 +168,7 @@
void PersistentJsonStorage::assertThatPathIsNotSymlink(
const std::filesystem::path& path)
{
- if (std::filesystem::is_symlink(path))
+ if (isAnySymlink(path))
{
throw std::runtime_error(
"Source/Target file is a symlink! Operation canceled on path "s +
diff --git a/tests/src/test_persistent_json_storage.cpp b/tests/src/test_persistent_json_storage.cpp
index 8e67a2e..cec0019 100644
--- a/tests/src/test_persistent_json_storage.cpp
+++ b/tests/src/test_persistent_json_storage.cpp
@@ -114,6 +114,38 @@
ASSERT_THAT(sut.load(fileName), Eq(std::nullopt));
}
+struct TestFileSymlink
+{
+ static interfaces::JsonStorage::FilePath
+ setupSymlinks(const std::filesystem::path& originalFile,
+ const interfaces::JsonStorage::DirectoryPath& directory)
+ {
+ auto linkPath = std::filesystem::path(directory) /
+ "report/symlink.json";
+ std::filesystem::create_directories(std::filesystem::path(directory) /
+ "report");
+ std::filesystem::create_symlink(originalFile, linkPath);
+ return interfaces::JsonStorage::FilePath(linkPath);
+ }
+};
+
+struct TestDirectorySymlink
+{
+ static interfaces::JsonStorage::FilePath
+ setupSymlinks(const std::filesystem::path& originalFile,
+ const interfaces::JsonStorage::DirectoryPath& directory)
+ {
+ auto linkPath = std::filesystem::path(directory) / "reportLink";
+ std::filesystem::create_directories(std::filesystem::path(directory) /
+ "report");
+ std::filesystem::create_directory_symlink(originalFile.parent_path(),
+ linkPath);
+ return interfaces::JsonStorage::FilePath(linkPath /
+ originalFile.filename());
+ }
+};
+
+template <typename T>
class TestPersistentJsonStorageWithSymlink : public TestPersistentJsonStorage
{
public:
@@ -123,11 +155,7 @@
file << "{}";
file.close();
- std::filesystem::create_directories(std::filesystem::path(directory) /
- "report");
- std::filesystem::create_symlink(dummyReportPath,
- std::filesystem::path(directory) /
- "report/symlink.json");
+ linkPath = T::setupSymlinks(dummyReportPath, directory);
}
static void SetUpTestSuite()
@@ -146,36 +174,43 @@
}
static const std::filesystem::path dummyReportPath;
+ interfaces::JsonStorage::FilePath linkPath;
};
+template <typename T>
const std::filesystem::path
- TestPersistentJsonStorageWithSymlink::dummyReportPath =
- std::filesystem::temp_directory_path() / "report";
+ TestPersistentJsonStorageWithSymlink<T>::dummyReportPath =
+ std::filesystem::temp_directory_path() / "report.json";
-TEST_F(TestPersistentJsonStorageWithSymlink, symlinksAreNotListed)
+using SymlinkTypes = Types<TestFileSymlink, TestDirectorySymlink>;
+TYPED_TEST_SUITE(TestPersistentJsonStorageWithSymlink, SymlinkTypes);
+
+TYPED_TEST(TestPersistentJsonStorageWithSymlink, symlinksAreNotListed)
{
- ASSERT_THAT(sut.list(), UnorderedElementsAre());
+ ASSERT_THAT(TestPersistentJsonStorage::sut.list(), UnorderedElementsAre());
}
-TEST_F(TestPersistentJsonStorageWithSymlink, throwsWhenStoreTargetIsSymlink)
+TYPED_TEST(TestPersistentJsonStorageWithSymlink, throwsWhenStoreTargetIsSymlink)
{
- ASSERT_THROW(
- sut.store(FilePath("report/symlink.json"), nlohmann::json("data")),
- std::runtime_error);
+ ASSERT_THROW(TestPersistentJsonStorage::sut.store(TestFixture::linkPath,
+ nlohmann::json("data")),
+ std::runtime_error);
- ASSERT_THAT(sut.list(), UnorderedElementsAre());
+ ASSERT_THAT(TestPersistentJsonStorage::sut.list(), UnorderedElementsAre());
}
-TEST_F(TestPersistentJsonStorageWithSymlink, returnsNulloptWhenFileIsSymlink)
+TYPED_TEST(TestPersistentJsonStorageWithSymlink,
+ returnsNulloptWhenFileIsSymlink)
{
- ASSERT_THAT(sut.load(FilePath("report/symlink.json")), Eq(std::nullopt));
+ ASSERT_THAT(TestPersistentJsonStorage::sut.load(TestFixture::linkPath),
+ Eq(std::nullopt));
}
-TEST_F(TestPersistentJsonStorageWithSymlink,
- returnsFalseWhenTryingToDeleteSymlink)
+TYPED_TEST(TestPersistentJsonStorageWithSymlink,
+ returnsFalseWhenTryingToDeleteSymlink)
{
- EXPECT_THAT(sut.remove(FilePath("report/symlink.json")), Eq(false));
- EXPECT_TRUE(std::filesystem::exists(std::filesystem::path(directory) /
- "report/symlink.json"));
- EXPECT_TRUE(std::filesystem::exists(dummyReportPath));
+ EXPECT_THAT(TestPersistentJsonStorage::sut.remove(TestFixture::linkPath),
+ Eq(false));
+ EXPECT_TRUE(std::filesystem::exists(TestFixture::linkPath));
+ EXPECT_TRUE(std::filesystem::exists(TestFixture::dummyReportPath));
}