Disable symlinks and check for path traversal

lib/zip/entry.rb

@@ -109,6 +109,16 @@ 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?
+      naive_expanded_path = ::File.join(Dir.pwd, cleanpath.to_s)
+      cleanpath.expand_path.to_s == naive_expanded_path
+    end
+
     def local_entry_offset #:nodoc:all
       local_header_offset + @local_header_size
     end
@@ -147,15 +157,11 @@ def next_header_offset #:nodoc:all
     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? && Pathname.new(@name).absolute?
-        puts "WARNING: skipped absolute path in #{@name}"
-        return self
-      elsif @name.squeeze('/') =~ /\.{2}(?:\/|\z)/
-        puts "WARNING: skipped \"../\" path component(s) in #{@name}"
-        return self
-      elsif symlink? && get_input_stream.read =~ %r{../..}
-        puts "WARNING: skipped \"#{get_input_stream.read}\" symlink path in #{@name}"
+      if dest_path.nil? && !name_safe?
+        puts "WARNING: skipped #{@name} as unsafe"
         return self
       end
 
@@ -620,32 +626,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

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/tuzovakaoff/README.md

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

test/path_traversal_test.rb

@@ -47,7 +47,15 @@ def test_leading_dot_dot
     end
   end
 
-  def test_non_leading_dot_dot
+  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')
@@ -64,7 +72,10 @@ def test_file_symlink
 
   def test_directory_symlink
     in_tmpdir do
-      extract_path_traversal_zip 'jwilk/dirsymlink.zip'
+      # 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
@@ -83,9 +94,11 @@ def test_two_directory_symlinks_a
 
   def test_two_directory_symlinks_b
     in_tmpdir do
-      extract_path_traversal_zip 'jwilk/dirsymlink2b.zip'
-      assert File.exist?('cur')
-      assert_equal '.', File.readlink('cur')
+      # 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