Repository: rubyzip/rubyzip (ruby)

Issue:
Directory traversal vulnerability
## Summary
In version 2.3.2, we are again experiencing the exact same problem as below.
https://github.com/rubyzip/rubyzip/issues/315

## PoC

```
root@ea8d5cb86e9f:/work# ruby -v
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [aarch64-linux]
root@ea8d5cb86e9f:/work# uname -a
Linux ea8d5cb86e9f 5.15.64-0-virt #1-Alpine SMP Mon, 05 Sep 2022 08:02:49 +0000 aarch64 GNU/Linux
root@ea8d5cb86e9f:/work# gem list | grep rubyzip
rubyzip (2.3.2)
root@ea8d5cb86e9f:/work# zipinfo traversal.zip 
Archive:  traversal.zip
Zip file size: 166 bytes, number of entries: 1
-rw-r--r--  5.2 unx        0 t- defN 22-Nov-15 07:57 ../../.././../../../../tmp/hacked
1 file, 0 bytes uncompressed, 2 bytes compressed:  0.0%
root@ea8d5cb86e9f:/work# ls -l /tmp
total 0
root@ea8d5cb86e9f:/work# ruby extract.rb 
Extracting ../../.././../../../../tmp/hacked
root@ea8d5cb86e9f:/work# ls -l /tmp
total 0
-rw-r--r-- 1 root root 0 Nov 16 00:55 hacked
```

**extract.rb**

```ruby
require 'zip'

Zip::File.open('traversal.zip') do |zip_file|
  # Handle entries one by one
  zip_file.each do |entry|
    # Extract to file/directory/symlink
    puts "Extracting #{entry.name}"
    entry.extract(entry.name)
  end
end
```


Discussion:
Sorry.
I missed reading the following.
https://github.com/rubyzip/rubyzip/blob/v2.3.2/lib/zip/entry.rb#L174
Is there any particular plan to correct this issue?
If not, we will close this Issue and propose a fix in the Security Advisory.
https://github.com/github/advisory-database/pull/832
Hi @wonda-tea-coffee and thanks for raising this again.

The above code clearly short-circuits around the protections that Rubyzip has for traversal. For example the following code would be safe, simply by changing `entry.extract(entry.name)` to `entry.extract`:
```ruby
require 'zip'

Zip::File.open('traversal.zip') do |zip_file|
  # Handle entries one by one
  zip_file.each do |entry|
    # Extract to file/directory/symlink
    puts "Extracting #{entry.name}"
    entry.extract
  end
end
```

A developer would need to specifically write the unsafe version that you supply above, but I agree that it's probably easy to do so without knowing you've done it.

I do think it should be possible for a developer to knowingly and deliberately allow extracting to potentially unsafe names, if for their particular situation it is perfectly safe, but:
* it should not be the default;
* the default should be to not allow this, and the developer should have to specifically allow it themselves; and
* we should never allow extracting to an unsafe place directly to the name provided in zip archive itself

Do you think that would be acceptable?

I will think on how to implement that and get it into version 3 - which is getting closer (honest).
@hainesr 
Thanks for the reply.
I completely agree with the above 👍 
OK, having now tried a somewhat naive implementation based solely on that bulleted list above, I see a rather large UX hole. The above prevents one from extracting to, say, `/tmp` unless you were to completely turn off checks for unsafe paths. Anything starting with a `/` is currently considered unsafe - which it would be if the leading `/` was in the entry name in the zip file, but it should be OK to extract to `/tmp`.

I think the solution then is to add a parameter to the `extract` method which specifies a target directory for the extraction. This would be combined with the entry name and then we'd check that the result was safe. For example:

* `/tmp` joined with `etc/passwd` is safe
* `/tmp` joined with `../etc/passwd` is not safe

This seems to be how other libraries manage this situation: https://github.com/commonsguy/cwac-security/blob/master/security/src/main/java/com/commonsware/cwac/security/ZipUtils.java#L171-L181

Patch:
diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb
--- a/lib/zip/entry.rb
+++ b/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
 
diff --git a/lib/zip/file.rb b/lib/zip/file.rb
--- a/lib/zip/file.rb
+++ b/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 patch:
diff --git a/test/file_extract_directory_test.rb b/test/file_extract_directory_test.rb
--- a/test/file_extract_directory_test.rb
+++ b/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
diff --git a/test/file_extract_test.rb b/test/file_extract_test.rb
--- a/test/file_extract_test.rb
+++ b/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
diff --git a/test/file_options_test.rb b/test/file_options_test.rb
--- a/test/file_options_test.rb
+++ b/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
diff --git a/test/path_traversal_test.rb b/test/path_traversal_test.rb
--- a/test/path_traversal_test.rb
+++ b/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