From a3cf4748cc4be17c27030728fdd00f79419a2238 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Wed, 5 Feb 2014 16:15:01 -0700 Subject: [PATCH] check size of actual zip contents against quota because the zip central directory can lie test plan: 1. Create a huge file (say a gigabyte) where every byte is the same. 2. Zip it. The repeating data will be compressed. The ZIP will be tiny but will uncompress to a huge file. 3. Edit the zip file binary. Open it with a hex editor and change the file size in the directory. Make the file appear to be small enough to fit into the course quota. (The zip file format is documented at http://www.pkware.com/documents/casestudies/APPNOTE.TXT or ask the committer for help) 4. Try to import the zip file into a course (migrations/ import zip file into folder) 5. You should receive a quota error. fixes CNVS-10722 Change-Id: Ib1bd1c432ef900f0c6c61ebe6eab2881f8515104 Reviewed-on: https://gerrit.instructure.com/29704 Reviewed-by: Mark Severson Tested-by: Jenkins QA-Review: Nathan Rogowski Product-Review: Jeremy Stanley --- lib/unzip_attachment.rb | 39 +++++++++++++++++++++++++++--- spec/fixtures/zipbomb.zip | Bin 0 -> 662 bytes spec/lib/unzip_attachment_spec.rb | 31 ++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 spec/fixtures/zipbomb.zip diff --git a/lib/unzip_attachment.rb b/lib/unzip_attachment.rb index 509cd712452..6bc35ce5ac5 100644 --- a/lib/unzip_attachment.rb +++ b/lib/unzip_attachment.rb @@ -113,7 +113,7 @@ class UnzipAttachment # have to worry about what this name actually is. Tempfile.open(filename) do |f| begin - entry.extract(f.path) { true } + extract_entry(entry, f.path) # This is where the attachment actually happens. See file_in_context.rb attachment = attach(f.path, entry, folder) id_positions[attachment.id] = path_positions[entry.name] @@ -121,6 +121,9 @@ class UnzipAttachment attachment.update_attribute(:migration_id, migration_id) end @attachments << attachment if attachment + rescue Attachment::OverQuotaError + f.unlink + raise rescue => e @logger.warn "Couldn't unzip archived file #{f.path}: #{e.message}" if @logger end @@ -135,6 +138,18 @@ class UnzipAttachment update_progress(1.0) end + def extract_entry(entry, dest_path) + ::File.open(dest_path, "wb") do |os| + entry.get_input_stream do |is| + entry.set_extra_attributes_on_path(dest_path) + buf = '' + while buf = is.sysread(::Zip::Decompressor::CHUNK_SIZE, buf) + os << buf + zip_stats.charge_quota(buf.size) + end + end + end + end def zip_stats @zip_stats ||= ZipFileStats.new(filename) @@ -246,13 +261,14 @@ end #since it's such an integral part of the unzipping #process class ZipFileStats - attr_reader :file_count, :total_size, :paths, :filename + attr_reader :file_count, :total_size, :paths, :filename, :quota_remaining def initialize(filename) @filename = filename @paths = [] @file_count = 0 @total_size = 0 + @quota_remaining = nil process! end @@ -262,12 +278,27 @@ class ZipFileStats raise ArgumentError, "Zip File cannot have more than #{max} entries" end + # check whether the nominal size of the zip's contents would exceed + # quota, and reject the zip immediately if so quota_hash = Attachment.get_quota(context) - if quota_hash[:quota] > 0 && (quota_hash[:quota_used] + total_size) > quota_hash[:quota] - raise Attachment::OverQuotaError, "Zip file would exceed quota limit" + if quota_hash[:quota] > 0 + if (quota_hash[:quota_used] + total_size) > quota_hash[:quota] + raise Attachment::OverQuotaError, "Zip file would exceed quota limit" + end + @quota_remaining = quota_hash[:quota] - quota_hash[:quota_used] end end + # since the central directory can lie, track quota during extraction as well + # to prevent zip bomb denial-of-service attacks + def charge_quota(size) + return if @quota_remaining.nil? + if size > @quota_remaining + raise Attachment::OverQuotaError, "Zip contents exceed course quota limit" + end + @quota_remaining -= size + end + def paths_with_positions(base) positions_hash = {} paths.sort.each_with_index{|p, idx| positions_hash[p] = idx + base } diff --git a/spec/fixtures/zipbomb.zip b/spec/fixtures/zipbomb.zip new file mode 100644 index 0000000000000000000000000000000000000000..5c834de4cebe3556dcf74c1b08de3c83ebf5b86f GIT binary patch literal 662 zcmWIWW@h1H0D;T~SC>e2ePJFT8-y7dWEc`dLpT|jO%HzxDhA@x3T_5QmT!y<3@jo* z1&InJnR%58X_+~xTmf)na)8D(W!+&G0;&LEkTFSMV{C93lb2tTky@lsoST@FqmZ8m zF$`n@2g5d?VQljreRT)&zNteDO9mTu9B3E_qq*tr$&H*14m=D8?rCnC<#gmgvuo1h zFY6KuuiuNgT6pbL#8lbeh4#Xxva$Jj$!oV>i^|3%Zry*wj_^(?&;_ep?}`WmT>!!$ z@1%fT;ETgMzyQfh%z=6b8aRwha?H3wSpsM($Xo`7C5<2!Fw9vYVUFgv0B?jbn4yep z3@EsOVGc9~6y{is!315yYI2nGQAjhQh3 literal 0 HcmV?d00001 diff --git a/spec/lib/unzip_attachment_spec.rb b/spec/lib/unzip_attachment_spec.rb index 6f3a9c4b7fd..60af7a67fb8 100644 --- a/spec/lib/unzip_attachment_spec.rb +++ b/spec/lib/unzip_attachment_spec.rb @@ -160,6 +160,37 @@ describe UnzipAttachment do end end + describe 'zip bomb mitigation' do + # unzip -l output for this file: + # Length Date Time Name + # -------- ---- ---- ---- + # 12 02-05-14 16:03 a + # 18 02-05-14 16:03 b + # 70 02-05-14 16:05 c <-- this is a lie. the file is really 10K + # 19 02-05-14 16:03 d + let(:filename) { fixture_filename('zipbomb.zip') } + + it 'double-checks the extracted file sizes in case the central directory lies' do + Attachment.stubs(:get_quota).returns({:quota => 5000, :quota_used => 0}) + lambda{ unzipper.process }.should raise_error(Attachment::OverQuotaError) + # a and b should have been attached + # but we should have bailed once c ate the remaining quota + @course.attachments.count.should eql 2 + end + + it "doesn't interfere when the quota is 0 (unlimited)" do + Attachment.stubs(:get_quota).returns({:quota => 0, :quota_used => 0}) + lambda{ unzipper.process }.should_not raise_error + @course.attachments.count.should eql 4 + end + + it "lets incorrect central directory size slide if the quota isn't exceeded" do + Attachment.stubs(:get_quota).returns({:quota => 15000, :quota_used => 0}) + lambda{ unzipper.process }.should_not raise_error + @course.attachments.count.should eql 4 + end + end + end context "scribdable files" do