Fix allowed_local_media_path psirt (#4199)

Author: michalkulakowski

src/capi_frontend/capi.cpp

@@ -62,6 +62,7 @@
 #include "servablemetadata.hpp"
 #include "server_settings.hpp"
 #include "serialization.hpp"
+#include "../filesystem/filesystem.hpp"
 
 using ovms::Buffer;
 using ovms::ExecutionContext;
@@ -585,7 +586,7 @@ DLL_PUBLIC OVMS_Status* OVMS_ServerSettingsSetAllowedLocalMediaPath(OVMS_ServerS
         return reinterpret_cast<OVMS_Status*>(new Status(StatusCode::NONEXISTENT_PTR, "log path"));
     }
     ovms::ServerSettingsImpl* serverSettings = reinterpret_cast<ovms::ServerSettingsImpl*>(settings);
-    serverSettings->allowedLocalMediaPath = allowed_local_media_path;
+    serverSettings->allowedLocalMediaPath = ovms::FileSystem::normalizeConfiguredPath(allowed_local_media_path);
     return nullptr;
 }
 

src/cli_parser.cpp

@@ -532,7 +532,7 @@ void CLIParser::prepareServer(ServerSettingsImpl& serverSettings) {
         serverSettings.allowedMediaDomains = result->operator[]("allowed_media_domains").as<std::vector<std::string>>();
     }
     if (result->count("allowed_local_media_path")) {
-        serverSettings.allowedLocalMediaPath = result->operator[]("allowed_local_media_path").as<std::string>();
+        serverSettings.allowedLocalMediaPath = FileSystem::normalizeConfiguredPath(result->operator[]("allowed_local_media_path").as<std::string>());
     }
 
     if (result->count("grpc_bind_address"))

src/filesystem/filesystem.cpp

@@ -15,6 +15,7 @@
 //*****************************************************************************
 #include "filesystem.hpp"
 
+#include <algorithm>
 #include <memory>
 #ifdef _WIN32
 #include <windows.h>
@@ -200,4 +201,26 @@ Status FileSystem::createFileOverwrite(const std::string& filePath, const std::s
     return StatusCode::OK;
 }
 
+std::string FileSystem::normalizeConfiguredPath(const std::string& pathString) {
+    std::string normalized = pathString;
+#ifdef _WIN32
+    // Backslash is a path separator only on Windows. On POSIX it is a valid
+    // filename character; rewriting it would let a path like
+    // "/allowed\secret.jpg" be authorized as "/allowed/secret.jpg" while
+    // actually opening a sibling file outside the allowlist.
+    std::replace(normalized.begin(), normalized.end(), '\\', '/');
+#endif
+    std::filesystem::path path(normalized);
+    if (path.is_relative()) {
+        path = std::filesystem::current_path() / path;
+    }
+    path = path.lexically_normal();
+    std::error_code ec;
+    auto weakCanonicalPath = std::filesystem::weakly_canonical(path, ec);
+    if (!ec) {
+        return weakCanonicalPath.lexically_normal().string();
+    }
+    return path.string();
+}
+
 }  // namespace ovms

src/filesystem/filesystem.hpp

@@ -231,6 +231,8 @@ class FileSystem {
         return !path.empty() && (path[0] == '/');
     }
 
+    static std::string normalizeConfiguredPath(const std::string& pathString);
+
     static std::string joinPath(std::initializer_list<std::string> segments) {
         std::string joined;
 

src/llm/apis/openai_api_handler.cpp

@@ -18,6 +18,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <filesystem>
 #include <limits>
 #include <memory>
 #include <sstream>
@@ -46,6 +47,17 @@ namespace ovms {
 
 constexpr size_t DEFAULT_MAX_STOP_WORDS = 16;  // same as deep-seek
 
+namespace {
+
+bool isPathInsideDirectory(const std::filesystem::path& testedPath, const std::filesystem::path& allowedDirectory) {
+    const auto mismatch = std::mismatch(
+        allowedDirectory.begin(), allowedDirectory.end(),
+        testedPath.begin(), testedPath.end());
+    return mismatch.first == allowedDirectory.end();
+}
+
+}  // namespace
+
 ov::genai::JsonContainer rapidJsonValueToJsonContainer(const rapidjson::Value& value) {
     if (value.IsNull()) {
         return ov::genai::JsonContainer(nullptr);
@@ -234,15 +246,17 @@ absl::StatusOr<ov::Tensor> loadImage(const std::string& imageSource,
             return absl::InvalidArgumentError(ss.str());
         }
         SPDLOG_LOGGER_TRACE(llm_calculator_logger, "Loading image from local filesystem");
-        const auto firstMissmatch = std::mismatch(imageSource.begin(), imageSource.end(), allowedLocalMediaPath.value().begin(), allowedLocalMediaPath.value().end());
-        if (firstMissmatch.second != allowedLocalMediaPath.value().end()) {
+        const std::filesystem::path resolvedAllowedPath = FileSystem::normalizeConfiguredPath(allowedLocalMediaPath.value());
+        const std::string resolvedImagePathStr = FileSystem::normalizeConfiguredPath(imageSource);
+        const std::filesystem::path resolvedImagePath = resolvedImagePathStr;
+        if (!isPathInsideDirectory(resolvedImagePath, resolvedAllowedPath)) {
             return absl::InvalidArgumentError("Given filepath is not subpath of allowed_local_media_path");
         }
         try {
-            tensor = loadImageStbiFromFile(imageSource.c_str());
+            tensor = loadImageStbiFromFile(resolvedImagePathStr.c_str());
         } catch (std::runtime_error& e) {
             std::stringstream ss;
-            ss << "Image file " << imageSource.c_str() << " parsing failed: " << e.what();
+            ss << "Image file " << resolvedImagePathStr << " parsing failed: " << e.what();
             SPDLOG_LOGGER_DEBUG(llm_calculator_logger, ss.str());
             return absl::InvalidArgumentError(ss.str());
         }

src/test/c_api_tests.cpp

@@ -38,6 +38,7 @@
 #include "../capi_frontend/capi_dag_utils.hpp"
 #include "../capi_frontend/servablemetadata.hpp"
 #include "../dags/pipelinedefinitionstatus.hpp"
+#include "../filesystem/filesystem.hpp"
 #include "src/metrics/metric_module.hpp"
 #include "../ovms.h"
 #include "../servablemanagermodule.hpp"
@@ -183,7 +184,7 @@ TEST(CAPIConfigTest, MultiModelConfiguration) {
     EXPECT_EQ(serverSettings->logLevel, "TRACE");
     EXPECT_EQ(serverSettings->logPath, getGenericFullPathForTmp("/tmp/logs"));
     ASSERT_TRUE(serverSettings->allowedLocalMediaPath.has_value());
-    EXPECT_EQ(serverSettings->allowedLocalMediaPath.value(), getGenericFullPathForTmp("/tmp/path"));
+    EXPECT_EQ(serverSettings->allowedLocalMediaPath.value(), ovms::FileSystem::normalizeConfiguredPath(getGenericFullPathForTmp("/tmp/path")));
     ASSERT_TRUE(serverSettings->allowedMediaDomains.has_value());
     EXPECT_EQ(serverSettings->allowedMediaDomains.value().size(), 3);
     EXPECT_EQ(serverSettings->allowedMediaDomains.value()[0], "raw.githubusercontent.com");
@@ -258,6 +259,22 @@ TEST(CAPIConfigTest, SingleModelConfiguration) {
     GTEST_SKIP() << "Use C-API to initialize in next stages, currently not supported";
 }
 
+TEST(CAPIConfigTest, AllowedLocalMediaPathRelativeIsNormalized) {
+    OVMS_ServerSettings* serverSettingsRaw = nullptr;
+    ASSERT_CAPI_STATUS_NULL(OVMS_ServerSettingsNew(&serverSettingsRaw));
+    ASSERT_NE(serverSettingsRaw, nullptr);
+    ServerSettingsImpl* serverSettings = reinterpret_cast<ServerSettingsImpl*>(serverSettingsRaw);
+
+    ASSERT_CAPI_STATUS_NULL(OVMS_ServerSettingsSetAllowedLocalMediaPath(serverSettingsRaw, "src/test"));
+    ASSERT_TRUE(serverSettings->allowedLocalMediaPath.has_value());
+
+    const auto configuredPath = std::filesystem::path(serverSettings->allowedLocalMediaPath.value());
+    const auto expectedPath = std::filesystem::path(ovms::FileSystem::normalizeConfiguredPath("src/test"));
+    EXPECT_EQ(configuredPath.lexically_normal(), expectedPath.lexically_normal());
+
+    OVMS_ServerSettingsDelete(serverSettingsRaw);
+}
+
 TEST(CAPIStartTest, InitializingMultipleServers) {
     OVMS_Server* srv1 = nullptr;
     OVMS_Server* srv2 = nullptr;