Merge branch...

Merge branch 'security-validate-use-propery-workhorse-rewritten-fields-for-multipart-uploads' into 'master'

Properly use workhorse rewritten fields and validate them

Closes #116

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

changelogs/unreleased/security-validate-use-propery-workhorse-rewritten-fields-for-multipart-up.yml

+---
+title: Validate workhorse 'rewritten_fields' and properly use them during multipart
+  uploads
+merge_request:
+author:
+type: security

lib/gitlab/middleware/multipart.rb

@@ -43,11 +43,13 @@ module Gitlab
             raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
 
             key, value = parsed_field.first
-            if value.nil?
-              value = open_file(@request.params, key)
+            if value.nil? # we have a top level param, eg. field = 'foo' and not 'foo[bar]'
+              raise "invalid field: #{field.inspect}" if field != key
+
+              value = open_file(@request.params, key, tmp_path.presence)
               @open_files << value
             else
-              value = decorate_params_value(value, @request.params[key])
+              value = decorate_params_value(value, @request.params[key], tmp_path.presence)
             end
 
             update_param(key, value)
@@ -59,7 +61,7 @@ module Gitlab
         end
 
         # This function calls itself recursively
-        def decorate_params_value(path_hash, value_hash)
+        def decorate_params_value(path_hash, value_hash, path_override = nil)
           unless path_hash.is_a?(Hash) && path_hash.count == 1
             raise "invalid path: #{path_hash.inspect}"
           end
@@ -72,19 +74,19 @@ module Gitlab
 
           case path_value
           when nil
-            value_hash[path_key] = open_file(value_hash.dig(path_key), '')
+            value_hash[path_key] = open_file(value_hash.dig(path_key), '', path_override)
             @open_files << value_hash[path_key]
             value_hash
           when Hash
-            decorate_params_value(path_value, value_hash[path_key])
+            decorate_params_value(path_value, value_hash[path_key], path_override)
             value_hash
           else
             raise "unexpected path value: #{path_value.inspect}"
           end
         end
 
-        def open_file(params, key)
-          ::UploadedFile.from_params(params, key, allowed_paths)
+        def open_file(params, key, path_override = nil)
+          ::UploadedFile.from_params(params, key, allowed_paths, path_override)
         end
 
         # update_params ensures that both rails controllers and rack middleware can find

lib/uploaded_file.rb

@@ -42,13 +42,14 @@ class UploadedFile
     @remote_id = remote_id
   end
 
-  def self.from_params(params, field, upload_paths)
-    path = params["#{field}.path"]
+  def self.from_params(params, field, upload_paths, path_override = nil)
+    path = path_override || params["#{field}.path"]
     remote_id = params["#{field}.remote_id"]
     return if path.blank? && remote_id.blank?
 
-    file_path = nil
-    if path.present?
+    if remote_id.present? # don't use file_path if remote_id is set
+      file_path = nil
+    elsif path.present?
       file_path = File.realpath(path)
 
       paths = Array(upload_paths) << Dir.tmpdir

spec/lib/gitlab/middleware/multipart_spec.rb

@@ -7,11 +7,11 @@ require 'tempfile'
 describe Gitlab::Middleware::Multipart do
   include_context 'multipart middleware context'
 
-  shared_examples_for 'multipart upload files' do
+  RSpec.shared_examples_for 'multipart upload files' do
     it 'opens top-level files' do
       Tempfile.open('top-level') do |tempfile|
         rewritten = { 'file' => tempfile.path }
-        in_params = { 'file.name' => original_filename, 'file.path' => tempfile.path, 'file.remote_id' => remote_id }
+        in_params = { 'file.name' => original_filename, 'file.path' => file_path, 'file.remote_id' => remote_id, 'file.size' => file_size }
         env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
         expect_uploaded_file(tempfile, %w(file))
@@ -22,8 +22,8 @@ describe Gitlab::Middleware::Multipart do
 
     it 'opens files one level deep' do
       Tempfile.open('one-level') do |tempfile|
-        in_params = { 'user' => { 'avatar' => { '.name' => original_filename, '.path' => tempfile.path, '.remote_id' => remote_id } } }
         rewritten = { 'user[avatar]' => tempfile.path }
+        in_params = { 'user' => { 'avatar' => { '.name' => original_filename, '.path' => file_path, '.remote_id' => remote_id, '.size' => file_size } } }
         env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
         expect_uploaded_file(tempfile, %w(user avatar))
@@ -34,7 +34,7 @@ describe Gitlab::Middleware::Multipart do
 
     it 'opens files two levels deep' do
       Tempfile.open('two-levels') do |tempfile|
-        in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename, '.path' => tempfile.path, '.remote_id' => remote_id } } } }
+        in_params = { 'project' => { 'milestone' => { 'themesong' => { '.name' => original_filename, '.path' => file_path, '.remote_id' => remote_id, '.size' => file_size } } } }
         rewritten = { 'project[milestone][themesong]' => tempfile.path }
         env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
 
@@ -44,13 +44,61 @@ describe Gitlab::Middleware::Multipart do
       end
     end
 
-    def expect_uploaded_file(tempfile, path, remote: false)
+    def expect_uploaded_file(tempfile, path)
       expect(app).to receive(:call) do |env|
         file = get_params(env).dig(*path)
         expect(file).to be_a(::UploadedFile)
-        expect(file.path).to eq(tempfile.path)
         expect(file.original_filename).to eq(original_filename)
-        expect(file.remote_id).to eq(remote_id)
+
+        if remote_id
+          expect(file.remote_id).to eq(remote_id)
+          expect(file.path).to be_nil
+        else
+          expect(file.path).to eq(File.realpath(tempfile.path))
+          expect(file.remote_id).to be_nil
+        end
+      end
+    end
+  end
+
+  RSpec.shared_examples_for 'handling CI artifact upload' do
+    it 'uploads both file and metadata' do
+      Tempfile.open('file') do |file|
+        Tempfile.open('metadata') do |metadata|
+          rewritten = { 'file' => file.path, 'metadata' => metadata.path }
+          in_params = { 'file.name' => 'file.txt', 'file.path' => file_path, 'file.remote_id' => file_remote_id, 'file.size' => file_size, 'metadata.name' => 'metadata.gz' }
+          env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+          with_expected_uploaded_artifact_files(file, metadata) do |uploaded_file, uploaded_metadata|
+            expect(uploaded_file).to be_a(::UploadedFile)
+            expect(uploaded_file.original_filename).to eq('file.txt')
+
+            if file_remote_id
+              expect(uploaded_file.remote_id).to eq(file_remote_id)
+              expect(uploaded_file.size).to eq(file_size)
+              expect(uploaded_file.path).to be_nil
+            else
+              expect(uploaded_file.path).to eq(File.realpath(file.path))
+              expect(uploaded_file.remote_id).to be_nil
+            end
+
+            expect(uploaded_metadata).to be_a(::UploadedFile)
+            expect(uploaded_metadata.original_filename).to eq('metadata.gz')
+            expect(uploaded_metadata.path).to eq(File.realpath(metadata.path))
+            expect(uploaded_metadata.remote_id).to be_nil
+          end
+
+          middleware.call(env)
+        end
+      end
+    end
+
+    def with_expected_uploaded_artifact_files(file, metadata)
+      expect(app).to receive(:call) do |env|
+        file = get_params(env).dig('file')
+        metadata = get_params(env).dig('metadata')
+
+        yield file, metadata
       end
     end
   end
@@ -67,18 +115,65 @@ describe Gitlab::Middleware::Multipart do
     expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError)
   end
 
+  context 'with invalid rewritten field' do
+    invalid_field_names = [
+      '[file]',
+      ';file',
+      'file]',
+      ';file]',
+      'file]]',
+      'file;;'
+    ]
+
+    invalid_field_names.each do |invalid_field_name|
+      it "rejects invalid rewritten field name #{invalid_field_name}" do
+        env = post_env({ invalid_field_name => nil }, {}, Gitlab::Workhorse.secret, 'gitlab-workhorse')
+
+        expect { middleware.call(env) }.to raise_error(RuntimeError, "invalid field: \"#{invalid_field_name}\"")
+      end
+    end
+  end
+
   context 'with remote file' do
     let(:remote_id) { 'someid' }
+    let(:file_size) { 300 }
+    let(:file_path) { '' }
+
+    it_behaves_like 'multipart upload files'
+  end
+
+  context 'with remote file and a file path set' do
+    let(:remote_id) { 'someid' }
+    let(:file_size) { 300 }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
 
     it_behaves_like 'multipart upload files'
   end
 
   context 'with local file' do
     let(:remote_id) { nil }
+    let(:file_size) { nil }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
 
     it_behaves_like 'multipart upload files'
   end
 
+  context 'with remote CI artifact upload' do
+    let(:file_remote_id) { 'someid' }
+    let(:file_size) { 300 }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
+
+    it_behaves_like 'handling CI artifact upload'
+  end
+
+  context 'with local CI artifact upload' do
+    let(:file_remote_id) { nil }
+    let(:file_size) { nil }
+    let(:file_path) { 'not_a_valid_file_path' } # file path will come from the rewritten_fields
+
+    it_behaves_like 'handling CI artifact upload'
+  end
+
   it 'allows files in uploads/tmp directory' do
     with_tmp_dir('public/uploads/tmp') do |dir, env|
       expect(app).to receive(:call) do |env|

spec/lib/uploaded_file_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe UploadedFile do
   let(:temp_dir) { Dir.tmpdir }
-  let(:temp_file) { Tempfile.new("test", temp_dir) }
+  let(:temp_file) { Tempfile.new(%w[test test], temp_dir) }
 
   before do
     FileUtils.touch(temp_file)
@@ -16,13 +16,14 @@ describe UploadedFile do
 
   describe ".from_params" do
     let(:upload_path) { nil }
+    let(:file_path_override) { nil }
 
     after do
       FileUtils.rm_r(upload_path) if upload_path
     end
 
     subject do
-      described_class.from_params(params, :file, upload_path)
+      described_class.from_params(params, :file, upload_path, file_path_override)
     end
 
     context 'when valid file is specified' do
@@ -31,9 +32,7 @@ describe UploadedFile do
           { 'file.path' => temp_file.path }
         end
 
-        it "succeeds" do
-          is_expected.not_to be_nil
-        end
+        it { is_expected.not_to be_nil }
 
         it "generates filename from path" do
           expect(subject.original_filename).to eq(::File.basename(temp_file.path))
@@ -41,33 +40,153 @@ describe UploadedFile do
       end
 
       context 'all parameters are specified' do
-        let(:params) do
-          { 'file.path' => temp_file.path,
-            'file.name' => 'dir/my file&.txt',
-            'file.type' => 'my/type',
-            'file.sha256' => 'sha256',
-            'file.remote_id' => 'remote_id' }
+        RSpec.shared_context 'filepath override' do
+          let(:temp_file_override) { Tempfile.new(%w[override override], temp_dir) }
+          let(:file_path_override) { temp_file_override.path }
+
+          before do
+            FileUtils.touch(temp_file_override)
+          end
+
+          after do
+            FileUtils.rm_f(temp_file_override)
+          end
         end
 
-        it "succeeds" do
-          is_expected.not_to be_nil
+        RSpec.shared_examples 'using the file path' do |filename:, content_type:, sha256:, path_suffix:|
+          it 'sets properly the attributes' do
+            expect(subject.original_filename).to eq(filename)
+            expect(subject.content_type).to eq(content_type)
+            expect(subject.sha256).to eq(sha256)
+            expect(subject.remote_id).to be_nil
+            expect(subject.path).to end_with(path_suffix)
+          end
+
+          it 'handles a blank path' do
+            params['file.path'] = ''
+
+            # Not a real file, so can't determine size itself
+            params['file.size'] = 1.byte
+
+            expect { described_class.from_params(params, :file, upload_path) }
+              .not_to raise_error
+          end
         end
 
-        it "generates filename from path" do
-          expect(subject.original_filename).to eq('my_file_.txt')
-          expect(subject.content_type).to eq('my/type')
-          expect(subject.sha256).to eq('sha256')
-          expect(subject.remote_id).to eq('remote_id')
+        RSpec.shared_examples 'using the remote id' do |filename:, content_type:, sha256:, size:, remote_id:|
+          it 'sets properly the attributes' do
+            expect(subject.original_filename).to eq(filename)
+            expect(subject.content_type).to eq('application/octet-stream')
+            expect(subject.sha256).to eq('sha256')
+            expect(subject.path).to be_nil
+            expect(subject.size).to eq(123456)
+            expect(subject.remote_id).to eq('1234567890')
+          end
+        end
+
+        context 'with a filepath' do
+          let(:params) do
+            { 'file.path' => temp_file.path,
+              'file.name' => 'dir/my file&.txt',
+              'file.type' => 'my/type',
+              'file.sha256' => 'sha256' }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the file path',
+                          filename: 'my_file_.txt',
+                          content_type: 'my/type',
+                          sha256: 'sha256',
+                          path_suffix: 'test'
+        end
+
+        context 'with a filepath override' do
+          include_context 'filepath override'
+
+          let(:params) do
+            { 'file.path' => temp_file.path,
+              'file.name' => 'dir/my file&.txt',
+              'file.type' => 'my/type',
+              'file.sha256' => 'sha256' }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the file path',
+                          filename: 'my_file_.txt',
+                          content_type: 'my/type',
+                          sha256: 'sha256',
+                          path_suffix: 'override'
         end
 
-        it 'handles a blank path' do
-          params['file.path'] = ''
+        context 'with a remote id' do
+          let(:params) do
+            {
+              'file.name' => 'dir/my file&.txt',
+              'file.sha256' => 'sha256',
+              'file.remote_url' => 'http://localhost/file',
+              'file.remote_id' => '1234567890',
+              'file.etag' => 'etag1234567890',
+              'file.size' => '123456'
+            }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the remote id',
+                          filename: 'my_file_.txt',
+                          content_type: 'application/octet-stream',
+                          sha256: 'sha256',
+                          size: 123456,
+                          remote_id: '1234567890'
+        end
 
-          # Not a real file, so can't determine size itself
-          params['file.size'] = 1.byte
+        context 'with a path and a remote id' do
+          let(:params) do
+            {
+              'file.path' => temp_file.path,
+              'file.name' => 'dir/my file&.txt',
+              'file.sha256' => 'sha256',
+              'file.remote_url' => 'http://localhost/file',
+              'file.remote_id' => '1234567890',
+              'file.etag' => 'etag1234567890',
+              'file.size' => '123456'
+            }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the remote id',
+                          filename: 'my_file_.txt',
+                          content_type: 'application/octet-stream',
+                          sha256: 'sha256',
+                          size: 123456,
+                          remote_id: '1234567890'
+        end
 
-          expect { described_class.from_params(params, :file, upload_path) }
-            .not_to raise_error
+        context 'with a path override and a remote id' do
+          include_context 'filepath override'
+
+          let(:params) do
+            {
+              'file.name' => 'dir/my file&.txt',
+              'file.sha256' => 'sha256',
+              'file.remote_url' => 'http://localhost/file',
+              'file.remote_id' => '1234567890',
+              'file.etag' => 'etag1234567890',
+              'file.size' => '123456'
+            }
+          end
+
+          it { is_expected.not_to be_nil }
+
+          it_behaves_like 'using the remote id',
+                          filename: 'my_file_.txt',
+                          content_type: 'application/octet-stream',
+                          sha256: 'sha256',
+                          size: 123456,
+                          remote_id: '1234567890'
         end
       end
     end