Add latest changes from gitlab-org/security/gitlab@12-7-stable-ee

GITLAB_WORKHORSE_VERSION

-8.20.1
+8.20.2

changelogs/unreleased/security-fix-issue-213139.yml

+---
+title: Prevent filename bypass on artifact upload
+merge_request:
+author:
+type: security

lib/api/runner.rb

@@ -247,21 +247,14 @@ module API
       end
       params do
         requires :id, type: Integer, desc: %q(Job's ID)
+        requires :file, type: ::API::Validations::Types::WorkhorseFile, desc: %(The artifact file to store (generated by Multipart middleware))
         optional :token, type: String, desc: %q(Job's authentication token)
         optional :expire_in, type: String, desc: %q(Specify when artifacts should expire)
         optional :artifact_type, type: String, desc: %q(The type of artifact),
                                  default: 'archive', values: Ci::JobArtifact.file_types.keys
         optional :artifact_format, type: String, desc: %q(The format of artifact),
                                    default: 'zip', values: Ci::JobArtifact.file_formats.keys
-        optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse))
-        optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse))
-        optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse))
-        optional 'file.size', type: Integer, desc: %q(real size of file (generated by Workhorse))
-        optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file (generated by Workhorse))
-        optional 'metadata.path', type: String, desc: %q(path to locally stored body (generated by Workhorse))
-        optional 'metadata.name', type: String, desc: %q(filename (generated by Workhorse))
-        optional 'metadata.size', type: Integer, desc: %q(real size of metadata (generated by Workhorse))
-        optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of metadata (generated by Workhorse))
+        optional :metadata, type: ::API::Validations::Types::WorkhorseFile, desc: %(The artifact metadata to store (generated by Multipart middleware))
       end
       post '/:id/artifacts' do
         not_allowed! unless Gitlab.config.artifacts.enabled
@@ -270,10 +263,9 @@ module API
         job = authenticate_job!
         forbidden!('Job is not running!') unless job.running?
 
-        artifacts = UploadedFile.from_params(params, :file, JobArtifactUploader.workhorse_local_upload_path)
-        metadata = UploadedFile.from_params(params, :metadata, JobArtifactUploader.workhorse_local_upload_path)
+        artifacts = params[:file]
+        metadata = params[:metadata]
 
-        bad_request!('Missing artifacts file!') unless artifacts
         file_too_large! unless artifacts.size < max_artifacts_size(job)
 
         expire_in = params['expire_in'] ||

lib/gitlab/middleware/multipart.rb

@@ -87,6 +87,7 @@ module Gitlab
           allowed_paths = [
             ::FileUploader.root,
             Gitlab.config.uploads.storage_path,
+            JobArtifactUploader.workhorse_upload_path,
             File.join(Rails.root, 'public/uploads/tmp')
           ]
 
@@ -121,6 +122,8 @@ module Gitlab
         Handler.new(env, message).with_open_files do
           @app.call(env)
         end
+      rescue UploadedFile::InvalidPathError => e
+        [400, { 'Content-Type' => 'text/plain' }, e.message]
       end
     end
   end

spec/lib/gitlab/middleware/multipart_spec.rb

@@ -5,9 +5,7 @@ require 'spec_helper'
 require 'tempfile'
 
 describe Gitlab::Middleware::Multipart do
-  let(:app) { double(:app) }
-  let(:middleware) { described_class.new(app) }
-  let(:original_filename) { 'filename' }
+  include_context 'multipart middleware context'
 
   shared_examples_for 'multipart upload files' do
     it 'opens top-level files' do
@@ -82,22 +80,23 @@ describe Gitlab::Middleware::Multipart do
   end
 
   it 'allows files in uploads/tmp directory' do
-    Dir.mktmpdir do |dir|
-      uploads_dir = File.join(dir, 'public/uploads/tmp')
-      FileUtils.mkdir_p(uploads_dir)
-
-      allow(Rails).to receive(:root).and_return(dir)
-      allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir'))
-
-      Tempfile.open('top-level', uploads_dir) do |tempfile|
-        env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+    with_tmp_dir('public/uploads/tmp') do |dir, env|
+      expect(app).to receive(:call) do |env|
+        expect(get_params(env)['file']).to be_a(::UploadedFile)
+      end
 
-        expect(app).to receive(:call) do |env|
-          expect(get_params(env)['file']).to be_a(::UploadedFile)
-        end
+      middleware.call(env)
+    end
+  end
 
-        middleware.call(env)
+  it 'allows files in the job artifact upload path' do
+    with_tmp_dir('artifacts') do |dir, env|
+      expect(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(File.join(dir, 'artifacts'))
+      expect(app).to receive(:call) do |env|
+        expect(get_params(env)['file']).to be_a(::UploadedFile)
       end
+
+      middleware.call(env)
     end
   end
 
@@ -127,22 +126,4 @@ describe Gitlab::Middleware::Multipart do
       middleware.call(env)
     end
   end
-
-  # Rails 5 doesn't combine the GET/POST parameters in
-  # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set:
-  # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41
-  def get_params(env)
-    req = ActionDispatch::Request.new(env)
-    req.GET.merge(req.POST)
-  end
-
-  def post_env(rewritten_fields, params, secret, issuer)
-    token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256')
-    Rack::MockRequest.env_for(
-      '/',
-      method: 'post',
-      params: params,
-      described_class::RACK_ENV_KEY => token
-    )
-  end
 end

spec/requests/api/runner_spec.rb

@@ -1400,8 +1400,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
 
     describe 'artifacts' do
       let(:job) { create(:ci_build, :pending, pipeline: pipeline, runner_id: runner.id) }
-      let(:jwt_token) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
-      let(:headers) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt_token } }
+      let(:jwt) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') }
+      let(:headers) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt } }
       let(:headers_with_token) { headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.token) }
       let(:file_upload) { fixture_file_upload('spec/fixtures/banana_sample.gif', 'image/gif') }
       let(:file_upload2) { fixture_file_upload('spec/fixtures/dk.png', 'image/gif') }
@@ -1671,12 +1671,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             it 'fails to post artifacts without GitLab-Workhorse' do
               post api("/jobs/#{job.id}/artifacts"), params: { token: job.token }, headers: {}
 
-              expect(response).to have_gitlab_http_status(403)
+              expect(response).to have_gitlab_http_status(:bad_request)
             end
           end
 
           context 'Is missing GitLab Workhorse token headers' do
-            let(:jwt_token) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') }
+            let(:jwt) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') }
 
             it 'fails to post artifacts without GitLab-Workhorse' do
               expect(Gitlab::ErrorTracking).to receive(:track_exception).once
@@ -1690,15 +1690,14 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           context 'when setting an expire date' do
             let(:default_artifacts_expire_in) {}
             let(:post_data) do
-              { 'file.path' => file_upload.path,
-                'file.name' => file_upload.original_filename,
-                'expire_in' => expire_in }
+              { file: file_upload,
+                expire_in: expire_in }
             end
 
             before do
               stub_application_setting(default_artifacts_expire_in: default_artifacts_expire_in)
 
-              post(api("/jobs/#{job.id}/artifacts"), params: post_data, headers: headers_with_token)
+              upload_artifacts(file_upload, headers_with_token, post_data)
             end
 
             context 'when an expire_in is given' do
@@ -1751,20 +1750,22 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             let(:stored_artifacts_size) { job.reload.artifacts_size }
             let(:stored_artifacts_sha256) { job.reload.job_artifacts_archive.file_sha256 }
             let(:stored_metadata_sha256) { job.reload.job_artifacts_metadata.file_sha256 }
+            let(:file_keys) { post_data.keys }
+            let(:send_rewritten_field) { true }
 
             before do
-              post(api("/jobs/#{job.id}/artifacts"), params: post_data, headers: headers_with_token)
+              workhorse_finalize_with_multiple_files(
+                api("/jobs/#{job.id}/artifacts"),
+                method: :post,
+                file_keys: file_keys,
+                params: post_data,
+                headers: headers_with_token,
+                send_rewritten_field: send_rewritten_field
+              )
             end
 
             context 'when posts data accelerated by workhorse is correct' do
-              let(:post_data) do
-                { 'file.path' => artifacts.path,
-                  'file.name' => artifacts.original_filename,
-                  'file.sha256' => artifacts_sha256,
-                  'metadata.path' => metadata.path,
-                  'metadata.name' => metadata.original_filename,
-                  'metadata.sha256' => metadata_sha256 }
-              end
+              let(:post_data) { { file: artifacts, metadata: metadata } }
 
               it 'stores artifacts and artifacts metadata' do
                 expect(response).to have_gitlab_http_status(201)
@@ -1776,9 +1777,30 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
               end
             end
 
+            context 'with a malicious file.path param' do
+              let(:post_data) { {} }
+              let(:tmp_file) { Tempfile.new('crafted.file.path') }
+              let(:url) { "/jobs/#{job.id}/artifacts?file.path=#{tmp_file.path}" }
+
+              it 'rejects the request' do
+                expect(response).to have_gitlab_http_status(:bad_request)
+                expect(stored_artifacts_size).to be_nil
+              end
+            end
+
+            context 'when workhorse header is missing' do
+              let(:post_data) { { file: artifacts, metadata: metadata } }
+              let(:send_rewritten_field) { false }
+
+              it 'rejects the request' do
+                expect(response).to have_gitlab_http_status(:bad_request)
+                expect(stored_artifacts_size).to be_nil
+              end
+            end
+
             context 'when there is no artifacts file in post data' do
               let(:post_data) do
-                { 'metadata' => metadata }
+                { metadata: metadata }
               end
 
               it 'is expected to respond with bad request' do
@@ -1923,7 +1945,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             method: :post,
             file_key: :file,
             params: params.merge(file: file),
-            headers: headers
+            headers: headers,
+            send_rewritten_field: true
           )
         end
       end

spec/support/helpers/workhorse_helpers.rb

@@ -33,22 +33,36 @@ module WorkhorseHelpers
   # workhorse_finalize will transform file_key inside params as if it was the finalize call of an inline object storage upload.
   # note that based on the content of the params it can simulate a disc acceleration or an object storage upload
   def workhorse_finalize(url, method: :post, file_key:, params:, headers: {}, send_rewritten_field: false)
-    workhorse_request_with_file(method, url,
-                                file_key: file_key,
-                                params: params,
-                                extra_headers: headers,
-                                send_rewritten_field: send_rewritten_field
+    workhorse_finalize_with_multiple_files(url, method: method, file_keys: file_key, params: params, headers: headers, send_rewritten_field: send_rewritten_field)
+  end
+
+  def workhorse_finalize_with_multiple_files(url, method: :post, file_keys:, params:, headers: {}, send_rewritten_field: false)
+    workhorse_request_with_multiple_files(method, url,
+                                          file_keys: file_keys,
+                                          params: params,
+                                          extra_headers: headers,
+                                          send_rewritten_field: send_rewritten_field
     )
   end
 
   def workhorse_request_with_file(method, url, file_key:, params:, env: {}, extra_headers: {}, send_rewritten_field:)
+    workhorse_request_with_multiple_files(method, url, file_keys: file_key, params: params, env: env, extra_headers: extra_headers, send_rewritten_field: send_rewritten_field)
+  end
+
+  def workhorse_request_with_multiple_files(method, url, file_keys:, params:, env: {}, extra_headers: {}, send_rewritten_field:)
     workhorse_params = params.dup
-    file = workhorse_params.delete(file_key)
 
-    workhorse_params = workhorse_disk_accelerated_file_params(file_key, file).merge(workhorse_params)
+    file_keys = Array(file_keys)
+    rewritten_fields = {}
+
+    file_keys.each do |key|
+      file = workhorse_params.delete(key)
+      rewritten_fields[key] = file.path if file
+      workhorse_params = workhorse_disk_accelerated_file_params(key, file).merge(workhorse_params)
+    end
 
     headers = if send_rewritten_field
-                workhorse_rewritten_fields_header(file_key => file.path)
+                workhorse_rewritten_fields_header(rewritten_fields)
               else
                 {}
               end
@@ -75,7 +89,11 @@ module WorkhorseHelpers
       "#{key}.name" => file.original_filename,
       "#{key}.size" => file.size
     }.tap do |params|
-      params["#{key}.path"] = file.path if file.path
+      if file.path
+        params["#{key}.path"] = file.path
+        params["#{key}.sha256"] = Digest::SHA256.file(file.path).hexdigest
+      end
+
       params["#{key}.remote_id"] = file.remote_id if file.respond_to?(:remote_id) && file.remote_id.present?
     end
   end

spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'multipart middleware context' do
+  let(:app) { double(:app) }
+  let(:middleware) { described_class.new(app) }
+  let(:original_filename) { 'filename' }
+
+  # Rails 5 doesn't combine the GET/POST parameters in
+  # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set:
+  # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41
+  def get_params(env)
+    req = ActionDispatch::Request.new(env)
+    req.GET.merge(req.POST)
+  end
+
+  def post_env(rewritten_fields, params, secret, issuer)
+    token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256')
+    Rack::MockRequest.env_for(
+      '/',
+      method: 'post',
+      params: params,
+      described_class::RACK_ENV_KEY => token
+    )
+  end
+
+  def with_tmp_dir(uploads_sub_dir, storage_path = '')
+    Dir.mktmpdir do |dir|
+      upload_dir = File.join(dir, storage_path, uploads_sub_dir)
+      FileUtils.mkdir_p(upload_dir)
+
+      allow(Rails).to receive(:root).and_return(dir)
+      allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir'))
+      allow(GitlabUploader).to receive(:root).and_return(File.join(dir, storage_path))
+
+      Tempfile.open('top-level', upload_dir) do |tempfile|
+        env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+        yield dir, env
+      end
+    end
+  end
+end