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

@@ -4,6 +4,9 @@
 #include "miniz_extension.hpp"
 #include "Utils.hpp"
 
+#include <boost/filesystem.hpp>
+#include <boost/log/trivial.hpp>
+
 #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);

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:

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());

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 "<<file_path;
-        return false;
-    }
-
-    mz_uint num_entries = mz_zip_reader_get_num_files(&archive);
-
-    mz_zip_archive_file_stat stat;
-    // we first loop the entries to read from the archive the .amf file only, in order to extract the version from it
-    for (mz_uint i = 0; i < num_entries; ++i)
-    {
-        if (mz_zip_reader_file_stat(&archive, i, &stat))
-        {
-            std::string dest_file = parent_path+"/"+stat.m_filename;
-            if (stat.m_is_directory) {
-                fs::path dest_path(dest_file);
-                if (!fs::exists(dest_path))
-                    fs::create_directories(dest_path);
-				continue;
-            }
-            else if (stat.m_uncomp_size == 0) {
-                BOOST_LOG_TRIVIAL(warning) << "[Orca Updater]Unzip: invalid size for file "<<stat.m_filename;
-                continue;
-            }
-            try
-            {
-                res = mz_zip_reader_extract_to_file(&archive, stat.m_file_index, dest_file.c_str(), 0);
-                if (!res) {
-                    BOOST_LOG_TRIVIAL(error) << "[Orca Updater]extract file "<<stat.m_filename<<" to dest "<<dest_file<<" failed";
-                    close_zip_reader(&archive);
-                    return res;
-                }
-                BOOST_LOG_TRIVIAL(info) << "[Orca Updater]successfully extract file " << stat.m_file_index << " to "<<dest_file;
-            }
-            catch (const std::exception& e)
-            {
-                // ensure the zip archive is closed and rethrow the exception
-                close_zip_reader(&archive);
-                BOOST_LOG_TRIVIAL(error) << "[Orca Updater]Archive read exception:"<<e.what();
-                return false;
-            }
-        }
-        else {
-            BOOST_LOG_TRIVIAL(warning) << "[Orca Updater]Unzip: read file stat failed";
-        }
-    }
-    close_zip_reader(&archive);
-
-	return true;
+    const std::string parent_path = (!dest_path.empty() ? dest_path : source_path.parent_path()).string();
+    return extract_archive_confined(source_path.string(), parent_path);
 }
 
 // Remove a leftover partial archive for the vendor about to be synchronized.

tests/libslic3r/CMakeLists.txt

@@ -44,6 +44,7 @@ add_executable(${_TEST_NAME}_tests
     test_lay_on_face.cpp
     test_model.cpp
     test_utils.cpp
+    test_miniz_extension.cpp
     test_timeutils.cpp
     test_voronoi.cpp
     test_wipe_tower_estimate.cpp

tests/libslic3r/test_miniz_extension.cpp

@@ -0,0 +1,69 @@
+#include <catch2/catch_all.hpp>
+
+#include "libslic3r/miniz_extension.hpp"
+
+#include "test_utils.hpp"
+
+#include <boost/filesystem.hpp>
+
+#include <fstream>
+#include <iterator>
+#include <string>
+#include <utility>
+#include <vector>
+
+using namespace Slic3r;
+namespace fs = boost::filesystem;
+
+namespace {
+
+void write_zip(const fs::path &zip_file, const std::vector<std::pair<std::string, std::string>> &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<char>(in), std::istreambuf_iterator<char>());
+}
+
+} // 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));
+}