Switched from boolean method to verification method that thorw different exception for each case

Author: andsel

lib/bootstrap/util/compress.rb

@@ -36,7 +36,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|
-            raise CompressError.new("Extracting file outside target directory: #{file.name}") unless LogStash::Util.name_safe?(file.name)
+            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
@@ -73,7 +73,7 @@ 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|
-              raise CompressError.new("Extracting file outside target directory: #{entry.full_name}") unless LogStash::Util.name_safe?(entry.full_name)
+              LogStash::Util.verify_name_safe!(entry.full_name)
               target_path = ::File.join(target, entry.full_name)
 
               if entry.directory?
@@ -134,21 +134,22 @@ def gzip(path, target_file)
       end
     end
 
-    #TODO handle symlink case, this is a bit more complex as we need to check the real path of the file
-    # to be extracted and make sure it is still in the target directory. This is not handled by Zip::File and is a known security issue:
-    #
-    # Returns true if the path string is safe (no path traversal, no absolute path).
-    # A path is safe when it is relative and does not contain `..` segments that
-    # could escape the target directory. Works on both Unix and Windows.
-    # @param path [String] path string to validate
-    # @return [Boolean] true if safe, false if absolute or relative-with-escape
-    def self.name_safe?(name)
-      return false if name.nil? || name.to_s.strip.empty?
-      pn = Pathname.new(name)
-      cleanpath = pn.cleanpath
-      return false if cleanpath.absolute?
-      return false if cleanpath.each_filename.to_a.include?("..")
-      true
+    # Verifies that a path string is safe for extraction (relative, no `..` traversal).
+    # Raises CompressError with a specific message if the path is nil/empty, absolute, or
+    # contains `..`. Does NOT handle symlinks. Works on both Unix and Windows.
+    # @param name [String] path string to validate
+    # @raise [CompressError] if path 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
   end
 end

spec/unit/util/compress_spec.rb

@@ -53,6 +53,33 @@ def list_files(target)
   Dir.glob(::File.join(target, "**", "*")).select { |f| ::File.file?(f) }.size
 end
 
+describe LogStash::Util do
+  describe ".verify_name_safe!" 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
+end
+
 describe LogStash::Util::Zip do
   subject { Class.new { extend LogStash::Util::Zip } }
 
@@ -79,6 +106,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.
@@ -177,6 +210,14 @@ 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, :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
   end
 
   context "#compression" do