Ensure that entries can be extracted safely without path traversal.

This commit adds a parameter to the `File#extract` and `Entry#extract` methods
so that a base destination directory can be specified for extracting archives
in bulk to somewhere in the filesystem that isn't the current working
directory. This directory is `.` by default. It is combined with the entry
path - which shouldn't but could have relative directories (e.g. `..`) in it -
and tested for safety before extracting.

Resolves rubyzip#540.

lib/zip/entry.rb

@@ -247,21 +247,25 @@ def next_header_offset #:nodoc:all
       local_entry_offset + compressed_size
     end
 
-    # Extracts entry to file dest_path (defaults to @name).
-    # NB: The caller is responsible for making sure dest_path is safe, if it
-    # is passed.
-    def extract(dest_path = nil, &block)
-      if dest_path.nil? && !name_safe?
-        warn "WARNING: skipped '#{@name}' as unsafe."
+    # Extracts this entry to a file at `entry_path`, with
+    # `destination_directory` as the base location in the filesystem.
+    #
+    # NB: The caller is responsible for making sure `destination_directory` is
+    # safe, if it is passed.
+    def extract(entry_path = @name, destination_directory: '.', &block)
+      dest_dir = ::File.absolute_path(destination_directory || '.')
+      extract_path = ::File.absolute_path(::File.join(dest_dir, entry_path))
+
+      unless extract_path.start_with?(dest_dir)
+        warn "WARNING: skipped extracting '#{@name}' to '#{extract_path}' as unsafe."
         return self
       end
 
-      dest_path ||= @name
       block ||= proc { ::Zip.on_exists_proc }
 
       raise "unknown file type #{inspect}" unless directory? || file? || symlink?
 
-      __send__("create_#{ftype}", dest_path, &block)
+      __send__("create_#{ftype}", extract_path, &block)
       self
     end
 

lib/zip/file.rb

@@ -245,11 +245,16 @@ def replace(entry, src_path)
       add(entry, src_path)
     end
 
-    # Extracts entry to file dest_path.
-    def extract(entry, dest_path, &block)
+    # Extracts `entry` to a file at `entry_path`, with `destination_directory`
+    # as the base location in the filesystem.
+    #
+    # NB: The caller is responsible for making sure `destination_directory` is
+    # safe, if it is passed.
+    def extract(entry, entry_path = nil, destination_directory: '.', &block)
       block ||= proc { ::Zip.on_exists_proc }
       found_entry = get_entry(entry)
-      found_entry.extract(dest_path, &block)
+      entry_path ||= found_entry.name
+      found_entry.extract(entry_path, destination_directory: destination_directory, &block)
     end
 
     # Commits changes that has been made since the previous commit to

test/file_extract_directory_test.rb

@@ -48,7 +48,7 @@ def test_extract_directory_exists_as_file_overwrite
     called = false
     extract_test_dir do |entry, dest_path|
       called = true
-      assert_equal(TEST_OUT_NAME, dest_path)
+      assert_equal(File.absolute_path(TEST_OUT_NAME), dest_path)
       assert(entry.directory?)
       true
     end

test/file_extract_test.rb

@@ -5,6 +5,7 @@
 class ZipFileExtractTest < MiniTest::Test
   include CommonZipFileFixture
   EXTRACTED_FILENAME = 'test/data/generated/extEntry'
+  EXTRACTED_FILENAME_ABS = ::File.absolute_path(EXTRACTED_FILENAME)
   ENTRY_TO_EXTRACT, *REMAINING_ENTRIES = TEST_ZIP.entry_names.reverse
 
   def setup
@@ -58,7 +59,7 @@ def test_extract_exists_overwrite
     ::Zip::File.open(TEST_ZIP.zip_name) do |zf|
       zf.extract(zf.entries.first, EXTRACTED_FILENAME) do |entry, extract_loc|
         called_correctly = zf.entries.first == entry &&
-                           extract_loc == EXTRACTED_FILENAME
+                           extract_loc == EXTRACTED_FILENAME_ABS
         true
       end
     end

test/file_options_test.rb

@@ -7,12 +7,15 @@ class FileOptionsTest < MiniTest::Test
   TXTPATH = ::File.expand_path(::File.join('data', 'file1.txt'), __dir__).freeze
   TXTPATH_600 = ::File.join(Dir.tmpdir, 'file1.600.txt').freeze
   TXTPATH_755 = ::File.join(Dir.tmpdir, 'file1.755.txt').freeze
-  EXTPATH_1 = ::File.join(Dir.tmpdir, 'extracted_1.txt').freeze
-  EXTPATH_2 = ::File.join(Dir.tmpdir, 'extracted_2.txt').freeze
-  EXTPATH_3 = ::File.join(Dir.tmpdir, 'extracted_3.txt').freeze
   ENTRY_1 = 'entry_1.txt'
   ENTRY_2 = 'entry_2.txt'
   ENTRY_3 = 'entry_3.txt'
+  EXTRACT_1 = 'extracted_1.txt'
+  EXTRACT_2 = 'extracted_2.txt'
+  EXTRACT_3 = 'extracted_3.txt'
+  EXTPATH_1 = ::File.join(Dir.tmpdir, EXTRACT_1).freeze
+  EXTPATH_2 = ::File.join(Dir.tmpdir, EXTRACT_2).freeze
+  EXTPATH_3 = ::File.join(Dir.tmpdir, EXTRACT_3).freeze
 
   def teardown
     ::File.unlink(ZIPPATH) if ::File.exist?(ZIPPATH)
@@ -37,9 +40,9 @@ def test_restore_permissions_true
     end
 
     ::Zip::File.open(ZIPPATH, restore_permissions: true) do |zip|
-      zip.extract(ENTRY_1, EXTPATH_1)
-      zip.extract(ENTRY_2, EXTPATH_2)
-      zip.extract(ENTRY_3, EXTPATH_3)
+      zip.extract(ENTRY_1, EXTRACT_1, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_2, EXTRACT_2, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_3, EXTRACT_3, destination_directory: Dir.tmpdir)
     end
 
     assert_equal(::File.stat(TXTPATH).mode, ::File.stat(EXTPATH_1).mode)
@@ -61,9 +64,9 @@ def test_restore_permissions_false
     end
 
     ::Zip::File.open(ZIPPATH, restore_permissions: false) do |zip|
-      zip.extract(ENTRY_1, EXTPATH_1)
-      zip.extract(ENTRY_2, EXTPATH_2)
-      zip.extract(ENTRY_3, EXTPATH_3)
+      zip.extract(ENTRY_1, EXTRACT_1, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_2, EXTRACT_2, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_3, EXTRACT_3, destination_directory: Dir.tmpdir)
     end
 
     default_perms = (Zip::RUNNING_ON_WINDOWS ? 0o100_644 : 0o100_666) - ::File.umask
@@ -86,9 +89,9 @@ def test_restore_permissions_as_default
     end
 
     ::Zip::File.open(ZIPPATH) do |zip|
-      zip.extract(ENTRY_1, EXTPATH_1)
-      zip.extract(ENTRY_2, EXTPATH_2)
-      zip.extract(ENTRY_3, EXTPATH_3)
+      zip.extract(ENTRY_1, EXTRACT_1, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_2, EXTRACT_2, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_3, EXTRACT_3, destination_directory: Dir.tmpdir)
     end
 
     assert_equal(::File.stat(TXTPATH).mode, ::File.stat(EXTPATH_1).mode)
@@ -103,8 +106,8 @@ def test_restore_times_true
     end
 
     ::Zip::File.open(ZIPPATH, restore_times: true) do |zip|
-      zip.extract(ENTRY_1, EXTPATH_1)
-      zip.extract(ENTRY_2, EXTPATH_2)
+      zip.extract(ENTRY_1, EXTRACT_1, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_2, EXTRACT_2, destination_directory: Dir.tmpdir)
     end
 
     assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_1))
@@ -118,8 +121,8 @@ def test_restore_times_false
     end
 
     ::Zip::File.open(ZIPPATH, restore_times: false) do |zip|
-      zip.extract(ENTRY_1, EXTPATH_1)
-      zip.extract(ENTRY_2, EXTPATH_2)
+      zip.extract(ENTRY_1, EXTRACT_1, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_2, EXTRACT_2, destination_directory: Dir.tmpdir)
     end
 
     assert_time_equal(::Time.now, ::File.mtime(EXTPATH_1))
@@ -133,8 +136,8 @@ def test_restore_times_true_as_default
     end
 
     ::Zip::File.open(ZIPPATH) do |zip|
-      zip.extract(ENTRY_1, EXTPATH_1)
-      zip.extract(ENTRY_2, EXTPATH_2)
+      zip.extract(ENTRY_1, EXTRACT_1, destination_directory: Dir.tmpdir)
+      zip.extract(ENTRY_2, EXTRACT_2, destination_directory: Dir.tmpdir)
     end
 
     assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_1))
@@ -143,20 +146,19 @@ def test_restore_times_true_as_default
 
   def test_get_find_consistency
     testzip = ::File.expand_path(::File.join('data', 'globTest.zip'), __dir__)
-    file_f = ::File.expand_path('f_test.txt', Dir.tmpdir)
-    file_g = ::File.expand_path('g_test.txt', Dir.tmpdir)
-
-    ::Zip::File.open(testzip) do |zip|
-      e1 = zip.find_entry('globTest/food.txt')
-      e1.extract(file_f)
-      e2 = zip.get_entry('globTest/food.txt')
-      e2.extract(file_g)
+    Dir.mktmpdir do |tmp|
+      file_f = ::File.expand_path('f_test.txt', tmp)
+      file_g = ::File.expand_path('g_test.txt', tmp)
+
+      ::Zip::File.open(testzip) do |zip|
+        e1 = zip.find_entry('globTest/food.txt')
+        e1.extract('f_test.txt', destination_directory: tmp)
+        e2 = zip.get_entry('globTest/food.txt')
+        e2.extract('g_test.txt', destination_directory: tmp)
+      end
+
+      assert_time_equal(::File.mtime(file_f), ::File.mtime(file_g))
     end
-
-    assert_time_equal(::File.mtime(file_f), ::File.mtime(file_g))
-  ensure
-    ::File.unlink(file_f)
-    ::File.unlink(file_g)
   end
 
   private

test/path_traversal_test.rb

@@ -39,23 +39,31 @@ def in_tmpdir
   end
 
   def test_leading_slash
-    entries = { '/tmp/moo' => /WARNING: skipped '\/tmp\/moo'/ }
-    in_tmpdir do
+    entries = { '/tmp/moo' => '' }
+    in_tmpdir do |test_path|
+      Dir.mkdir('tmp') # Create 'tmp' dir within test directory.
       extract_paths(['jwilk', 'absolute1.zip'], entries)
+
+      # Check that only the relative file is created.
       refute File.exist?('/tmp/moo')
+      assert File.exist?(File.join(test_path, 'tmp', 'moo'))
     end
   end
 
   def test_multiple_leading_slashes
-    entries = { '//tmp/moo' => /WARNING: skipped '\/\/tmp\/moo'/ }
-    in_tmpdir do
+    entries = { '//tmp/moo' => '' }
+    in_tmpdir do |test_path|
+      Dir.mkdir('tmp') # Create 'tmp' dir within test directory.
       extract_paths(['jwilk', 'absolute2.zip'], entries)
+
+      # Check that only the relative file is created.
       refute File.exist?('/tmp/moo')
+      assert File.exist?(File.join(test_path, 'tmp', 'moo'))
     end
   end
 
   def test_leading_dot_dot
-    entries = { '../moo' => /WARNING: skipped '\.\.\/moo'/ }
+    entries = { '../moo' => /WARNING: skipped extracting '\.\.\/moo'/ }
     in_tmpdir do
       extract_paths(['jwilk', 'relative0.zip'], entries)
       refute File.exist?('../moo')
@@ -65,7 +73,7 @@ def test_leading_dot_dot
   def test_non_leading_dot_dot_with_existing_folder
     entries = {
       'tmp/'          => '',
-      'tmp/../../moo' => /WARNING: skipped 'tmp\/\.\.\/\.\.\/moo'/
+      'tmp/../../moo' => /WARNING: skipped extracting 'tmp\/\.\.\/\.\.\/moo'/
     }
     in_tmpdir do
       extract_paths('relative1.zip', entries)
@@ -75,7 +83,7 @@ def test_non_leading_dot_dot_with_existing_folder
   end
 
   def test_non_leading_dot_dot_without_existing_folder
-    entries = { 'tmp/../../moo' => /WARNING: skipped 'tmp\/\.\.\/\.\.\/moo'/ }
+    entries = { 'tmp/../../moo' => /WARNING: skipped extracting 'tmp\/\.\.\/\.\.\/moo'/ }
     in_tmpdir do
       extract_paths(['jwilk', 'relative2.zip'], entries)
       refute File.exist?('../moo')
@@ -94,7 +102,7 @@ def test_file_symlink
   def test_directory_symlink
     # Can't create tmp/moo, because the tmp symlink is skipped.
     entries = {
-      'tmp'     => /WARNING: skipped symlink 'tmp'/,
+      'tmp'     => /WARNING: skipped symlink '.*\/tmp'/,
       'tmp/moo' => :error
     }
     in_tmpdir do
@@ -106,8 +114,8 @@ def test_directory_symlink
   def test_two_directory_symlinks_a
     # Can't create par/moo because the symlinks are skipped.
     entries = {
-      'cur'     => /WARNING: skipped symlink 'cur'/,
-      'par'     => /WARNING: skipped symlink 'par'/,
+      'cur'     => /WARNING: skipped symlink '.*\/cur'/,
+      'par'     => /WARNING: skipped symlink '.*\/par'/,
       'par/moo' => :error
     }
     in_tmpdir do
@@ -121,8 +129,8 @@ def test_two_directory_symlinks_a
   def test_two_directory_symlinks_b
     # Can't create par/moo, because the symlinks are skipped.
     entries = {
-      'cur'     => /WARNING: skipped symlink 'cur'/,
-      'cur/par' => /WARNING: skipped symlink 'cur\/par'/,
+      'cur'     => /WARNING: skipped symlink '.*\/cur'/,
+      'cur/par' => /WARNING: skipped symlink '.*\/cur\/par'/,
       'par/moo' => :error
     }
     in_tmpdir do
@@ -132,14 +140,29 @@ def test_two_directory_symlinks_b
     end
   end
 
-  def test_entry_name_with_absolute_path_does_not_extract
-    entries = {
-      '/tmp/'         => /WARNING: skipped '\/tmp\/'/,
-      '/tmp/file.txt' => /WARNING: skipped '\/tmp\/file.txt'/
-    }
-    in_tmpdir do
-      extract_paths(['tuzovakaoff', 'absolutepath.zip'], entries)
+  def test_entry_name_with_absolute_path_does_not_extract_by_accident
+    in_tmpdir do |test_path|
+      zip_path = File.join(TEST_FILE_ROOT, 'tuzovakaoff', 'absolutepath.zip')
+      Zip::File.open(zip_path) do |zip_file|
+        zip_file.each do |entry|
+          entry.extract(entry.name, destination_directory: nil)
+          assert File.exist?(File.join(test_path, entry.name))
+          refute File.exist?(entry.name) unless entry.name == '/tmp/'
+        end
+      end
+    end
+  end
+
+  def test_entry_name_with_absolute_path_extracts_to_cwd_by_default
+    in_tmpdir do |test_path|
+      zip_path = File.join(TEST_FILE_ROOT, 'tuzovakaoff', 'absolutepath.zip')
+      Zip::File.open(zip_path) do |zip_file|
+        zip_file.each(&:extract)
+      end
+
+      # Check that only the relative file is created.
       refute File.exist?('/tmp/file.txt')
+      assert File.exist?(File.join(test_path, 'tmp', 'file.txt'))
     end
   end
 
@@ -148,17 +171,20 @@ def test_entry_name_with_absolute_path_extract_when_given_different_path
       zip_path = File.join(TEST_FILE_ROOT, 'tuzovakaoff', 'absolutepath.zip')
       Zip::File.open(zip_path) do |zip_file|
         zip_file.each do |entry|
-          entry.extract(File.join(test_path, entry.name))
+          entry.extract(destination_directory: test_path)
         end
       end
+
+      # Check that only the relative file is created.
       refute File.exist?('/tmp/file.txt')
+      assert File.exist?(File.join(test_path, 'tmp', 'file.txt'))
     end
   end
 
   def test_entry_name_with_relative_symlink
     # Doesn't create the symlink path, so can't create path/file.txt.
     entries = {
-      'path'          => /WARNING: skipped symlink 'path'/,
+      'path'          => /WARNING: skipped symlink '.*\/path'/,
       'path/file.txt' => :error
     }
     in_tmpdir do