diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index b21da72fc8..ee9e00fcae 100644 --- a/src/libslic3r/Utils.hpp +++ b/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"); } diff --git a/src/libslic3r/miniz_extension.cpp b/src/libslic3r/miniz_extension.cpp index 620cb6fb27..4d9b528267 100644 --- a/src/libslic3r/miniz_extension.cpp +++ b/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); diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 9def5dad17..77c7d4c683 100644 --- a/src/libslic3r/utils.cpp +++ b/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"); diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 99f0d8088a..dd2a9d8269 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/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) { diff --git a/tests/libslic3r/test_miniz_extension.cpp b/tests/libslic3r/test_miniz_extension.cpp index 6d82799ed4..ff3274a886 100644 --- a/tests/libslic3r/test_miniz_extension.cpp +++ b/tests/libslic3r/test_miniz_extension.cpp @@ -6,6 +6,7 @@ #include +#include #include #include #include @@ -28,6 +29,33 @@ void write_zip(const fs::path &zip_file, const std::vector(in), std::istreambuf_iterator()); + } + 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 list_dir(const fs::path &dir) +{ + std::vector 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{"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 diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 7880b783f1..572ae6e5ae 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -152,3 +152,71 @@ TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed uncha REQUIRE(resolve_cli_input_path("").empty()); } } + +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(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