Adds verification of the file name

lib/bootstrap/util/compress.rb

@@ -20,6 +20,7 @@
 require "fileutils"
 require "zlib"
 require "stud/temporary"
+require "pathname"
 
 module LogStash
   class CompressError < StandardError; end
@@ -36,6 +37,7 @@ def extract(source, target, pattern = nil)
         raise CompressError.new("Directory #{target} exist") if ::File.exist?(target)
         ::Zip::File.open(source) do |zip_file|
           zip_file.each do |file|
+            LogStash::Util.verify_name_safe!(file.name)
             path = ::File.join(target, file.name)
             FileUtils.mkdir_p(::File.dirname(path))
             zip_file.extract(file, path) if pattern.nil? || pattern =~ file.name
@@ -72,11 +74,13 @@ def extract(file, target)
         Zlib::GzipReader.open(file) do |gzip_file|
           ::Gem::Package::TarReader.new(gzip_file) do |tar_file|
             tar_file.each do |entry|
+              LogStash::Util.verify_name_safe!(entry.full_name)
+              LogStash::Util.verify_tar_link_entry!(entry)
               target_path = ::File.join(target, entry.full_name)
 
               if entry.directory?
                 FileUtils.mkdir_p(target_path)
-              else # is a file to be extracted
+              else # is a file to be extracted (symlinks rejected in verify_tar_link_entry!)
                 ::File.open(target_path, "wb") { |f| f.write(entry.read) }
               end
             end
@@ -131,5 +135,31 @@ def gzip(path, target_file)
         end
       end
     end
+
+    # Verifies a path string is safe for zip or tar entry names (relative, no `..` traversal).
+    # @param name [String] archive entry path (e.g. Zip entry name or tar full_name)
+    # @raise [CompressError] if name is nil, empty, absolute, or traverses with `..`
+    def self.verify_name_safe!(name)
+      if name.nil? || name.to_s.strip.empty?
+        raise CompressError.new("Refusing to extract file to unsafe path. Path cannot be nil or empty.")
+      end
+      cleanpath = Pathname.new(name).cleanpath
+      if cleanpath.absolute?
+        raise CompressError.new("Refusing to extract file to unsafe path: #{name}. Absolute paths are not allowed.")
+      end
+      if cleanpath.each_filename.to_a.include?("..")
+        raise CompressError.new("Refusing to extract file to unsafe path: #{name}. Files may not traverse with `..`")
+      end
+    end
+
+    # Tar.gz-only: rejects symlink entries
+    # @param entry [Gem::Package::TarReader::Entry]
+    # @raise [CompressError] if entry is a symlink or path is unsafe
+    def self.verify_tar_link_entry!(entry)
+      if entry.symlink?
+        raise CompressError.new("Refusing to extract symlink entry: #{entry.full_name}")
+      end
+    end
+    #private :verify_tar_link_entry!
   end
 end

lib/pluginmanager/pack_installer/local.rb

@@ -75,6 +75,8 @@ def uncompress(source)
       # OK Zip's handling of file is bit weird, if the file exist but is not a valid zip, it will raise
       # a `Zip::Error` exception with a file not found message...
       raise InvalidPackError, "Cannot uncompress the zip: #{source}"
+    rescue LogStash::CompressError => e
+      raise InvalidPackError, "Cannot uncompress the archive: #{e.message}"
     end
 
     def valid_format?(local_file)

spec/unit/util/compress_spec.rb

@@ -53,6 +53,44 @@ def list_files(target)
   Dir.glob(::File.join(target, "**", "*")).select { |f| ::File.file?(f) }.size
 end
 
+describe LogStash::Util do
+  context "verify entry files destinations" do
+    it "raises CompressError for nil or empty path" do
+      expect { LogStash::Util.verify_name_safe!(nil) }.to raise_error(LogStash::CompressError, /Path cannot be nil or empty/)
+      expect { LogStash::Util.verify_name_safe!("") }.to raise_error(LogStash::CompressError, /Path cannot be nil or empty/)
+      expect { LogStash::Util.verify_name_safe!("   ") }.to raise_error(LogStash::CompressError, /Path cannot be nil or empty/)
+    end
+
+    it "raises CompressError for absolute paths" do
+      expect { LogStash::Util.verify_name_safe!("/etc/passwd") }.to raise_error(LogStash::CompressError, /Absolute paths are not allowed/)
+      expect { LogStash::Util.verify_name_safe!("/foo/bar") }.to raise_error(LogStash::CompressError, /Absolute paths are not allowed/)
+    end
+
+    it "raises CompressError for relative paths that traverse with .." do
+      expect { LogStash::Util.verify_name_safe!("../foo") }.to raise_error(LogStash::CompressError, /Files may not traverse with `..`/)
+      expect { LogStash::Util.verify_name_safe!("..") }.to raise_error(LogStash::CompressError, /Files may not traverse with `..`/)
+      expect { LogStash::Util.verify_name_safe!("a/../../etc") }.to raise_error(LogStash::CompressError, /Files may not traverse with `..`/)
+    end
+
+    it "does not raise for safe relative paths" do
+      expect { LogStash::Util.verify_name_safe!("foo/bar") }.not_to raise_error
+      expect { LogStash::Util.verify_name_safe!("a/b/c") }.not_to raise_error
+      expect { LogStash::Util.verify_name_safe!("single") }.not_to raise_error
+    end
+  end
+
+  context "#verify_tar_link_entry!" do
+    def tar_entry(full_name, symlink: false, directory: false)
+      OpenStruct.new(:full_name => full_name, :symlink? => symlink, :directory? => directory)
+    end
+
+    it "raises CompressError for symlink entries without path validation" do
+      entry = tar_entry("logstash/link", symlink: true)
+      expect { LogStash::Util.verify_tar_link_entry!(entry) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink entry: logstash\/link/)
+    end
+  end
+end
+
 describe LogStash::Util::Zip do
   subject { Class.new { extend LogStash::Util::Zip } }
 
@@ -79,6 +117,12 @@ def list_files(target)
       subject.extract(source, target)
     end
 
+    it "raises CompressError when an entry path traverses with .." do
+      zip_with_evil = [OpenStruct.new(:name => "../evil")]
+      allow(Zip::File).to receive(:open).with(source).and_yield(zip_with_evil)
+      expect { subject.extract(source, target) }.to raise_error(LogStash::CompressError, /Refusing to extract file to unsafe path.*Files may not traverse with `..`/)
+    end
+
     context "patterns" do
       # Theses tests sound duplicated but they are actually better than the other one
       # since they do not involve any mocks.
@@ -164,7 +208,7 @@ def list_files(target)
 
     let(:tar_file) do
       ["foo", "bar", "zoo"].inject([]) do |acc, name|
-        acc << OpenStruct.new(:full_name => name)
+        acc << OpenStruct.new(:full_name => name, :symlink? => false, :directory? => false)
         acc
       end
     end
@@ -177,6 +221,21 @@ def list_files(target)
       expect(File).to receive(:open).exactly(3).times
       subject.extract(source, target)
     end
+
+    it "raises CompressError when an entry path traverses with .." do
+      tar_with_evil = [OpenStruct.new(:full_name => "../evil", :directory? => false, :symlink? => false, :read => "")]
+      allow(Zlib::GzipReader).to receive(:open).with(source).and_yield(gzip_file)
+      allow(Gem::Package::TarReader).to receive(:new).with(gzip_file).and_yield(tar_with_evil)
+      expect(FileUtils).to receive(:mkdir).with(target)
+      expect { subject.extract(source, target) }.to raise_error(LogStash::CompressError, /Refusing to extract file to unsafe path.*Files may not traverse with `..`/)
+    end
+
+    it "raises CompressError when tarball contains a symlink entry" do
+      fixture = ::File.expand_path("../../../x-pack/spec/geoip_database_management/fixtures/sample_with_symlink.tgz", ::File.dirname(__FILE__))
+      expect(::File).to exist(fixture)
+      target_dir = Stud::Temporary.pathname
+      expect { subject.extract(fixture, target_dir) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink entry:.*GeoLite2-City-alias\.mmdb/)
+    end
   end
 
   context "#compression" do

x-pack/spec/geoip_database_management/downloader_spec.rb

@@ -188,6 +188,13 @@
       expect(::File.exist?(folder_more_path)).to be_truthy
       expect(::File.exist?(folder_less_path)).to be_truthy
     end
+
+    it "raises when tarball contains a symlink entry" do
+      zip_path = ::File.expand_path("./fixtures/sample_with_symlink.tgz", ::File.dirname(__FILE__))
+      expect(::File.exist?(zip_path)).to be_truthy
+
+      expect { downloader.send(:unzip, database_type, dirname, zip_path) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink entry/)
+    end
   end
 
   context "assert_database!" do

x-pack/spec/geoip_database_management/fixtures/README.md

@@ -0,0 +1,13 @@
+# GeoIP downloader spec fixtures
+
+## Recreating `sample_with_symlink.tgz`
+
+This archive is the same as `sample.tgz` plus a symbolic link `GeoLite2-City-alias.mmdb` → `GeoLite2-City.mmdb` at the archive root. `LogStash::Util::Tar.extract` rejects symlink entries, so this fixture is only for specs that assert that behavior. Run from this directory (Unix/macOS):
+
+```bash
+cd "$(dirname "$0")"  # or: cd x-pack/spec/geoip_database_management/fixtures
+rm -rf _symlink_build && mkdir _symlink_build && tar -xzf sample.tgz -C _symlink_build
+ln -s GeoLite2-City.mmdb _symlink_build/GeoLite2-City-alias.mmdb
+tar -czf sample_with_symlink.tgz -C _symlink_build .
+rm -rf _symlink_build
+```