From eb6f62b40bcf7d06d2591684cc8fac81f0252a16 Mon Sep 17 00:00:00 2001 From: John McCrae Date: Tue, 22 Sep 2026 12:14:18 -0700 Subject: [PATCH] fix(tar): reject symlink targets that resolve outside destination chef-client -z --recipe-url downloads a tarball and extracts it with mixlib-archive, which followed symlinks in the archive without verifying that the symlink target stayed within the destination directory. A crafted tarball containing a symlink entry pointing outside destination, followed by a second entry whose path traverses that symlink, let an attacker write files anywhere on disk that the extracting process could reach (tar-slip). Since chef-client runs as root, this is an arbitrary file write as root and a path to full node compromise (e.g. dropping a cron.d file, systemd unit, or SSH authorized_keys entry). - Tar: before creating a symlink, resolve its target relative to its own directory and skip (with a warning) any symlink whose target resolves outside destination_root. Because every other write is already confined to destination_root, this closes the escape for any chain of symlinks. - LibArchive: always set EXTRACT_SECURE_SYMLINKS and EXTRACT_SECURE_NODOTDOT flags on extraction (previously only permission flags were applied), giving the same protection natively in the libarchive-backed extractor used in production. - Add regression specs proving the escape is blocked while legitimate in-destination symlinks continue to work. Signed-off-by: John McCrae john.mccrae@progress.com Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: John McCrae --- lib/mixlib/archive/lib_archive.rb | 7 ++++- lib/mixlib/archive/tar.rb | 16 ++++++++++-- spec/mixlib/lib_archive_spec.rb | 2 +- spec/mixlib/tar_spec.rb | 43 +++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lib/mixlib/archive/lib_archive.rb b/lib/mixlib/archive/lib_archive.rb index 1edb6dd..edeb6b8 100644 --- a/lib/mixlib/archive/lib_archive.rb +++ b/lib/mixlib/archive/lib_archive.rb @@ -18,7 +18,12 @@ def initialize(archive, options = {}) # ignore[Array]:: an array of matches of file paths to ignore def extract(destination, perms: true, ignore: []) ignore_re = Regexp.union(ignore) - flags = perms ? ::Archive::EXTRACT_PERM : nil + # EXTRACT_SECURE_SYMLINKS refuses to write through a symlink (including + # one from an earlier entry in this same archive) that would otherwise + # let a later entry escape destination (tar-slip). EXTRACT_SECURE_NODOTDOT + # closes the equivalent ".." based escape. + flags = ::Archive::EXTRACT_SECURE_SYMLINKS | ::Archive::EXTRACT_SECURE_NODOTDOT + flags |= ::Archive::EXTRACT_PERM if perms FileUtils.mkdir_p(destination) reader = ::Archive::Reader.open_filename(@archive) diff --git a/lib/mixlib/archive/tar.rb b/lib/mixlib/archive/tar.rb index 55fa1f2..d06960c 100644 --- a/lib/mixlib/archive/tar.rb +++ b/lib/mixlib/archive/tar.rb @@ -47,7 +47,7 @@ def extract(destination, perms: true, ignore: []) next end parent = File.dirname(dest) - FileUtils.mkdir_p(parent) + FileUtils.mkdir_p(parent) unless File.directory?(parent) if entry.directory? || (entry.header.typeflag == "" && entry.full_name.end_with?("/")) File.delete(dest) if File.file?(dest) @@ -66,7 +66,19 @@ def extract(destination, perms: true, ignore: []) FileUtils.chmod(entry.header.mode, dest, verbose: false) if perms elsif entry.header.typeflag == "2" # handle symlink - File.symlink(entry.header.linkname, dest) + # + # A symlink whose target resolves outside destination would let a + # *later* entry that writes "through" this symlink escape the + # destination directory entirely (tar-slip). Since every path we + # write is already confined to destination_root, refusing to create + # symlinks whose own target escapes destination_root guarantees no + # chain of symlinks can ever lead outside of it either. + link_target = File.expand_path(entry.header.linkname, File.dirname(dest)) + if link_target == destination_root || link_target.start_with?(destination_root + File::SEPARATOR) + File.symlink(entry.header.linkname, dest) + else + Mixlib::Archive::Log.warn "ignoring entry #{entry.full_name}: symlink target #{entry.header.linkname} resolves outside #{destination}" + end else Mixlib::Archive::Log.warn "unknown tar entry: #{entry.full_name} type: #{entry.header.typeflag}" end diff --git a/spec/mixlib/lib_archive_spec.rb b/spec/mixlib/lib_archive_spec.rb index a8d7505..1e5fd98 100644 --- a/spec/mixlib/lib_archive_spec.rb +++ b/spec/mixlib/lib_archive_spec.rb @@ -49,7 +49,7 @@ Mixlib::Archive::LibArchive.new(archive_path).create(file_paths, gzip: true) end expect(File.file?(archive_path)).to be true - Mixlib::Archive::LibArchive.new(archive_path).extract(target) + Mixlib::Archive::LibArchive.new(archive_path).extract(target, ignore: %w{ . .. }) expect(Dir.entries(target)).to match_array file_paths expect(File.size("#{target}/fixture_binary")).to eql 6 expect(File.size("#{target}/fixture_a")).to eql 10 diff --git a/spec/mixlib/tar_spec.rb b/spec/mixlib/tar_spec.rb index dc6d88e..e97c078 100644 --- a/spec/mixlib/tar_spec.rb +++ b/spec/mixlib/tar_spec.rb @@ -118,6 +118,49 @@ def write_longlink_tar(archive_path, longlink_target, payload) expect(File.read(File.join(target, long_name))).to eq("hello") end + + # Builds a tar with a symlink entry pointing outside of the extraction + # destination, followed by a regular file entry whose path travels + # "through" that symlink. Without a containment check on symlink targets + # this is the classic tar-slip attack: the symlink is planted first, then + # the second entry's write follows it to land anywhere on disk. + def write_symlink_escape_tar(archive_path, link_name, link_target, file_through_link, payload) + File.open(archive_path, "wb") do |f| + writer = Gem::Package::TarWriter.new(f) + writer.add_symlink(link_name, link_target, 0o777) + writer.add_file_simple(file_through_link, 0o644, payload.bytesize) { |io| io.write(payload) } + writer.close + end + end + + it "does not create a symlink that escapes destination" do + write_symlink_escape_tar(archive_path, "link", outside_path, "link/pwned.txt", "pwned!") + + described_class.new(archive_path).extract(target) + + expect(File.symlink?(File.join(target, "link"))).to be false + expect(File.exist?(outside_path)).to be false + end + + it "does not write a file outside destination via a symlinked path" do + FileUtils.mkdir_p(outside_path) + write_symlink_escape_tar(archive_path, "link", outside_path, "link/pwned.txt", "pwned!") + + described_class.new(archive_path).extract(target) + + expect(File.exist?(File.join(outside_path, "pwned.txt"))).to be false + end + + it "still allows symlinks that resolve within destination" do + real_dir = File.join(target, "realdir") + FileUtils.mkdir_p(real_dir) + write_symlink_escape_tar(archive_path, "link", real_dir, "link/ok.txt", "hello") + + described_class.new(archive_path).extract(target) + + expect(File.symlink?(File.join(target, "link"))).to be true + expect(File.read(File.join(target, "realdir", "ok.txt"))).to eq("hello") + end end describe "#is_tar_archive?" do