Prevent unnecessary Zip64 data being stored.

With Zip64 write support enabled by default, it's important that we
only store the extra data when we need to. This commit ensures that
the Zip64 extra data is included for an entry if its size is over
4GB, or if we don't know how big it will be at the point of writing
the local header data.

This commit also removes the need for the Zip64Placeholder extra
data field. Now we just use the Zip64 field itself and ensure it's
filled in correctly.

README.md

@@ -342,7 +342,7 @@ By default, Zip64 support is enabled for writing. To disable it do this:
 Zip.write_zip64_support = false
 ```
 
-_NOTE_: If you will enable Zip64 writing then you will need zip extractor with Zip64 support to extract archive.
+_NOTE_: If Zip64 write support is enabled then any extractor subsequently used may also require Zip64 support to read from the resultant archive.
 
 ### Block Form
 

lib/zip/entry.rb

@@ -21,13 +21,15 @@ class Entry
 
     attr_accessor :comment, :compressed_size, :follow_symlinks, :name,
                   :restore_ownership, :restore_permissions, :restore_times,
-                  :size, :unix_gid, :unix_perms, :unix_uid
+                  :unix_gid, :unix_perms, :unix_uid
 
     attr_accessor :crc, :external_file_attributes, :fstype, :gp_flags,
                   :internal_file_attributes, :local_header_offset # :nodoc:
 
     attr_reader :extra, :compression_level, :filepath # :nodoc:
 
+    attr_writer :size # :nodoc:
+
     mark_dirty :comment=, :compressed_size=, :external_file_attributes=,
                :fstype=, :gp_flags=, :name=, :size=,
                :unix_gid=, :unix_perms=, :unix_uid=
@@ -67,7 +69,7 @@ def check_name(name)
 
     def initialize(
       zipfile = '', name = '',
-      comment: '', size: 0, compressed_size: 0, crc: 0,
+      comment: '', size: nil, compressed_size: 0, crc: 0,
       compression_method: DEFLATED,
       compression_level: ::Zip.default_compression,
       time: ::Zip::DOSTime.now, extra: ::Zip::ExtraField.new
@@ -85,7 +87,7 @@ def initialize(
       @compression_level  = compression_level || ::Zip.default_compression
       @compressed_size    = compressed_size || 0
       @crc                = crc || 0
-      @size               = size || 0
+      @size               = size
       @time               = case time
                             when ::Zip::DOSTime
                               time
@@ -108,6 +110,10 @@ def incomplete?
       gp_flags & 8 == 8
     end
 
+    def size
+      @size || 0
+    end
+
     def time(component: :mtime)
       time =
         if @extra['UniversalTime']
@@ -355,13 +361,13 @@ def pack_local_entry
        @time.to_binary_dos_date, # @last_mod_date
        @crc,
        zip64 && zip64.compressed_size ? 0xFFFFFFFF : @compressed_size,
-       zip64 && zip64.original_size ? 0xFFFFFFFF : @size,
+       zip64 && zip64.original_size ? 0xFFFFFFFF : (@size || 0),
        name_size,
        @extra ? @extra.local_size : 0].pack('VvvvvvVVVvv')
     end
 
     def write_local_entry(io, rewrite: false) #:nodoc:all
-      prep_zip64_extra(true)
+      prep_local_zip64_extra
       verify_local_header_size! if rewrite
       @local_header_offset = io.tell
 
@@ -531,7 +537,7 @@ def pack_c_dir_entry
         @time.to_binary_dos_date, # @last_mod_date
         @crc,
         zip64 && zip64.compressed_size ? 0xFFFFFFFF : @compressed_size,
-        zip64 && zip64.original_size ? 0xFFFFFFFF : @size,
+        zip64 && zip64.original_size ? 0xFFFFFFFF : (@size || 0),
         name_size,
         @extra ? @extra.c_dir_size : 0,
         comment_size,
@@ -546,7 +552,8 @@ def pack_c_dir_entry
     end
 
     def write_c_dir_entry(io) #:nodoc:all
-      prep_zip64_extra(false)
+      prep_cdir_zip64_extra
+
       case @fstype
       when ::Zip::FSTYPE_UNIX
         ft = case ftype
@@ -645,6 +652,7 @@ def gather_fileinfo_from_srcpath(src_path) # :nodoc:
                end
 
       @filepath = src_path
+      @size = stat.size
       get_extra_attributes_from_path(@filepath)
     end
 
@@ -769,39 +777,38 @@ def set_compression_level_flags
       end
     end
 
-    # create a zip64 extra information field if we need one
-    def prep_zip64_extra(for_local_header) #:nodoc:all
+    # rubocop:disable Style/GuardClause
+    def prep_local_zip64_extra
       return unless ::Zip.write_zip64_support
+      return if (!zip64? && @size && @size < 0xFFFFFFFF) || !file?
 
-      need_zip64 = @size >= 0xFFFFFFFF || @compressed_size >= 0xFFFFFFFF
-      need_zip64 ||= @local_header_offset >= 0xFFFFFFFF unless for_local_header
-      if need_zip64
+      # Might not know size here, so need ZIP64 just in case.
+      # If we already have a ZIP64 extra (placeholder) then we must fill it in.
+      if zip64? || @size.nil? || @size >= 0xFFFFFFFF || @compressed_size >= 0xFFFFFFFF
         @version_needed_to_extract = VERSION_NEEDED_TO_EXTRACT_ZIP64
-        @extra.delete('Zip64Placeholder')
-        zip64 = @extra.create('Zip64')
-        if for_local_header
-          # local header always includes size and compressed size
-          zip64.original_size = @size
-          zip64.compressed_size = @compressed_size
-        else
-          # central directory entry entries include whichever fields are necessary
-          zip64.original_size = @size if @size >= 0xFFFFFFFF
-          zip64.compressed_size = @compressed_size if @compressed_size >= 0xFFFFFFFF
-          zip64.relative_header_offset = @local_header_offset if @local_header_offset >= 0xFFFFFFFF
-        end
-      else
-        @extra.delete('Zip64')
+        zip64 = @extra['Zip64'] || @extra.create('Zip64')
 
-        # if this is a local header entry, create a placeholder
-        # so we have room to write a zip64 extra field afterward
-        # (we won't know if it's needed until the file data is written)
-        if for_local_header
-          @extra.create('Zip64Placeholder')
-        else
-          @extra.delete('Zip64Placeholder')
-        end
+        # Local header always includes size and compressed size.
+        zip64.original_size = @size || 0
+        zip64.compressed_size = @compressed_size
       end
     end
+
+    def prep_cdir_zip64_extra
+      return unless ::Zip.write_zip64_support
+
+      if (@size && @size >= 0xFFFFFFFF) || @compressed_size >= 0xFFFFFFFF ||
+         @local_header_offset >= 0xFFFFFFFF
+        @version_needed_to_extract = VERSION_NEEDED_TO_EXTRACT_ZIP64
+        zip64 = @extra['Zip64'] || @extra.create('Zip64')
+
+        # Central directory entry entries include whichever fields are necessary.
+        zip64.original_size = @size if @size && @size >= 0xFFFFFFFF
+        zip64.compressed_size = @compressed_size if @compressed_size >= 0xFFFFFFFF
+        zip64.relative_header_offset = @local_header_offset if @local_header_offset >= 0xFFFFFFFF
+      end
+    end
+    # rubocop:enable Style/GuardClause
   end
 end
 

lib/zip/extra_field.rb

@@ -90,7 +90,6 @@ def local_size
 require 'zip/extra_field/old_unix'
 require 'zip/extra_field/unix'
 require 'zip/extra_field/zip64'
-require 'zip/extra_field/zip64_placeholder'
 require 'zip/extra_field/ntfs'
 
 # Copyright (C) 2002, 2003 Thomas Sondergaard

test/central_directory_test.rb

@@ -88,7 +88,6 @@ def test_write_to_stream
   end
 
   def test_write64_to_stream
-    ::Zip.write_zip64_support = true
     entries = [
       ::Zip::Entry.new(
         'file.zip', 'file1-little', comment: 'comment1', size: 200,

test/file_extract_test.rb

@@ -89,6 +89,8 @@ def test_extract_another_non_entry
   end
 
   def test_extract_incorrect_size
+    Zip.write_zip64_support = false
+
     # The uncompressed size fields in the zip file cannot be trusted. This makes
     # it harder for callers to validate the sizes of the files they are
     # extracting, which can lead to denial of service. See also
@@ -148,4 +150,68 @@ def test_extract_incorrect_size
       end
     end
   end
+
+  def test_extract_incorrect_size_zip64
+    # The uncompressed size fields in the zip file cannot be trusted. This makes
+    # it harder for callers to validate the sizes of the files they are
+    # extracting, which can lead to denial of service. See also
+    # https://en.wikipedia.org/wiki/Zip_bomb
+    #
+    # This version of the test ensures that fraudulent sizes in the ZIP64
+    # extensions are caught.
+    Dir.mktmpdir do |tmp|
+      real_zip = File.join(tmp, 'real.zip')
+      fake_zip = File.join(tmp, 'fake.zip')
+      file_name = 'a'
+      true_size = 500_000
+      fake_size = 1
+
+      ::Zip::File.open(real_zip, create: true) do |zf|
+        zf.get_output_stream(file_name) do |os|
+          os.write 'a' * true_size
+        end
+      end
+
+      compressed_size = nil
+      ::Zip::File.open(real_zip) do |zf|
+        a_entry = zf.find_entry(file_name)
+        compressed_size = a_entry.compressed_size
+        assert_equal true_size, a_entry.size
+      end
+
+      true_size_bytes = [0x1, 16, true_size, compressed_size].pack('vvQ<Q<')
+      fake_size_bytes = [0x1, 16, fake_size, compressed_size].pack('vvQ<Q<')
+
+      data = File.binread(real_zip)
+      assert data.include?(true_size_bytes)
+      data.gsub! true_size_bytes, fake_size_bytes
+
+      File.open(fake_zip, 'wb') do |file|
+        file.write data
+      end
+
+      Dir.chdir tmp do
+        ::Zip::File.open(fake_zip) do |zf|
+          a_entry = zf.find_entry(file_name)
+          assert_equal fake_size, a_entry.size
+
+          ::Zip.validate_entry_sizes = false
+          assert_output('', /.+'a'.+1B.+/) do
+            a_entry.extract
+          end
+          assert_equal true_size, File.size(file_name)
+          FileUtils.rm file_name
+
+          ::Zip.validate_entry_sizes = true
+          error = assert_raises ::Zip::EntrySizeError do
+            a_entry.extract
+          end
+          assert_equal(
+            "Entry 'a' should be 1B, but is larger when inflated.",
+            error.message
+          )
+        end
+      end
+    end
+  end
 end

test/file_test.rb

@@ -305,6 +305,7 @@ def test_add_different_compression
       zf_read = ::Zip::File.new(name)
       entry = zf_read.entries.first
       assert_equal(File.size(src_file), entry.size)
+      refute(entry.zip64?) # No ZIP64 extra as we know the entry size here.
       AssertEntry.assert_contents(
         src_file, zf_read.get_input_stream(entry.name, &:read)
       )
@@ -338,6 +339,7 @@ def test_add_different_compression_as_default
       zf_read = ::Zip::File.new(name)
       entry = zf_read.entries.first
       assert_equal(File.size(src_file), entry.size)
+      refute(entry.zip64?) # No ZIP64 extra as we know the entry size here.
       AssertEntry.assert_contents(
         src_file, zf_read.get_input_stream(entry.name, &:read)
       )
@@ -367,6 +369,7 @@ def test_add_stored
     assert_equal(File.size(src_file), entry.size)
     assert_equal(entry.size, entry.compressed_size)
     assert_equal(::Zip::Entry::STORED, entry.compression_method)
+    refute(entry.zip64?) # No ZIP64 extra as we know the entry size here.
     AssertEntry.assert_contents(src_file,
                                 zf_read.get_input_stream(entry_name, &:read))
   end
@@ -424,6 +427,7 @@ def test_add_directory
       end
 
       assert(dir_entry.directory?)
+      refute(dir_entry.zip64?) # No ZIP64 extra as we know the entry size here.
     end
   end
 
@@ -457,11 +461,12 @@ def test_mkdir
     end
 
     ::Zip::File.open_buffer(buffer) do |zf|
-      assert(zf.find_entry('dir/').directory?)
-      assert(zf.find_entry('dir').directory?)
+      ['dir/', 'dir', 'folder/', 'folder'].each do |dir|
+        entry = zf.find_entry(dir)
 
-      assert(zf.find_entry('folder/').directory?)
-      assert(zf.find_entry('folder').directory?)
+        assert(entry.directory?)
+        refute(entry.zip64?)
+      end
     end
   end
 
@@ -679,7 +684,6 @@ def test_double_commit(filename = 'test/data/generated/double_commit_test.zip')
   end
 
   def test_double_commit_zip64
-    ::Zip.write_zip64_support = true
     test_double_commit('test/data/generated/double_commit_test64.zip')
   end