From ac0c76602486efeb5d520f71f066372858bf4da5 Mon Sep 17 00:00:00 2001 From: fvaloppi-nv Date: Wed, 28 Oct 2026 21:09:36 +0000 Subject: [PATCH] Harden C++ temporary path handling Signed-off-by: fvaloppi-nv --- src/core/log_bridge/cpp/CMakeLists.txt | 7 + .../log_bridge/cpp/posix_secure_directory.cpp | 151 ++++++++++++++++++ .../log_bridge/cpp/posix_secure_directory.hpp | 16 ++ src/core/log_bridge/cpp/sink_config.cpp | 25 +-- .../vive_se3_tracker_plugin.cpp | 5 +- tests/cpp/core/log_bridge/CMakeLists.txt | 9 ++ .../test_posix_secure_directory.cpp | 83 ++++++++++ tests/cpp/core/mcap/mcap_test_support.hpp | 20 +++ .../plugin_manager/temporary_directory.hpp | 14 ++ .../test_replay_session.cpp | 18 +++ .../spacemouse/spacemouse_test_dirs.hpp | 21 ++- 11 files changed, 345 insertions(+), 24 deletions(-) create mode 100644 src/core/log_bridge/cpp/posix_secure_directory.cpp create mode 100644 src/core/log_bridge/cpp/posix_secure_directory.hpp create mode 100644 tests/cpp/core/log_bridge/test_posix_secure_directory.cpp diff --git a/src/core/log_bridge/cpp/CMakeLists.txt b/src/core/log_bridge/cpp/CMakeLists.txt index 800edea46a..1815ca08fa 100644 --- a/src/core/log_bridge/cpp/CMakeLists.txt +++ b/src/core/log_bridge/cpp/CMakeLists.txt @@ -15,6 +15,13 @@ add_library(log_bridge_core STATIC inc/log_bridge/logger.hpp ) +if(NOT WIN32) + target_sources(log_bridge_core PRIVATE + posix_secure_directory.cpp + posix_secure_directory.hpp + ) +endif() + target_include_directories(log_bridge_core INTERFACE $ diff --git a/src/core/log_bridge/cpp/posix_secure_directory.cpp b/src/core/log_bridge/cpp/posix_secure_directory.cpp new file mode 100644 index 0000000000..43d76ee1ad --- /dev/null +++ b/src/core/log_bridge/cpp/posix_secure_directory.cpp @@ -0,0 +1,151 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#include "posix_secure_directory.hpp" + +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace isaaccapture::detail +{ +namespace +{ + +class FileDescriptor +{ +public: + explicit FileDescriptor(int fd) : fd_(fd) + { + } + + ~FileDescriptor() + { + if (fd_ >= 0) + { + ::close(fd_); + } + } + + FileDescriptor(const FileDescriptor&) = delete; + FileDescriptor& operator=(const FileDescriptor&) = delete; + + FileDescriptor(FileDescriptor&& other) noexcept : fd_(std::exchange(other.fd_, -1)) + { + } + + FileDescriptor& operator=(FileDescriptor&& other) noexcept + { + if (this != &other) + { + if (fd_ >= 0) + { + ::close(fd_); + } + fd_ = std::exchange(other.fd_, -1); + } + return *this; + } + + int get() const + { + return fd_; + } + +private: + int fd_; +}; + +bool has_secure_permissions(const struct ::stat& info, bool target) +{ + if (!S_ISDIR(info.st_mode) || (target && info.st_uid != ::getuid())) + { + return false; + } + const bool publicly_writable = (info.st_mode & (S_IWGRP | S_IWOTH)) != 0; + return !publicly_writable || (!target && (info.st_mode & S_ISVTX) != 0); +} + +std::vector path_components(const std::filesystem::path& path) +{ + std::vector components; + for (const auto& component : path.relative_path()) + { + if (component != ".") + { + components.push_back(component.string()); + } + } + return components; +} + +bool open_or_create_child(FileDescriptor& parent, const std::string& name, bool target) +{ + struct ::stat entry + { + }; + if (::fstatat(parent.get(), name.c_str(), &entry, AT_SYMLINK_NOFOLLOW) != 0) + { + if (errno != ENOENT || (::mkdirat(parent.get(), name.c_str(), 0700) != 0 && errno != EEXIST)) + { + return false; + } + } + + // Traverse relative to the vetted parent. O_NOFOLLOW rejects a symlink + // even if the entry changed after fstatat. + const int child = ::openat(parent.get(), name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (child < 0) + { + return false; + } + FileDescriptor opened(child); + if (::fstat(opened.get(), &entry) != 0 || !has_secure_permissions(entry, target)) + { + return false; + } + parent = std::move(opened); + return true; +} + +} // namespace + +bool ensure_secure_directory(const std::filesystem::path& directory) +{ + std::error_code absolute_ec; + const auto absolute = std::filesystem::absolute(directory, absolute_ec).lexically_normal(); + if (absolute_ec || !absolute.is_absolute()) + { + return false; + } + + const auto components = path_components(absolute); + if (components.empty()) + { + return false; + } + + FileDescriptor current(::open("/", O_RDONLY | O_DIRECTORY | O_CLOEXEC)); + if (current.get() < 0) + { + return false; + } + + for (std::size_t index = 0; index < components.size(); ++index) + { + const bool target = index + 1 == components.size(); + if (components[index] == ".." || !open_or_create_child(current, components[index], target)) + { + return false; + } + } + return true; +} + +} // namespace isaaccapture::detail diff --git a/src/core/log_bridge/cpp/posix_secure_directory.hpp b/src/core/log_bridge/cpp/posix_secure_directory.hpp new file mode 100644 index 0000000000..652e410c48 --- /dev/null +++ b/src/core/log_bridge/cpp/posix_secure_directory.hpp @@ -0,0 +1,16 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +#include + +namespace isaaccapture::detail +{ + +// std::filesystem does not expose POSIX ownership, sticky bits, or no-follow +// traversal. Keep this policy in one testable helper instead of open-coding it +// at each sink setup site. +bool ensure_secure_directory(const std::filesystem::path& directory); + +} // namespace isaaccapture::detail diff --git a/src/core/log_bridge/cpp/sink_config.cpp b/src/core/log_bridge/cpp/sink_config.cpp index 310852e1fb..22b9ceb387 100644 --- a/src/core/log_bridge/cpp/sink_config.cpp +++ b/src/core/log_bridge/cpp/sink_config.cpp @@ -8,6 +8,9 @@ #include "sink_config.hpp" #include "inc/log_bridge/logger.hpp" +#ifndef _WIN32 +# include "posix_secure_directory.hpp" +#endif #include "socket_sink.hpp" #include @@ -33,7 +36,6 @@ # include # include -# include #else # include #endif @@ -358,28 +360,13 @@ const std::vector& local_sinks() auto dir = log_dir(); #ifndef _WIN32 - // Track newly created components for owner-only permissions. - std::vector missing; + if (!ensure_secure_directory(dir)) { - std::error_code exists_ec; - std::filesystem::path probe = dir; - while (!probe.empty() && probe != probe.parent_path() && !std::filesystem::exists(probe, exists_ec)) - { - missing.push_back(probe); - probe = probe.parent_path(); - } + return std::vector{ console }; } -#endif +#else std::error_code dir_ec; std::filesystem::create_directories(dir, dir_ec); -#ifndef _WIN32 - // Preserve permissions on existing operator-provided directories. - for (const auto& component : missing) - { - std::error_code perms_ec; - std::filesystem::permissions( - component, std::filesystem::perms::owner_all, std::filesystem::perm_options::replace, perms_ec); - } #endif // Give each C++ writer a timestamped, PID-qualified path. diff --git a/src/plugins/vive_se3_tracker/vive_se3_tracker_plugin.cpp b/src/plugins/vive_se3_tracker/vive_se3_tracker_plugin.cpp index 9e50463ed7..36c586cb3c 100644 --- a/src/plugins/vive_se3_tracker/vive_se3_tracker_plugin.cpp +++ b/src/plugins/vive_se3_tracker/vive_se3_tracker_plugin.cpp @@ -61,8 +61,7 @@ bool is_secure_dir(const std::string& dir) // 1. VIVE_SE3_COLLECTIONS_FILE, if set, wins verbatim (caller's responsibility). // 2. else $XDG_RUNTIME_DIR (systemd gives a per-user, mode-0700 dir). // 3. else a private /tmp/vive_se3_tracker- we create 0700 and verify we own. -// Falls through to the plain /tmp file only if the private dir can't be secured, -// and warns — never silently write to a world-writable fixed path. +// Disables the advertisement if the private directory cannot be secured. // Keep in sync with record_se3_vive.py. std::string resolve_collections_file() { @@ -83,7 +82,7 @@ std::string resolve_collections_file() ->warn( "could not secure a per-user directory for the collections file; set VIVE_SE3_COLLECTIONS_FILE to a " "private path."); - return dir + "/" + kCollectionsFileName; // best effort; open still O_TRUNC-guarded below + return {}; } core::SchemaPusherConfig make_pusher_config(const std::string& collection_id) diff --git a/tests/cpp/core/log_bridge/CMakeLists.txt b/tests/cpp/core/log_bridge/CMakeLists.txt index b9b0c4c275..3c2b1eaf12 100644 --- a/tests/cpp/core/log_bridge/CMakeLists.txt +++ b/tests/cpp/core/log_bridge/CMakeLists.txt @@ -7,6 +7,15 @@ add_executable(log_bridge_tests test_logger.cpp ) +if(NOT WIN32) + target_sources(log_bridge_tests PRIVATE + test_posix_secure_directory.cpp + ) + target_include_directories(log_bridge_tests PRIVATE + ${PROJECT_SOURCE_DIR}/src/core/log_bridge/cpp + ) +endif() + target_link_libraries(log_bridge_tests PRIVATE log_bridge::log_bridge_core Catch2::Catch2WithMain diff --git a/tests/cpp/core/log_bridge/test_posix_secure_directory.cpp b/tests/cpp/core/log_bridge/test_posix_secure_directory.cpp new file mode 100644 index 0000000000..a3e05b8399 --- /dev/null +++ b/tests/cpp/core/log_bridge/test_posix_secure_directory.cpp @@ -0,0 +1,83 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#include "posix_secure_directory.hpp" + +#include +#include + +#include +#include +#include +#include +#include + +namespace +{ + +class TemporaryDirectory +{ +public: + TemporaryDirectory() + { + std::array pattern{}; + constexpr char kPattern[] = "/tmp/isaaccapture-log-test-XXXXXX"; + std::copy(std::begin(kPattern), std::end(kPattern), pattern.begin()); + char* created = ::mkdtemp(pattern.data()); + REQUIRE(created != nullptr); + path_ = created; + } + + ~TemporaryDirectory() + { + std::error_code ec; + std::filesystem::remove_all(path_, ec); + } + + const std::filesystem::path& path() const + { + return path_; + } + +private: + std::filesystem::path path_; +}; + +} // namespace + +TEST_CASE("secure directory creation uses owner-only permissions", "[unit]") +{ + TemporaryDirectory temporary; + const auto directory = temporary.path() / "nested" / "logs"; + + REQUIRE(isaaccapture::detail::ensure_secure_directory(directory)); + + struct ::stat info + { + }; + REQUIRE(::lstat(directory.c_str(), &info) == 0); + REQUIRE((info.st_mode & 0777) == 0700); + REQUIRE(info.st_uid == ::getuid()); +} + +TEST_CASE("secure directory creation rejects symlinked parents", "[unit]") +{ + TemporaryDirectory temporary; + const auto destination = temporary.path() / "destination"; + const auto link = temporary.path() / "runtime"; + REQUIRE(std::filesystem::create_directory(destination)); + REQUIRE(::symlink(destination.c_str(), link.c_str()) == 0); + + REQUIRE_FALSE(isaaccapture::detail::ensure_secure_directory(link / "logs")); + REQUIRE_FALSE(std::filesystem::exists(destination / "logs")); +} + +TEST_CASE("secure directory creation rejects writable parents", "[unit]") +{ + TemporaryDirectory temporary; + const auto shared = temporary.path() / "shared"; + REQUIRE(std::filesystem::create_directory(shared)); + REQUIRE(::chmod(shared.c_str(), 0777) == 0); + + REQUIRE_FALSE(isaaccapture::detail::ensure_secure_directory(shared / "logs")); +} diff --git a/tests/cpp/core/mcap/mcap_test_support.hpp b/tests/cpp/core/mcap/mcap_test_support.hpp index 8d4a0ff257..de24ea2389 100644 --- a/tests/cpp/core/mcap/mcap_test_support.hpp +++ b/tests/cpp/core/mcap/mcap_test_support.hpp @@ -13,11 +13,14 @@ #include #include +#include #include #include +#include #include #include #include +#include #ifdef _WIN32 # include @@ -43,10 +46,23 @@ inline int current_pid() //! A temp .mcap path unique to this process and call. `prefix` names the suite asking. inline std::string temp_mcap_path(std::string_view prefix) { +#ifndef _WIN32 + auto pattern = (fs::temp_directory_path() / (std::string(prefix) + "_XXXXXX")).string(); + std::vector buffer(pattern.begin(), pattern.end()); + buffer.push_back('\0'); + if (::mkdtemp(buffer.data()) == nullptr) + { + throw std::runtime_error("Failed to create private MCAP test directory"); + } + // Keep the recording below an atomically created owner-only directory; + // McapWriter may safely create and reopen its predictable child name. + return (fs::path(buffer.data()) / "recording.mcap").string(); +#else static std::atomic counter{ 0 }; const auto name = std::string(prefix) + "_" + std::to_string(current_pid()) + "_" + std::to_string(counter++) + ".mcap"; return (fs::temp_directory_path() / name).string(); +#endif } //! Removes the file when it goes out of scope, whether or not the case that wrote it passed. @@ -59,7 +75,11 @@ class TempFileCleanup ~TempFileCleanup() noexcept { std::error_code ec; +#ifndef _WIN32 + fs::remove_all(fs::path(path_).parent_path(), ec); +#else fs::remove(path_, ec); +#endif } TempFileCleanup(const TempFileCleanup&) = delete; TempFileCleanup& operator=(const TempFileCleanup&) = delete; diff --git a/tests/cpp/core/plugin_manager/temporary_directory.hpp b/tests/cpp/core/plugin_manager/temporary_directory.hpp index 5d9bf4f0f4..08bfd40039 100644 --- a/tests/cpp/core/plugin_manager/temporary_directory.hpp +++ b/tests/cpp/core/plugin_manager/temporary_directory.hpp @@ -4,16 +4,29 @@ #pragma once #include +#include #include #include #include #include +#include class TemporaryDirectory { public: TemporaryDirectory() { +#ifndef _WIN32 + auto pattern = (std::filesystem::temp_directory_path() / "isaacteleop_plugin_manager_XXXXXX").string(); + std::vector buffer(pattern.begin(), pattern.end()); + buffer.push_back('\0'); + if (::mkdtemp(buffer.data()) == nullptr) + { + throw std::runtime_error("Failed to create temporary test directory"); + } + // mkdtemp creates the directory atomically with owner-only access. + m_path = buffer.data(); +#else const auto seed = std::chrono::steady_clock::now().time_since_epoch().count(); for (int suffix = 0; suffix < 100; ++suffix) { @@ -25,6 +38,7 @@ class TemporaryDirectory } } throw std::runtime_error("Failed to create temporary test directory"); +#endif } ~TemporaryDirectory() diff --git a/tests/cpp/core/replay_deviceio_session/test_replay_session.cpp b/tests/cpp/core/replay_deviceio_session/test_replay_session.cpp index 4759840d0e..6a18820ab0 100644 --- a/tests/cpp/core/replay_deviceio_session/test_replay_session.cpp +++ b/tests/cpp/core/replay_deviceio_session/test_replay_session.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -55,9 +56,22 @@ namespace std::string get_temp_mcap_path() { +#ifndef _WIN32 + auto pattern = (fs::temp_directory_path() / "test_replay_XXXXXX").string(); + std::vector buffer(pattern.begin(), pattern.end()); + buffer.push_back('\0'); + if (::mkdtemp(buffer.data()) == nullptr) + { + throw std::runtime_error("Failed to create private replay test directory"); + } + // Keep the recording below an atomically created owner-only directory; + // McapWriter may safely create and reopen its predictable child name. + return (fs::path(buffer.data()) / "recording.mcap").string(); +#else static std::atomic cnt{ 0 }; auto fn = "test_replay_" + std::to_string(GET_PID()) + "_" + std::to_string(cnt++) + ".mcap"; return (fs::temp_directory_path() / fn).string(); +#endif } struct TempFileCleanup @@ -69,7 +83,11 @@ struct TempFileCleanup ~TempFileCleanup() noexcept { std::error_code ec; +#ifndef _WIN32 + fs::remove_all(fs::path(path).parent_path(), ec); +#else fs::remove(path, ec); +#endif } TempFileCleanup(const TempFileCleanup&) = delete; TempFileCleanup& operator=(const TempFileCleanup&) = delete; diff --git a/tests/cpp/plugins/spacemouse/spacemouse_test_dirs.hpp b/tests/cpp/plugins/spacemouse/spacemouse_test_dirs.hpp index 5cfb48e4cd..8f6f1aef90 100644 --- a/tests/cpp/plugins/spacemouse/spacemouse_test_dirs.hpp +++ b/tests/cpp/plugins/spacemouse/spacemouse_test_dirs.hpp @@ -7,11 +7,14 @@ #include +#include +#include #include #include -#include +#include #include #include +#include namespace spacemouse_test { @@ -20,8 +23,22 @@ class FakeDevices { public: FakeDevices() - : root_(std::filesystem::temp_directory_path() / ("spacemouse_test_" + std::to_string(std::random_device{}()))) { +#ifndef _WIN32 + auto pattern = (std::filesystem::temp_directory_path() / "spacemouse_test_XXXXXX").string(); + std::vector buffer(pattern.begin(), pattern.end()); + buffer.push_back('\0'); + if (::mkdtemp(buffer.data()) == nullptr) + { + throw std::runtime_error("Failed to create temporary SpaceMouse test directory"); + } + // mkdtemp prevents another user from pre-creating the test root. + root_ = buffer.data(); +#else + root_ = std::filesystem::temp_directory_path() / + ("spacemouse_test_" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count())); + std::filesystem::create_directory(root_); +#endif std::filesystem::create_directories(class_dir()); std::filesystem::create_directories(dev_dir()); }