Harden Archive Extraction Against Symlinks

The plugin installer now creates a symlink entry only when its target is
relative and, joined to the link's own directory, passes
is_path_within_root, via the new is_symlink_target_within_root helper.
Before writing any entry it checks the destination with symlink_status, so
an existing symlink, dangling or not, is replaced rather than followed, and
it creates parent directories inside the existing error handling.
extract_archive_confined replaces a symlink at a destination file the same
way.

is_path_within_root now ignores a trailing separator on the root, which
previously made every path fail the check.

src/libslic3r/Utils.hpp

@@ -260,6 +260,9 @@ extern bool is_json_file(const std::string& path);
 // Both '/' and '\\' are treated as separators on every platform, so an archive rejected on one OS
 // is rejected on all of them.
 extern bool is_path_within_root(const std::string &rel_path, const boost::filesystem::path &root);
+// True if a symlink stored at link_rel_path (relative to root) with this target stays inside root: the target
+// must be relative, and joined to the link's directory it must pass is_path_within_root.
+extern bool is_symlink_target_within_root(const std::string &link_rel_path, const std::string &target, const boost::filesystem::path &root);
 
 // Orca: custom protocal support utils
 inline bool is_orca_open(const std::string& url) { return boost::starts_with(url, "orcaslicer://open"); }

src/libslic3r/miniz_extension.cpp

@@ -158,6 +158,10 @@ bool extract_archive_confined(const std::string &zip_path_utf8, const std::strin
                 BOOST_LOG_TRIVIAL(warning) << "Unzip: invalid size for file " << stat.m_filename;
                 continue;
             }
+            // Replace a symlink at the destination rather than writing through it.
+            const boost::filesystem::path dest_path(dest_file);
+            if (boost::filesystem::is_symlink(boost::filesystem::symlink_status(dest_path)))
+                boost::filesystem::remove(dest_path);
             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);

src/libslic3r/utils.cpp

@@ -1103,7 +1103,10 @@ bool is_path_within_root(const std::string &rel_path, const boost::filesystem::p
     }
     // Resolve against the canonical root so a symlink inside it cannot lead back out.
     try {
-        const std::string root_str = boost::filesystem::weakly_canonical(root).string();
+        std::string root_str = boost::filesystem::weakly_canonical(root).string();
+        // A trailing separator on root would otherwise fail the prefix match below for every path.
+        while (!root_str.empty() && (root_str.back() == '/' || root_str.back() == boost::filesystem::path::preferred_separator))
+            root_str.pop_back();
         const std::string full_str = boost::filesystem::weakly_canonical(root / rel_path).string();
         return full_str.compare(0, root_str.size(), root_str) == 0 &&
                (full_str.size() == root_str.size() || full_str[root_str.size()] == boost::filesystem::path::preferred_separator);
@@ -1112,6 +1115,15 @@ bool is_path_within_root(const std::string &rel_path, const boost::filesystem::p
     }
 }
 
+bool is_symlink_target_within_root(const std::string &link_rel_path, const std::string &target, const boost::filesystem::path &root)
+{
+    if (target.empty() || target.front() == '/' || target.front() == '\\' || (target.size() > 1 && target[1] == ':'))
+        return false;
+    // A relative target without ".." only descends from the link's directory, so no chain of such links can leave root.
+    const size_t sep = link_rel_path.find_last_of("/\\");
+    return is_path_within_root((sep == std::string::npos ? std::string() : link_rel_path.substr(0, sep + 1)) + target, root);
+}
+
 bool is_img_file(const std::string &path)
 {
 	return boost::iends_with(path, ".png") || boost::iends_with(path, ".svg");

src/slic3r/GUI/GUI_App.cpp

@@ -1519,10 +1519,11 @@ int GUI_App::install_plugin(std::string name, std::string package_name, InstallP
                     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());
                 try {
-                    if (fs::exists(dest_path)) {
+                    boost::filesystem::create_directories(dest_path.parent_path());
+                    // symlink_status so that an existing symlink, dangling or not, is replaced rather than written through.
+                    if (fs::exists(fs::symlink_status(dest_path))) {
                         boost::system::error_code ec;
                         fs::remove(dest_path, ec);
                         if (ec) {
@@ -1551,8 +1552,14 @@ int GUI_App::install_plugin(std::string name, std::string package_name, InstallP
                     mz_bool res = 0;
 #ifndef WIN32
                     if (S_ISLNK(stat.m_external_attr >> 16)) {
-                        std::string link(stat.m_uncomp_size + 1, 0);
+                        std::string link(stat.m_uncomp_size, 0);
                         res = mz_zip_reader_extract_to_mem(&archive, stat.m_file_index, link.data(), stat.m_uncomp_size, 0);
+                        if (res && !is_symlink_target_within_root(dest_file, link, plugin_folder)) {
+                            BOOST_LOG_TRIVIAL(error) << "[install_plugin] link " << dest_file << " -> " << link << " resolves outside " << plugin_folder.string();
+                            close_zip_reader(&archive);
+                            if (pro_fn) { pro_fn(InstallStatusUnzipFailed, 0, cancel); }
+                            return InstallStatusUnzipFailed;
+                        }
                         try {
                             boost::filesystem::create_symlink(link, dest_path);
                         } catch (const std::exception &e) {

tests/libslic3r/test_miniz_extension.cpp

@@ -6,6 +6,7 @@
 
 #include <boost/filesystem.hpp>
 
+#include <algorithm>
 #include <fstream>
 #include <iterator>
 #include <string>
@@ -28,6 +29,33 @@ void write_zip(const fs::path &zip_file, const std::vector<std::pair<std::string
     REQUIRE(close_zip_writer(&zip));
 }
 
+// miniz refuses to write a name starting with '/', so write a placeholder of the same length and patch it in place.
+void rename_entry(const fs::path &zip_file, const std::string &from, const std::string &to)
+{
+    REQUIRE(from.size() == to.size());
+    std::string bytes;
+    {
+        std::ifstream in(zip_file.string(), std::ios::binary);
+        bytes.assign(std::istreambuf_iterator<char>(in), std::istreambuf_iterator<char>());
+    }
+    size_t count = 0;
+    for (size_t pos = bytes.find(from); pos != std::string::npos; pos = bytes.find(from, pos + to.size()), ++count)
+        bytes.replace(pos, from.size(), to);
+    // Once in the local header and once in the central directory.
+    REQUIRE(count == 2);
+    std::ofstream out(zip_file.string(), std::ios::binary | std::ios::trunc);
+    out << bytes;
+}
+
+std::vector<std::string> list_dir(const fs::path &dir)
+{
+    std::vector<std::string> names;
+    for (const fs::directory_entry &entry : fs::directory_iterator(dir))
+        names.push_back(entry.path().filename().string());
+    std::sort(names.begin(), names.end());
+    return names;
+}
+
 std::string read_file(const fs::path &file)
 {
     std::ifstream in(file.string(), std::ios::binary);
@@ -58,7 +86,8 @@ TEST_CASE("Confined extraction rejects an archive with an entry outside the targ
     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"));
+                                                std::string("sub/../../escape.txt"), std::string("C:/escape.txt"),
+                                                std::string("C:escape.txt"), std::string("\\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"}});
 
@@ -67,3 +96,99 @@ TEST_CASE("Confined extraction rejects an archive with an entry outside the targ
     CHECK_FALSE(fs::exists(tmp.path() / "escape.txt"));
     CHECK(fs::is_empty(target));
 }
+
+TEST_CASE("Confined extraction rejects an archive with an absolute entry name", "[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 absolute    = (tmp.path() / "escape.txt").generic_string();
+    const std::string placeholder = "#" + absolute.substr(1);
+    write_zip(zip_file, {{"normal.json", "{}"}, {placeholder, "escaped"}});
+    rename_entry(zip_file, placeholder, absolute);
+
+    CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
+    CHECK_FALSE(fs::exists(tmp.path() / "escape.txt"));
+    CHECK(fs::is_empty(target));
+}
+
+TEST_CASE("Confined extraction rejects a directory 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);
+    write_zip(zip_file, {{"vendor/", ""}, {"../outside/", ""}});
+
+    CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
+    CHECK_FALSE(fs::exists(tmp.path() / "outside"));
+    CHECK(fs::is_empty(target));
+}
+
+TEST_CASE("Confined extraction validates zero-size entries like any other", "[MinizExtension]")
+{
+    ScopedTemporaryDir tmp;
+    const fs::path     zip_file = tmp.path() / "bundle.zip";
+    const fs::path     target   = tmp.path() / "cache";
+    fs::create_directories(target);
+
+    SECTION("an empty file inside the target does not fail the archive") {
+        write_zip(zip_file, {{"empty.json", ""}, {"vendor.json", "{}"}});
+        CHECK(extract_archive_confined(zip_file.string(), target.string()));
+        CHECK(read_file(target / "vendor.json") == "{}");
+    }
+    SECTION("an empty file outside the target rejects the archive") {
+        write_zip(zip_file, {{"vendor.json", "{}"}, {"../escape.txt", ""}});
+        CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
+        CHECK_FALSE(fs::exists(tmp.path() / "escape.txt"));
+        CHECK(fs::is_empty(target));
+    }
+}
+
+TEST_CASE("Confined extraction writes nothing outside the target for Windows-specific name forms", "[MinizExtension]")
+{
+    ScopedTemporaryDir tmp;
+    const fs::path     zip_file = tmp.path() / "bundle.zip";
+    const fs::path     target   = tmp.path() / "cache";
+    fs::create_directories(target);
+
+    // Windows strips trailing dots and spaces and maps device names; whether these extract depends on the
+    // platform, but none of them may land beside the target.
+    const std::string name = GENERATE(std::string("name."), std::string("name "), std::string("..."), std::string(".. "),
+                                      std::string(".. /escape.txt"), std::string(".../escape.txt"), std::string("CON"),
+                                      std::string("sub/NUL.txt"), std::string("C:escape.txt"));
+    write_zip(zip_file, {{name, "payload"}});
+
+    CAPTURE(name);
+    extract_archive_confined(zip_file.string(), target.string());
+    CHECK(list_dir(tmp.path()) == std::vector<std::string>{"bundle.zip", "cache"});
+}
+
+#ifndef _WIN32
+TEST_CASE("Confined extraction replaces a symlink at the destination instead of writing through it", "[MinizExtension]")
+{
+    ScopedTemporaryDir tmp;
+    const fs::path     zip_file = tmp.path() / "bundle.zip";
+    const fs::path     target   = tmp.path() / "cache";
+    const fs::path     outside  = tmp.path() / "outside";
+    fs::create_directories(target);
+    fs::create_directories(outside);
+    write_zip(zip_file, {{"vendor.json", "{\"a\":1}"}});
+
+    SECTION("a dangling symlink") {
+        fs::create_symlink(outside / "vendor.json", target / "vendor.json");
+        CHECK(extract_archive_confined(zip_file.string(), target.string()));
+        CHECK_FALSE(fs::exists(outside / "vendor.json"));
+        CHECK_FALSE(fs::is_symlink(fs::symlink_status(target / "vendor.json")));
+        CHECK(read_file(target / "vendor.json") == "{\"a\":1}");
+    }
+    SECTION("a symlink to an existing file") {
+        { std::ofstream((outside / "vendor.json").string()) << "original"; }
+        fs::create_symlink(outside / "vendor.json", target / "vendor.json");
+        extract_archive_confined(zip_file.string(), target.string());
+        CHECK(read_file(outside / "vendor.json") == "original");
+    }
+}
+#endif

tests/libslic3r/test_utils.cpp

@@ -243,3 +243,71 @@ TEST_CASE("find_unused_filename gives up after 999 versions", "[Utils]") {
     REQUIRE_FALSE(find_unused_filename(dir.path(), "model.3mf", {}, name));
     CHECK(name == "model(999).3mf");
 }
+
+TEST_CASE("is_path_within_root accepts a root given with a trailing separator", "[utils]") {
+    ScopedTemporaryDir tmp;
+    const std::string root = tmp.path().string();
+    const std::string with_separator = GENERATE_COPY(root + "/", root + std::string(1, static_cast<char>(boost::filesystem::path::preferred_separator)));
+
+    CAPTURE(with_separator);
+    CHECK(is_path_within_root("vendor.json", with_separator));
+    CHECK(is_path_within_root("vendor/machine/printer.json", with_separator));
+    CHECK_FALSE(is_path_within_root("../vendor.json", with_separator));
+}
+
+TEST_CASE("is_path_within_root treats Windows-specific name forms the same on every platform", "[utils]") {
+    ScopedTemporaryDir tmp;
+
+    SECTION("names ending in dots or spaces stay inside the root") {
+        const std::string name = GENERATE(std::string("name."), std::string("name "), std::string("dir./file.json"), std::string("dir /file.json"));
+        CAPTURE(name);
+        CHECK(is_path_within_root(name, tmp.path()));
+    }
+    SECTION("drive-relative names are rejected") {
+        const std::string name = GENERATE(std::string("C:x"), std::string("c:x/y.json"), std::string("C:"));
+        CAPTURE(name);
+        CHECK_FALSE(is_path_within_root(name, tmp.path()));
+    }
+}
+
+TEST_CASE("is_symlink_target_within_root accepts relative targets that stay inside the root", "[utils]") {
+    ScopedTemporaryDir tmp;
+    const auto [link, target] = GENERATE(std::make_pair(std::string("Versions/Current"), std::string("A")),
+                                         std::make_pair(std::string("Foo.framework/Foo"), std::string("Versions/Current/Foo")),
+                                         std::make_pair(std::string("libfoo.so"), std::string("libfoo.so.1")),
+                                         std::make_pair(std::string("a/b/link"), std::string("c/d")));
+    CAPTURE(link, target);
+    CHECK(is_symlink_target_within_root(link, target, tmp.path()));
+}
+
+TEST_CASE("is_symlink_target_within_root rejects absolute targets and targets that climb out", "[utils]") {
+    ScopedTemporaryDir tmp;
+    const std::string outside = (tmp.path().parent_path() / "outside").generic_string();
+    const auto [link, target] = GENERATE_COPY(std::make_pair(std::string("sub/link"), outside),
+                                              std::make_pair(std::string("sub/link"), std::string("/etc/passwd")),
+                                              std::make_pair(std::string("sub/link"), std::string("\\outside")),
+                                              std::make_pair(std::string("sub/link"), std::string("C:/outside")),
+                                              std::make_pair(std::string("sub/link"), std::string("C:outside")),
+                                              std::make_pair(std::string("sub/link"), std::string("")),
+                                              std::make_pair(std::string("link"), std::string("..")),
+                                              std::make_pair(std::string("link"), std::string("../outside")),
+                                              std::make_pair(std::string("sub/link"), std::string("../../outside")),
+                                              std::make_pair(std::string("sub/link"), std::string("x/../../../outside")),
+                                              std::make_pair(std::string("sub/link"), std::string("..\\..\\outside")));
+    CAPTURE(link, target);
+    CHECK_FALSE(is_symlink_target_within_root(link, target, tmp.path()));
+}
+
+#ifndef _WIN32
+TEST_CASE("is_symlink_target_within_root rejects a target that passes through a symlink leading out", "[utils]") {
+    ScopedTemporaryDir tmp;
+    const boost::filesystem::path root    = tmp.path() / "root";
+    const boost::filesystem::path outside = tmp.path() / "outside";
+    boost::filesystem::create_directories(root);
+    boost::filesystem::create_directories(outside);
+    boost::filesystem::create_symlink(outside, root / "out");
+
+    CHECK_FALSE(is_symlink_target_within_root("link", "out/lib.so", root));
+    CHECK(is_symlink_target_within_root("link", "in/lib.so", root));
+}
+#endif