Adds verification of the file name (#18848) (#18909)

Verify the destination path to check some rules that avoid deliberate path traversal during extraction.
- avoid absolute path
- avoid symbolic links (only for tar.gz file)
- avoid paths with parent navigation, for example: `logstash/../../../something/in/target/host`

Covered all these branches with unit tests.

(cherry picked from commit c4d7362)

Co-authored-by: Andrea Selva <selva.andre@gmail.com>

Author: mergify[bot]

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,9 +37,11 @@ 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|
-            path = ::File.join(target, file.name)
+            LogStash::Util.verify_name_safe!(file.name)
+            safe_name = Pathname.new(file.name).cleanpath.to_s
+            path = ::File.join(target, safe_name)
             FileUtils.mkdir_p(::File.dirname(path))
-            zip_file.extract(file, path) if pattern.nil? || pattern =~ file.name
+            zip_file.extract(file, path) if pattern.nil? || pattern =~ safe_name
           end
         end
       end
@@ -72,11 +75,14 @@ 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!)
+                FileUtils.mkdir_p(::File.dirname(target_path))
                 ::File.open(target_path, "wb") { |f| f.write(entry.read) }
               end
             end
@@ -131,5 +137,30 @@ 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
+    def self.verify_tar_link_entry!(entry)
+      if entry.symlink?
+        raise CompressError.new("Refusing to extract symlink entry: #{entry.full_name}")
+      end
+    end
   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 zip #{source}: #{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.
@@ -153,7 +197,11 @@ def list_files(target)
 
   context "#extraction" do
     let(:source) { File.join(File.expand_path("."), "source_file.tar.gz") }
-    let(:target) { File.expand_path("target_dir") }
+    let(:target) { Stud::Temporary.pathname }
+
+    after do
+      FileUtils.rm_rf(target) if File.exist?(target)
+    end
 
     it "raise an exception if the target dir exist" do
       allow(File).to receive(:exist?).with(target).and_return(true)
@@ -164,7 +212,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
@@ -173,10 +221,26 @@ def list_files(target)
       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_file)
 
+      allow(FileUtils).to receive(:mkdir_p)
       expect(FileUtils).to receive(:mkdir).with(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)
+      allow(FileUtils).to receive(:mkdir_p)
+      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)
+      expect { subject.extract(fixture, target) }.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

@@ -154,10 +154,10 @@
 
     context "with matching md5 checksum" do
       let(:md5_hash) { LogStash::GeoipDatabaseManagement::Util.md5(sample_city_db_gz) }
-      it "should download file and return zip path" do
-        new_zip_path = downloader.send(:download_database, database_type, dirname, db_info)
-        expect(new_zip_path).to match /GeoLite2-City\.tgz/
-        expect(::File.exist?(new_zip_path)).to be_truthy
+      it "should download file and return tgz path" do
+        new_tgz_path = downloader.send(:download_database, database_type, dirname, db_info)
+        expect(new_tgz_path).to match /GeoLite2-City\.tgz/
+        expect(::File.exist?(new_tgz_path)).to be_truthy
       end
     end
   end
@@ -176,8 +176,8 @@
     end
 
     it "should extract all files in tarball" do
-      zip_path = ::File.expand_path("./fixtures/sample.tgz", ::File.dirname(__FILE__))
-      new_db_path = downloader.send(:unzip, database_type, dirname, zip_path)
+      tgz_path = ::File.expand_path("./fixtures/sample.tgz", ::File.dirname(__FILE__))
+      new_db_path = downloader.send(:unzip, database_type, dirname, tgz_path)
 
       expect(new_db_path).to match /GeoLite2-#{database_type}\.mmdb/
       expect(::File.exist?(new_db_path)).to be_truthy
@@ -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
+      tgz_path = ::File.expand_path("./fixtures/sample_with_symlink.tgz", ::File.dirname(__FILE__))
+      expect(::File.exist?(tgz_path)).to be_truthy
+
+      expect { downloader.send(:unzip, database_type, dirname, tgz_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,14 @@
+# 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
+# From the repository root:
+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
+```