Merge branch 'security-13-update-ruby-zip-pages-master' into 'master'

Bump rubyzip to 2.0.0

Closes #13

See merge request gitlab-org/security/gitlab!41

Gemfile

@@ -65,7 +65,7 @@ gem 'u2f', '~> 0.2.1'
 
 # GitLab Pages
 gem 'validates_hostname', '~> 1.0.6'
-gem 'rubyzip', '~> 1.3.0', require: 'zip'
+gem 'rubyzip', '~> 2.0.0', require: 'zip'
 # GitLab Pages letsencrypt support
 gem 'acme-client', '~> 2.0.5'
 

Gemfile.lock

@@ -942,7 +942,7 @@ GEM
       sexp_processor (~> 4.9)
     rubyntlm (0.6.2)
     rubypants (0.2.0)
-    rubyzip (1.3.0)
+    rubyzip (2.0.0)
     rugged (0.28.4.1)
     safe_yaml (1.0.4)
     sanitize (4.6.6)
@@ -1340,7 +1340,7 @@ DEPENDENCIES
   ruby-prof (~> 1.0.0)
   ruby-progressbar
   ruby_parser (~> 3.8)
-  rubyzip (~> 1.3.0)
+  rubyzip (~> 2.0.0)
   rugged (~> 0.28)
   sanitize (~> 4.6)
   sassc-rails (~> 2.1.0)

changelogs/unreleased/security-13-update-ruby-zip-pages-master.yml

+---
+title: Bump rubyzip to 2.0.0
+merge_request:
+author: Utkarsh Gupta
+type: security

spec/services/projects/update_pages_service_spec.rb

@@ -5,7 +5,7 @@ require "spec_helper"
 describe Projects::UpdatePagesService do
   let_it_be(:project, refind: true) { create(:project, :repository) }
   let_it_be(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) }
-  let_it_be(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
+  let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
   let(:invalid_file) { fixture_file_upload('spec/fixtures/dk.png') }
 
   let(:file) { fixture_file_upload("spec/fixtures/pages.zip") }
@@ -204,6 +204,32 @@ describe Projects::UpdatePagesService do
     end
   end
 
+  context 'when file size is spoofed' do
+    let(:metadata) { spy('metadata') }
+
+    include_context 'pages zip with spoofed size'
+
+    before do
+      file = fixture_file_upload(fake_zip_path, 'pages.zip')
+      metafile = fixture_file_upload('spec/fixtures/pages.zip.meta')
+
+      create(:ci_job_artifact, :archive, file: file, job: build)
+      create(:ci_job_artifact, :metadata, file: metafile, job: build)
+
+      allow(build).to receive(:artifacts_metadata_entry)
+                        .and_return(metadata)
+      allow(metadata).to receive(:total_size).and_return(100)
+    end
+
+    it 'raises an error' do
+      expect do
+        subject.execute
+      end.to raise_error(Projects::UpdatePagesService::FailedToExtractError,
+                         'Entry public/index.html should be 1B but is larger when inflated')
+      expect(deploy_status).to be_script_failure
+    end
+  end
+
   def deploy_status
     GenericCommitStatus.find_by(name: 'pages:deploy')
   end

spec/support/shared_contexts/pages_zip_with_spoofed_size_shared_context.rb

+# frozen_string_literal: true
+
+# the idea of creating zip archive with spoofed size is borrowed from
+# https://github.com/rubyzip/rubyzip/pull/403/files#diff-118213fb4baa6404a40f89e1147661ebR88
+RSpec.shared_context 'pages zip with spoofed size' do
+  let(:real_zip_path) { Tempfile.new(['real', '.zip']).path }
+  let(:fake_zip_path) { Tempfile.new(['fake', '.zip']).path }
+
+  before do
+    full_file_name = 'public/index.html'
+    true_size = 500_000
+    fake_size = 1
+
+    ::Zip::File.open(real_zip_path, ::Zip::File::CREATE) do |zf|
+      zf.get_output_stream(full_file_name) do |os|
+        os.write 'a' * true_size
+      end
+    end
+
+    compressed_size = nil
+    ::Zip::File.open(real_zip_path) do |zf|
+      a_entry = zf.find_entry(full_file_name)
+      compressed_size = a_entry.compressed_size
+    end
+
+    true_size_bytes = [compressed_size, true_size, full_file_name.size].pack('LLS')
+    fake_size_bytes = [compressed_size, fake_size, full_file_name.size].pack('LLS')
+
+    data = File.binread(real_zip_path)
+    data.gsub! true_size_bytes, fake_size_bytes
+
+    File.open(fake_zip_path, 'wb') do |file|
+      file.write data
+    end
+  end
+
+  after do
+    File.delete(real_zip_path) if File.exist?(real_zip_path)
+    File.delete(fake_zip_path) if File.exist?(fake_zip_path)
+  end
+end