diff --git a/src/capi_frontend/capi.cpp b/src/capi_frontend/capi.cpp index 2eb9b25920..77bd7dc732 100644 --- a/src/capi_frontend/capi.cpp +++ b/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(new Status(StatusCode::NONEXISTENT_PTR, "log path")); } ovms::ServerSettingsImpl* serverSettings = reinterpret_cast(settings); - serverSettings->allowedLocalMediaPath = allowed_local_media_path; + serverSettings->allowedLocalMediaPath = ovms::FileSystem::normalizeConfiguredPath(allowed_local_media_path); return nullptr; } diff --git a/src/cli_parser.cpp b/src/cli_parser.cpp index 9437fd9755..2bcd5d2eb8 100644 --- a/src/cli_parser.cpp +++ b/src/cli_parser.cpp @@ -530,7 +530,7 @@ void CLIParser::prepareServer(ServerSettingsImpl& serverSettings) { serverSettings.allowedMediaDomains = result->operator[]("allowed_media_domains").as>(); } if (result->count("allowed_local_media_path")) { - serverSettings.allowedLocalMediaPath = result->operator[]("allowed_local_media_path").as(); + serverSettings.allowedLocalMediaPath = FileSystem::normalizeConfiguredPath(result->operator[]("allowed_local_media_path").as()); } if (result->count("grpc_bind_address")) diff --git a/src/filesystem/filesystem.cpp b/src/filesystem/filesystem.cpp index edafa435e9..0c4f7832d3 100644 --- a/src/filesystem/filesystem.cpp +++ b/src/filesystem/filesystem.cpp @@ -15,6 +15,7 @@ //***************************************************************************** #include "filesystem.hpp" +#include #include #ifdef _WIN32 #include @@ -200,4 +201,20 @@ 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; + std::replace(normalized.begin(), normalized.end(), '\\', '/'); + 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 diff --git a/src/filesystem/filesystem.hpp b/src/filesystem/filesystem.hpp index fb658e8f0e..b5b1aca12c 100644 --- a/src/filesystem/filesystem.hpp +++ b/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 segments) { std::string joined; diff --git a/src/llm/apis/openai_api_handler.cpp b/src/llm/apis/openai_api_handler.cpp index dc7b9031d8..0254b42d3b 100644 --- a/src/llm/apis/openai_api_handler.cpp +++ b/src/llm/apis/openai_api_handler.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -46,6 +47,48 @@ namespace ovms { constexpr size_t DEFAULT_MAX_STOP_WORDS = 16; // same as deep-seek +namespace { + +std::string normalizePathSeparators(const std::string& path) { + std::string normalizedPath = path; + std::replace(normalizedPath.begin(), normalizedPath.end(), '\\', '/'); + return normalizedPath; +} + +std::string normalizeAllowedLocalMediaPath(const std::string& allowedLocalMediaPath) { + std::string normalizedAllowedLocalMediaPath = normalizePathSeparators(allowedLocalMediaPath); + if (normalizedAllowedLocalMediaPath.empty()) { + return normalizedAllowedLocalMediaPath; + } + if (normalizedAllowedLocalMediaPath.back() == '/') { + return normalizedAllowedLocalMediaPath; + } + return normalizedAllowedLocalMediaPath + '/'; +} + +std::filesystem::path normalizeAndResolvePath(const std::string& pathString) { + std::filesystem::path path = normalizePathSeparators(pathString); + 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(); + } + return path; +} + +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,8 +277,14 @@ absl::StatusOr 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 auto normalizedAllowedLocalMediaPath = normalizeAllowedLocalMediaPath(allowedLocalMediaPath.value()); + const auto imagePath = std::filesystem::path(normalizePathSeparators(imageSource)); + if (imagePath.is_relative()) { + return absl::InvalidArgumentError("Relative paths are not allowed for local filesystem access"); + } + const auto resolvedAllowedPath = normalizeAndResolvePath(normalizedAllowedLocalMediaPath); + const auto resolvedImagePath = normalizeAndResolvePath(imageSource); + if (!isPathInsideDirectory(resolvedImagePath, resolvedAllowedPath)) { return absl::InvalidArgumentError("Given filepath is not subpath of allowed_local_media_path"); } try { diff --git a/src/test/c_api_tests.cpp b/src/test/c_api_tests.cpp index 637a1edb5d..712fdcea62 100644 --- a/src/test/c_api_tests.cpp +++ b/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(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; diff --git a/src/test/http_openai_handler_test.cpp b/src/test/http_openai_handler_test.cpp index c3a40cba3c..63e0c376bd 100644 --- a/src/test/http_openai_handler_test.cpp +++ b/src/test/http_openai_handler_test.cpp @@ -14,6 +14,7 @@ // limitations under the License. //***************************************************************************** #include +#include #include #include #include @@ -2146,6 +2147,44 @@ TEST_F(HttpOpenAIHandlerParsingTest, ParsingMessagesImageLocalFilesystemWithinAl EXPECT_EQ(json, std::string("{\"model\":\"llama\",\"messages\":[{\"role\":\"user\",\"content\":\"What is in this image?\"}]}")); } +TEST_F(HttpOpenAIHandlerParsingTest, ParsingMessagesImageLocalFilesystemWithinAllowedPathMixedSeparators) { + std::string json = R"({ +"model": "llama", +"messages": [ + { + "role": "user", + "content": [ + { + "type": "text", + "text": "What is in this image?" + }, + { + "type": "image_url", + "image_url": { + "url": ")" + getGenericFullPathForSrcTest("/ovms/src/test/binaryutils/rgb.jpg") + + R"(" + } + } + ] + } +] +})"; + doc.Parse(json.c_str()); + ASSERT_FALSE(doc.HasParseError()); + + std::string mixedSeparatorAllowedPath = getGenericFullPathForSrcTest("/ovms/src/test/binaryutils"); + std::replace(mixedSeparatorAllowedPath.begin(), mixedSeparatorAllowedPath.end(), '/', '\\'); + + std::shared_ptr apiHandler = std::make_shared(doc, ovms::Endpoint::CHAT_COMPLETIONS, std::chrono::system_clock::now(), *tokenizer); + ASSERT_EQ(apiHandler->parseMessages(mixedSeparatorAllowedPath), absl::OkStatus()); + const ovms::ImageHistory& imageHistory = apiHandler->getImageHistory(); + ASSERT_EQ(imageHistory.size(), 1); + auto [index, image] = imageHistory[0]; + EXPECT_EQ(index, 0); + EXPECT_EQ(image.get_element_type(), ov::element::u8); + EXPECT_EQ(image.get_size(), 3); +} + TEST_F(HttpOpenAIHandlerParsingTest, ParsingMessagesImageLocalFilesystemNotWithinAllowedPath) { std::string json = R"({ "model": "llama", @@ -2173,6 +2212,36 @@ TEST_F(HttpOpenAIHandlerParsingTest, ParsingMessagesImageLocalFilesystemNotWithi ASSERT_EQ(apiHandler->parseMessages("src/test"), absl::InvalidArgumentError("Given filepath is not subpath of allowed_local_media_path")); } +TEST_F(HttpOpenAIHandlerParsingTest, ParsingMessagesImageLocalFilesystemPrefixPathBypassPrevented) { + const std::string allowedLocalMediaPath = getGenericFullPathForSrcTest("/ovms/src/test/binaryutils"); + const std::string siblingPrefixPath = allowedLocalMediaPath + "_private/rgb.jpg"; + std::string json = R"({ +"model": "llama", +"messages": [ + { + "role": "user", + "content": [ + { + "type": "text", + "text": "What is in this image?" + }, + { + "type": "image_url", + "image_url": { + "url": ")" + siblingPrefixPath + + R"(" + } + } + ] + } +] +})"; + doc.Parse(json.c_str()); + ASSERT_FALSE(doc.HasParseError()); + std::shared_ptr apiHandler = std::make_shared(doc, ovms::Endpoint::CHAT_COMPLETIONS, std::chrono::system_clock::now(), *tokenizer); + ASSERT_EQ(apiHandler->parseMessages(allowedLocalMediaPath), absl::InvalidArgumentError("Given filepath is not subpath of allowed_local_media_path")); +} + TEST_F(HttpOpenAIHandlerParsingTest, ParsingMessagesImageLocalFilesystemInvalidPath) { std::string json = R"({ "model": "llama", diff --git a/src/test/ovmsconfig_test.cpp b/src/test/ovmsconfig_test.cpp index 6b76d0e2fa..479543a6e8 100644 --- a/src/test/ovmsconfig_test.cpp +++ b/src/test/ovmsconfig_test.cpp @@ -2364,7 +2364,7 @@ TEST(OvmsConfigTest, positiveMulti) { #endif EXPECT_EQ(config.cacheDir(), "/tmp/model_cache"); ASSERT_TRUE(config.getServerSettings().allowedLocalMediaPath.has_value()); - EXPECT_EQ(config.getServerSettings().allowedLocalMediaPath.value(), "/tmp/path"); + EXPECT_EQ(config.getServerSettings().allowedLocalMediaPath.value(), ovms::FileSystem::normalizeConfiguredPath("/tmp/path")); ASSERT_TRUE(config.getServerSettings().allowedMediaDomains.has_value()); EXPECT_EQ(config.getServerSettings().allowedMediaDomains.value().size(), 3); EXPECT_EQ(config.getServerSettings().allowedMediaDomains.value()[0], "raw.githubusercontent.com"); @@ -2385,6 +2385,25 @@ TEST(OvmsConfigTest, positiveMulti) { #endif } +TEST(OvmsConfigTest, allowedLocalMediaPathRelativeIsNormalized) { + char* n_argv[] = { + "ovms", + "--rest_port", "45", + "--allowed_local_media_path", + "src/test", + "--config_path", + "/config.json"}; + + int arg_count = 7; + ConstructorEnabledConfig config; + config.parse(arg_count, n_argv); + + ASSERT_TRUE(config.getServerSettings().allowedLocalMediaPath.has_value()); + const auto configuredPath = std::filesystem::path(config.getServerSettings().allowedLocalMediaPath.value()); + const auto expectedPath = std::filesystem::path(ovms::FileSystem::normalizeConfiguredPath("src/test")); + EXPECT_EQ(configuredPath.lexically_normal(), expectedPath.lexically_normal()); +} + TEST(OvmsConfigTest, positiveSingle) { #ifdef _WIN32 const std::string cpu_extension_lib_path = "tmp_cpu_extension_library_dir";