Merge pull request rubyzip#376 from jdleesmiller/fix-cve-2018-1000544

Fix CVE-2018-1000544 and disable symlinks to avoid other security issues

.travis.yml

@@ -24,6 +24,7 @@ matrix:
     - rvm: ruby-head
     - rvm: rbx-3
     - rvm: jruby-head
+    - rvm: jruby
 before_install:
   - gem update --system
   - gem install bundler

lib/zip/entry.rb

@@ -109,6 +109,17 @@ def name_is_directory? #:nodoc:all
       @name.end_with?('/')
     end
 
+    # Is the name a relative path, free of `..` patterns that could lead to
+    # path traversal attacks? This does NOT handle symlinks; if the path
+    # contains symlinks, this check is NOT enough to guarantee safety.
+    def name_safe?
+      cleanpath = Pathname.new(@name).cleanpath
+      return false unless cleanpath.relative?
+      root = ::File::SEPARATOR
+      naive_expanded_path = ::File.join(root, cleanpath.to_s)
+      cleanpath.expand_path(root).to_s == naive_expanded_path
+    end
+
     def local_entry_offset #:nodoc:all
       local_header_offset + @local_header_size
     end
@@ -147,14 +158,17 @@ def next_header_offset #:nodoc:all
     end
 
     # Extracts entry to file dest_path (defaults to @name).
-    def extract(dest_path = @name, &block)
-      block ||= proc { ::Zip.on_exists_proc }
-
-      if @name.squeeze('/') =~ /\.{2}(?:\/|\z)/
-        puts "WARNING: skipped \"../\" path component(s) in #{@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?
+        puts "WARNING: skipped #{@name} as unsafe"
         return self
       end
 
+      dest_path ||= @name
+      block ||= proc { ::Zip.on_exists_proc }
+
       if directory? || file? || symlink?
         __send__("create_#{@ftype}", dest_path, &block)
       else
@@ -613,32 +627,9 @@ def create_directory(dest_path)
 
     # BUG: create_symlink() does not use &block
     def create_symlink(dest_path)
-      stat = nil
-      begin
-        stat = ::File.lstat(dest_path)
-      rescue Errno::ENOENT
-      end
-
-      io     = get_input_stream
-      linkto = io.read
-
-      if stat
-        if stat.symlink?
-          if ::File.readlink(dest_path) == linkto
-            return
-          else
-            raise ::Zip::DestinationFileExistsError,
-                  "Cannot create symlink '#{dest_path}'. " \
-                      'A symlink already exists with that name'
-          end
-        else
-          raise ::Zip::DestinationFileExistsError,
-                "Cannot create symlink '#{dest_path}'. " \
-                    'A file already exists with that name'
-        end
-      end
-
-      ::File.symlink(linkto, dest_path)
+      # TODO: Symlinks pose security challenges. Symlink support temporarily
+      # removed in view of https://github.com/rubyzip/rubyzip/issues/369 .
+      puts "WARNING: skipped symlink #{dest_path}"
     end
 
     # apply missing data from the zip64 extra information field, if present

lib/zip/version.rb

@@ -1,3 +1,3 @@
 module Zip
-  VERSION = '1.2.1'
+  VERSION = '1.2.2'
 end

test/data/path_traversal/Makefile

@@ -0,0 +1,10 @@
+# Based on 'relative2' in https://github.com/jwilk/path-traversal-samples,
+# but create the local `tmp` folder before adding the symlink. Otherwise
+# we may bail out before we get to trying to create the file.
+all: relative1.zip
+relative1.zip:
+	rm -f $(@)
+	mkdir -p -m 755 tmp/tmp
+	umask 022 && echo moo > moo
+	cd tmp && zip -X ../$(@) tmp tmp/../../moo
+	rm -rf tmp moo

test/data/path_traversal/jwilk/README.md

@@ -0,0 +1,5 @@
+# Path Traversal Samples
+
+Copied from https://github.com/jwilk/path-traversal-samples on 2018-08-26.
+
+License: MIT

test/data/path_traversal/tuzovakaoff/README.md

@@ -0,0 +1,3 @@
+# Path Traversal Samples
+
+Copied from https://github.com/tuzovakaoff/zip_path_traversal on 2018-08-25.

test/path_traversal_test.rb

@@ -0,0 +1,134 @@
+class PathTraversalTest < MiniTest::Test
+  TEST_FILE_ROOT = File.absolute_path('test/data/path_traversal')
+
+  def setup
+    # With apologies to anyone using these files... but they are the files in
+    # the sample zips, so we don't have much choice here.
+    FileUtils.rm_f '/tmp/moo'
+    FileUtils.rm_f '/tmp/file.txt'
+  end
+
+  def extract_path_traversal_zip(name)
+    Zip::File.open(File.join(TEST_FILE_ROOT, name)) do |zip_file|
+      zip_file.each do |entry|
+        entry.extract
+      end
+    end
+  end
+
+  def in_tmpdir
+    Dir.mktmpdir do |tmp|
+      test_path = File.join(tmp, 'test')
+      Dir.mkdir test_path
+      Dir.chdir test_path do
+        yield test_path
+      end
+    end
+  end
+
+  def test_leading_slash
+    in_tmpdir do
+      extract_path_traversal_zip 'jwilk/absolute1.zip'
+      refute File.exist?('/tmp/moo')
+    end
+  end
+
+  def test_multiple_leading_slashes
+    in_tmpdir do
+      extract_path_traversal_zip 'jwilk/absolute2.zip'
+      refute File.exist?('/tmp/moo')
+    end
+  end
+
+  def test_leading_dot_dot
+    in_tmpdir do
+      extract_path_traversal_zip 'jwilk/relative0.zip'
+      refute File.exist?('../moo')
+    end
+  end
+
+  def test_non_leading_dot_dot_with_existing_folder
+    in_tmpdir do
+      extract_path_traversal_zip 'relative1.zip'
+      assert Dir.exist?('tmp')
+      refute File.exist?('../moo')
+    end
+  end
+
+  def test_non_leading_dot_dot_without_existing_folder
+    in_tmpdir do
+      extract_path_traversal_zip 'jwilk/relative2.zip'
+      refute File.exist?('../moo')
+    end
+  end
+
+  def test_file_symlink
+    in_tmpdir do
+      extract_path_traversal_zip 'jwilk/symlink.zip'
+      assert File.exist?('moo')
+      refute File.exist?('/tmp/moo')
+    end
+  end
+
+  def test_directory_symlink
+    in_tmpdir do
+      # Can't create tmp/moo, because the tmp symlink is skipped.
+      assert_raises Errno::ENOENT do
+        extract_path_traversal_zip 'jwilk/dirsymlink.zip'
+      end
+      refute File.exist?('/tmp/moo')
+    end
+  end
+
+  def test_two_directory_symlinks_a
+    in_tmpdir do
+      # Can't create par/moo because the symlinks are skipped.
+      assert_raises Errno::ENOENT do
+        extract_path_traversal_zip 'jwilk/dirsymlink2a.zip'
+      end
+      refute File.exist?('cur')
+      refute File.exist?('par')
+      refute File.exist?('par/moo')
+    end
+  end
+
+  def test_two_directory_symlinks_b
+    in_tmpdir do
+      # Can't create par/moo, because the symlinks are skipped.
+      assert_raises Errno::ENOENT do
+        extract_path_traversal_zip 'jwilk/dirsymlink2b.zip'
+      end
+      refute File.exist?('cur')
+      refute File.exist?('../moo')
+    end
+  end
+
+  def test_entry_name_with_absolute_path_does_not_extract
+    in_tmpdir do
+      extract_path_traversal_zip 'tuzovakaoff/absolutepath.zip'
+      refute File.exist?('/tmp/file.txt')
+    end
+  end
+
+  def test_entry_name_with_absolute_path_extract_when_given_different_path
+    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(File.join(test_path, entry.name))
+        end
+      end
+      refute File.exist?('/tmp/file.txt')
+    end
+  end
+
+  def test_entry_name_with_relative_symlink
+    in_tmpdir do
+      # Doesn't create the symlink path, so can't create path/file.txt.
+      assert_raises Errno::ENOENT do
+        extract_path_traversal_zip 'tuzovakaoff/symlink.zip'
+      end
+      refute File.exist?('/tmp/file.txt')
+    end
+  end
+end