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