Moved tar.gz test to the geoip downloader part and avoid to support symlink just raising an error when a tgz contains a symlink

Author: andsel

lib/bootstrap/util/compress.rb

@@ -74,18 +74,12 @@ def extract(file, target)
           ::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)
-              elsif entry.symlink?
-                linkname = entry.header.linkname
-                unless LogStash::Util.symlink_target_safe?(linkname, target_path, target)
-                  raise CompressError.new("Refusing to extract symlink with unsafe target: #{entry.full_name} -> #{linkname}. Symlink target must remain inside extraction directory.")
-                end
-                FileUtils.mkdir_p(::File.dirname(target_path))
-                ::File.symlink(linkname, target_path)
-              else # is a file to be extracted
+              else # is a file to be extracted (symlinks rejected in verify_tar_entry_safe!)
                 ::File.open(target_path, "wb") { |f| f.write(entry.read) }
               end
             end
@@ -141,32 +135,12 @@ def gzip(path, target_file)
       end
     end
 
-    # Returns true if a symlink target (linkname) would resolve to a path under extraction_root
-    # when the symlink is created at symlink_path. Works on both Unix and Windows.
-    # @param linkname [String] symlink target (relative or absolute)
-    # @param symlink_path [String] full path where the symlink will be created
-    # @param extraction_root [String] root directory all paths must stay under
-    # @return [Boolean] true if resolved path is under extraction_root
-    def self.symlink_target_safe?(linkname, symlink_path, extraction_root)
-      return false if linkname.nil? || linkname.to_s.strip.empty?
-      symlink_dir = ::File.dirname(symlink_path)
-      resolved = Pathname.new(::File.expand_path(linkname, symlink_dir)).cleanpath
-      root = Pathname.new(::File.expand_path(extraction_root)).cleanpath
-      !resolved.relative_path_from(root).to_s.start_with?("..")
-    rescue ArgumentError
-      # relative_path_from raises if resolved is not under root
-      false
-    end
-
-    # Verifies that a path string is safe for extraction (relative, no parents traversal).
-    # Raises CompressError with a specific message if the path is nil/empty, absolute, or
-    # contains `..`. Does NOT handle symlinks, symlinks should be handled on per archive type basis.
-    # Works on both Unix and Windows.
-    # @param name [String] path string to validate
-    # @raise [CompressError] if path is nil, empty, absolute, or traverses with `..`
+    # 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. Path cannot be nil or 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?
@@ -176,5 +150,15 @@ def self.verify_name_safe!(name)
         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

@@ -24,7 +24,7 @@
 
 module LogStash module PluginManager module PackInstaller
   class Local
-    PACK_EXTENSIONS = ".zip"
+    PACK_EXTENSION = ".zip"
     LOGSTASH_PATTERN_RE = /logstash\/?/
 
     attr_reader :local_file

spec/unit/util/compress_spec.rb

@@ -78,6 +78,17 @@ def list_files(target)
       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
@@ -197,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
@@ -212,31 +223,18 @@ def list_files(target)
     end
 
     it "raises CompressError when an entry path traverses with .." do
-      tar_with_evil = [OpenStruct.new(:full_name => "../evil", :directory? => false, :read => "")]
+      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 "extracts a tar.gz containing a symlink and creates the symlink" do
-      fixture = ::File.join(::File.dirname(__FILE__), "..", "..", "support", "pack", "pack_with_symlink.tar.gz")
+    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__))
       skip("Fixture not found") unless ::File.exist?(fixture)
       target_dir = Stud::Temporary.pathname
-      subject.extract(fixture, target_dir)
-      symlink_path = ::File.join(target_dir, "logstash", "link_to_somefile")
-      expect(::File.symlink?(symlink_path)).to be true
-      expect(::File.readlink(symlink_path)).to eq("somefile.txt")
-    end
-
-    it "raises CompressError when a symlink target would escape the extraction directory" do
-      header = OpenStruct.new(:typeflag => "2", :linkname => "../../etc/passwd")
-      entry = OpenStruct.new(:full_name => "logstash/link_evil", :directory? => false, :symlink? => true, :header => header, :read => nil)
-      tar_with_evil_symlink = [entry]
-      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_symlink)
-      expect(FileUtils).to receive(:mkdir).with(target)
-      expect { subject.extract(source, target) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink with unsafe target/)
+      expect { subject.extract(fixture, target_dir) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink entry:.*GeoLite2-City-alias\.mmdb/)
     end
   end
 

x-pack/spec/geoip_database_management/downloader_spec.rb

@@ -189,18 +189,11 @@
       expect(::File.exist?(folder_less_path)).to be_truthy
     end
 
-    it "extracts tarball containing a symlink and preserves the symlink" do
+    it "raises when tarball contains a symlink entry" do
       zip_path = ::File.expand_path("./fixtures/sample_with_symlink.tgz", ::File.dirname(__FILE__))
       skip("sample_with_symlink.tgz not found") unless ::File.exist?(zip_path)
 
-      new_db_path = downloader.send(:unzip, database_type, dirname, zip_path)
-
-      expect(new_db_path).to match /GeoLite2-#{database_type}\.mmdb/
-      expect(::File.exist?(new_db_path)).to be_truthy
-
-      alias_path = data_path.resolve(dirname, "GeoLite2-City-alias.mmdb")
-      expect(::File.symlink?(alias_path)).to be true
-      expect(::File.readlink(alias_path)).to eq("GeoLite2-City.mmdb")
+      expect { downloader.send(:unzip, database_type, dirname, zip_path) }.to raise_error(LogStash::CompressError, /Refusing to extract symlink entry/)
     end
   end
 

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

@@ -2,7 +2,7 @@
 
 ## 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. Run from this directory (Unix/macOS):
+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