From 6dd8401c5957cb57bc41bdf418b36d92c581d01a Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Mon, 28 Sep 2026 05:59:41 +0800 Subject: [PATCH] Confine Updater Archive Extraction to the Target Directory The preset updater extracted downloaded archives by appending each entry name to the cache directory, and the network plugin installer did the same for the plugin folder, without checking that the result stays inside it. Move the updater's extraction into libslic3r as extract_archive_confined, which validates every entry with is_path_within_root before writing anything and fails the whole archive if one entry resolves outside the target. The plugin installer now rejects such an entry the same way. Well formed archives extract exactly as before. --- src/libslic3r/miniz_extension.cpp | 59 ++++++++++++++++++++ src/libslic3r/miniz_extension.hpp | 2 + src/slic3r/GUI/GUI_App.cpp | 6 +++ src/slic3r/Utils/PresetUpdater.cpp | 58 +------------------- tests/libslic3r/CMakeLists.txt | 1 + tests/libslic3r/test_miniz_extension.cpp | 69 ++++++++++++++++++++++++ 6 files changed, 139 insertions(+), 56 deletions(-) create mode 100644 tests/libslic3r/test_miniz_extension.cpp diff --git a/src/libslic3r/miniz_extension.cpp b/src/libslic3r/miniz_extension.cpp index 53a4a8e9de..620cb6fb27 100644 --- a/src/libslic3r/miniz_extension.cpp +++ b/src/libslic3r/miniz_extension.cpp @@ -4,6 +4,9 @@ #include "miniz_extension.hpp" #include "Utils.hpp" +#include +#include + #if defined(_MSC_VER) || defined(__MINGW64__) #include "boost/nowide/cstdio.hpp" #endif @@ -115,6 +118,62 @@ std::string decode_archive_entry_path(mz_zip_archive *zip, const mz_zip_archive_ return decode_zip_unicode_path_extra_field(extra.substr(0, extra_size > 0 ? extra_size - 1 : 0), stat.m_filename); } +bool extract_archive_confined(const std::string &zip_path_utf8, const std::string &dest_dir) +{ + mz_zip_archive archive; + mz_zip_zero_struct(&archive); + + if (!open_zip_reader(&archive, zip_path_utf8)) { + BOOST_LOG_TRIVIAL(error) << "Unable to open zip reader for " << zip_path_utf8; + return false; + } + + const mz_uint num_entries = mz_zip_reader_get_num_files(&archive); + mz_zip_archive_file_stat stat; + + // Validate every entry first so an archive with a single escaping entry leaves no partial output behind. + const boost::filesystem::path root(dest_dir); + for (mz_uint i = 0; i < num_entries; ++i) { + if (mz_zip_reader_file_stat(&archive, i, &stat) && !is_path_within_root(stat.m_filename, root)) { + BOOST_LOG_TRIVIAL(error) << "Unzip: rejecting " << zip_path_utf8 << ", entry " << stat.m_filename << " resolves outside " << dest_dir; + close_zip_reader(&archive); + return false; + } + } + + for (mz_uint i = 0; i < num_entries; ++i) { + if (!mz_zip_reader_file_stat(&archive, i, &stat)) { + BOOST_LOG_TRIVIAL(warning) << "Unzip: read file stat failed"; + continue; + } + const std::string dest_file = dest_dir + "/" + stat.m_filename; + try { + if (stat.m_is_directory) { + const boost::filesystem::path dest_path(dest_file); + if (!boost::filesystem::exists(dest_path)) + boost::filesystem::create_directories(dest_path); + continue; + } + if (stat.m_uncomp_size == 0) { + BOOST_LOG_TRIVIAL(warning) << "Unzip: invalid size for file " << stat.m_filename; + continue; + } + if (!mz_zip_reader_extract_to_file(&archive, stat.m_file_index, dest_file.c_str(), 0)) { + BOOST_LOG_TRIVIAL(error) << "Unzip: extract file " << stat.m_filename << " to dest " << dest_file << " failed"; + close_zip_reader(&archive); + return false; + } + BOOST_LOG_TRIVIAL(info) << "Unzip: successfully extract file " << stat.m_file_index << " to " << dest_file; + } catch (const std::exception &e) { + close_zip_reader(&archive); + BOOST_LOG_TRIVIAL(error) << "Unzip: archive read exception: " << e.what(); + return false; + } + } + close_zip_reader(&archive); + return true; +} + MZ_Archive::MZ_Archive() { mz_zip_zero_struct(&arch); diff --git a/src/libslic3r/miniz_extension.hpp b/src/libslic3r/miniz_extension.hpp index 1a1c96689f..97aa91b93f 100644 --- a/src/libslic3r/miniz_extension.hpp +++ b/src/libslic3r/miniz_extension.hpp @@ -11,6 +11,8 @@ bool open_zip_writer(mz_zip_archive *zip, const std::string &fname_utf8); bool close_zip_reader(mz_zip_archive *zip); bool close_zip_writer(mz_zip_archive *zip); std::string decode_archive_entry_path(mz_zip_archive *zip, const mz_zip_archive_file_stat &stat); +// Extracts every entry of the archive under dest_dir. Nothing is written if any entry would resolve outside dest_dir. +bool extract_archive_confined(const std::string &zip_path_utf8, const std::string &dest_dir); class MZ_Archive { public: diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index a41001f9e0..99f0d8088a 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -1512,6 +1512,12 @@ int GUI_App::install_plugin(std::string name, std::string package_name, InstallP size_t n = mz_zip_reader_get_extra(&archive, stat.m_file_index, extra.data(), extra.size()); dest_file = decode(extra.substr(0, n), stat.m_filename); } + if (!is_path_within_root(dest_file, plugin_folder)) { + BOOST_LOG_TRIVIAL(error) << "[install_plugin] entry " << dest_file << " resolves outside " << plugin_folder.string(); + close_zip_reader(&archive); + if (pro_fn) { pro_fn(InstallStatusUnzipFailed, 0, cancel); } + return InstallStatusUnzipFailed; + } auto dest_path = plugin_folder / dest_file; boost::filesystem::create_directories(dest_path.parent_path()); std::string dest_zip_file = encode_path(dest_path.string().c_str()); diff --git a/src/slic3r/Utils/PresetUpdater.cpp b/src/slic3r/Utils/PresetUpdater.cpp index b8379697d6..5bbe779970 100644 --- a/src/slic3r/Utils/PresetUpdater.cpp +++ b/src/slic3r/Utils/PresetUpdater.cpp @@ -339,62 +339,8 @@ bool PresetUpdater::priv::get_file(const std::string &url, const fs::path &targe //BBS: refine preset update logic bool PresetUpdater::priv::extract_file(const fs::path &source_path, const fs::path &dest_path) { - bool res = true; - std::string file_path = source_path.string(); - std::string parent_path = (!dest_path.empty() ? dest_path : source_path.parent_path()).string(); - mz_zip_archive archive; - mz_zip_zero_struct(&archive); - - if (!open_zip_reader(&archive, file_path)) - { - BOOST_LOG_TRIVIAL(error) << "Unable to open zip reader for "< + +#include "libslic3r/miniz_extension.hpp" + +#include "test_utils.hpp" + +#include + +#include +#include +#include +#include +#include + +using namespace Slic3r; +namespace fs = boost::filesystem; + +namespace { + +void write_zip(const fs::path &zip_file, const std::vector> &entries) +{ + mz_zip_archive zip; + mz_zip_zero_struct(&zip); + REQUIRE(open_zip_writer(&zip, zip_file.string())); + for (const auto &[name, content] : entries) + REQUIRE(mz_zip_writer_add_mem(&zip, name.c_str(), content.data(), content.size(), MZ_DEFAULT_COMPRESSION)); + REQUIRE(mz_zip_writer_finalize_archive(&zip)); + REQUIRE(close_zip_writer(&zip)); +} + +std::string read_file(const fs::path &file) +{ + std::ifstream in(file.string(), std::ios::binary); + return std::string(std::istreambuf_iterator(in), std::istreambuf_iterator()); +} + +} // namespace + +TEST_CASE("Confined extraction writes a well-formed archive under the target directory", "[MinizExtension]") +{ + ScopedTemporaryDir tmp; + const fs::path zip_file = tmp.path() / "bundle.zip"; + const fs::path target = tmp.path() / "cache"; + fs::create_directories(target); + write_zip(zip_file, {{"vendor/", ""}, {"vendor/machine/", ""}, {"vendor.json", "{\"a\":1}"}, {"vendor/machine/printer.json", "{\"b\":2}"}}); + + REQUIRE(extract_archive_confined(zip_file.string(), target.string())); + CHECK(fs::is_directory(target / "vendor")); + CHECK(read_file(target / "vendor.json") == "{\"a\":1}"); + CHECK(read_file(target / "vendor" / "machine" / "printer.json") == "{\"b\":2}"); +} + +TEST_CASE("Confined extraction rejects an archive with an entry outside the target directory", "[MinizExtension]") +{ + ScopedTemporaryDir tmp; + const fs::path zip_file = tmp.path() / "bundle.zip"; + const fs::path target = tmp.path() / "cache"; + fs::create_directories(target); + + const std::string escaping_entry = GENERATE(std::string("../escape.txt"), std::string("..\\escape.txt"), + std::string("sub/../../escape.txt"), std::string("C:/escape.txt")); + // The normal entry comes first so a per-entry check would already have written it. + write_zip(zip_file, {{"normal.json", "{}"}, {escaping_entry, "escaped"}}); + + CAPTURE(escaping_entry); + CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string())); + CHECK_FALSE(fs::exists(tmp.path() / "escape.txt")); + CHECK(fs::is_empty(target)); +}