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 00000000000..5c834de4ceb Binary files /dev/null and b/spec/fixtures/zipbomb.zip differ 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