Blob Blame Raw
From 4167f0ce67e42b082605bca75c7bdfd01eb23804 Mon Sep 17 00:00:00 2001
From: John Lees-Miller <jdleesmiller@gmail.com>
Date: Thu, 12 Sep 2019 22:01:38 +0100
Subject: [PATCH 1/2] Validate entry sizes when extracting

---
 README.md                 | 67 +++++++++++++++++++++++++++++++--------
 lib/zip.rb                |  4 ++-
 lib/zip/entry.rb          |  7 ++++
 lib/zip/errors.rb         |  1 +
 test/file_extract_test.rb | 62 ++++++++++++++++++++++++++++++++++++
 5 files changed, 127 insertions(+), 14 deletions(-)

diff --git a/README.md b/README.md
index 321b811..a193c24 100644
--- a/README.md
+++ b/README.md
@@ -129,12 +129,15 @@ After this entries in zip archive will be saved in ordered state.
 ### Reading a Zip file
 
 ```ruby
+MAX_SIZE = 1024**2 # 1MiB (but of course you can increase this)
 Zip::File.open('foo.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(dest_file)
+    raise 'File too large when extracted' if entry.size > MAX_SIZE
+
+    # Extract to file or directory based on name in the archive
+    entry.extract
 
     # Read into memory
     content = entry.get_input_stream.read
@@ -142,6 +145,7 @@ Zip::File.open('foo.zip') do |zip_file|
 
   # Find specific entry
   entry = zip_file.glob('*.csv').first
+  raise 'File too large when extracted' if entry.size > MAX_SIZE
   puts entry.get_input_stream.read
 end
 ```
@@ -212,6 +216,29 @@ In some zip date of files stored in incorrect format. You can hide warning about
 Zip.warn_invalid_date = false
 ```
 
+By default, `rubyzip`'s `extract` method checks that an entry's reported uncompressed size is not (significantly) smaller than its actual size. This is to help you protect your application against [zip bombs](https://en.wikipedia.org/wiki/Zip_bomb). Before `extract`ing an entry, you should check that its size is in the range you expect. For example, if your application supports processing up to 100 files at once, each up to 10MiB, your zip extraction code might look like:
+
+```ruby
+MAX_FILE_SIZE = 10 * 1024**2 # 10MiB
+MAX_FILES = 100
+Zip::File.open('foo.zip') do |zip_file|
+  num_files = 0
+  zip_file.each do |entry|
+    num_files += 1 if entry.file?
+    raise 'Too many extracted files' if num_files > MAX_FILES
+    raise 'File too large when extracted' if entry.size > MAX_FILE_SIZE
+    entry.extract
+  end
+end
+```
+
+If you need to extract zip files that report incorrect uncompressed sizes and you really trust them not too be too large, you can disable this setting with
+```ruby
+Zip.validate_entry_sizes = false
+```
+
+Note that if you use the lower level `Zip::InputStream` interface, `rubyzip` does *not* check the entry `size`s. In this case, the caller is responsible for making sure it does not read more data than expected from the input stream.
+
 You can set the default compression level like so:
 
 ```ruby
diff --git a/lib/zip.rb b/lib/zip.rb
index e6ebad3..25d3764 100644
--- a/lib/zip.rb
+++ b/lib/zip.rb
@@ -37,7 +37,7 @@ end
 
 module Zip
   extend self
-  attr_accessor :unicode_names, :on_exists_proc, :continue_on_exists_proc, :sort_entries, :default_compression, :write_zip64_support, :warn_invalid_date
+  attr_accessor :unicode_names, :on_exists_proc, :continue_on_exists_proc, :sort_entries, :default_compression, :write_zip64_support, :warn_invalid_date, :validate_entry_sizes
 
   def reset!
     @_ran_once = false
@@ -48,6 +48,7 @@ module Zip
     @default_compression = ::Zlib::DEFAULT_COMPRESSION
     @write_zip64_support = false
     @warn_invalid_date = true
+    @validate_entry_sizes = true
   end
 
   def setup
diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb
index 80160b5..bd3e4f3 100644
--- a/lib/zip/entry.rb
+++ b/lib/zip/entry.rb
@@ -583,9 +583,16 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi
         get_input_stream do |is|
           set_extra_attributes_on_path(dest_path)
 
+          bytes_written = 0
           buf = ''
           while buf = is.sysread(::Zip::Decompressor::CHUNK_SIZE, buf)
             os << buf
+
+            next unless ::Zip.validate_entry_sizes
+            bytes_written += buf.bytesize
+            if bytes_written > size
+              raise ::Zip::EntrySizeError, "Entry #{name} should be #{size}B but is larger when inflated"
+            end
           end
         end
       end
diff --git a/lib/zip/errors.rb b/lib/zip/errors.rb
index b2bcccd..364c6ee 100644
--- a/lib/zip/errors.rb
+++ b/lib/zip/errors.rb
@@ -4,6 +4,7 @@ class EntryExistsError < Error; end
   class DestinationFileExistsError < Error; end
   class CompressionMethodError < Error; end
   class EntryNameError < Error; end
+  class EntrySizeError < Error; end
   class InternalError < Error; end
 
   # Backwards compatibility with v1 (delete in v2)
diff --git a/test/file_extract_test.rb b/test/file_extract_test.rb
index 57833fc..6103aea 100644
--- a/test/file_extract_test.rb
+++ b/test/file_extract_test.rb
@@ -10,6 +10,10 @@ def setup
     ::File.delete(EXTRACTED_FILENAME) if ::File.exist?(EXTRACTED_FILENAME)
   end
 
+  def teardown
+    ::Zip.reset!
+  end
+
   def test_extract
     ::Zip::File.open(TEST_ZIP.zip_name) {
         |zf|
@@ -87,4 +91,61 @@ def test_extractNonEntry2
     assert(!File.exist?(outFile))
   end
 
+  def test_extract_incorrect_size
+    # 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
+    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, ::Zip::File::CREATE) 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 = [compressed_size, true_size, file_name.size].pack('LLS')
+      fake_size_bytes = [compressed_size, fake_size, file_name.size].pack('LLS')
+
+      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
+          a_entry.extract
+          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

From 97cb6aefe6d12bd2429d7a2e119ccb26f259d71d Mon Sep 17 00:00:00 2001
From: John Lees-Miller <jdleesmiller@gmail.com>
Date: Wed, 18 Sep 2019 18:34:23 +0100
Subject: [PATCH 2/2] Warn when an entry size is invalid

---
 lib/zip/entry.rb | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb
index bd3e4f3..677e49e 100644
--- a/lib/zip/entry.rb
+++ b/lib/zip/entry.rb
@@ -604,14 +604,19 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi
           set_extra_attributes_on_path(dest_path)
 
           bytes_written = 0
+          warned = false
           buf = ''
           while buf = is.sysread(::Zip::Decompressor::CHUNK_SIZE, buf)
             os << buf
-
-            next unless ::Zip.validate_entry_sizes
             bytes_written += buf.bytesize
-            if bytes_written > size
-              raise ::Zip::EntrySizeError, "Entry #{name} should be #{size}B but is larger when inflated"
+            if bytes_written > size && !warned
+              message = "Entry #{name} should be #{size}B but is larger when inflated"
+              if ::Zip.validate_entry_sizes
+                raise ::Zip::EntrySizeError, message
+              else
+                puts "WARNING: #{message}"
+                warned = true
+              end
             end
           end
         end