Commit ce1fa35a authored by GitLab Bot's avatar GitLab Bot

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

parent 7c9f211b
---
title: Validate workhorse 'rewritten_fields' and properly use them during multipart
uploads
merge_request:
author:
type: security
...@@ -43,11 +43,13 @@ module Gitlab ...@@ -43,11 +43,13 @@ module Gitlab
raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1 raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
key, value = parsed_field.first key, value = parsed_field.first
if value.nil? if value.nil? # we have a top level param, eg. field = 'foo' and not 'foo[bar]'
value = open_file(@request.params, key) raise "invalid field: #{field.inspect}" if field != key
value = open_file(@request.params, key, tmp_path.presence)
@open_files << value @open_files << value
else else
value = decorate_params_value(value, @request.params[key]) value = decorate_params_value(value, @request.params[key], tmp_path.presence)
end end
update_param(key, value) update_param(key, value)
...@@ -59,7 +61,7 @@ module Gitlab ...@@ -59,7 +61,7 @@ module Gitlab
end end
# This function calls itself recursively # 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 unless path_hash.is_a?(Hash) && path_hash.count == 1
raise "invalid path: #{path_hash.inspect}" raise "invalid path: #{path_hash.inspect}"
end end
...@@ -72,19 +74,19 @@ module Gitlab ...@@ -72,19 +74,19 @@ module Gitlab
case path_value case path_value
when nil 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] @open_files << value_hash[path_key]
value_hash value_hash
when 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 value_hash
else else
raise "unexpected path value: #{path_value.inspect}" raise "unexpected path value: #{path_value.inspect}"
end end
end end
def open_file(params, key) def open_file(params, key, path_override = nil)
::UploadedFile.from_params(params, key, allowed_paths) ::UploadedFile.from_params(params, key, allowed_paths, path_override)
end end
# update_params ensures that both rails controllers and rack middleware can find # update_params ensures that both rails controllers and rack middleware can find
......
...@@ -42,13 +42,14 @@ class UploadedFile ...@@ -42,13 +42,14 @@ class UploadedFile
@remote_id = remote_id @remote_id = remote_id
end end
def self.from_params(params, field, upload_paths) def self.from_params(params, field, upload_paths, path_override = nil)
path = params["#{field}.path"] path = path_override || params["#{field}.path"]
remote_id = params["#{field}.remote_id"] remote_id = params["#{field}.remote_id"]
return if path.blank? && remote_id.blank? return if path.blank? && remote_id.blank?
file_path = nil if remote_id.present? # don't use file_path if remote_id is set
if path.present? file_path = nil
elsif path.present?
file_path = File.realpath(path) file_path = File.realpath(path)
paths = Array(upload_paths) << Dir.tmpdir paths = Array(upload_paths) << Dir.tmpdir
......
...@@ -7,11 +7,11 @@ require 'tempfile' ...@@ -7,11 +7,11 @@ require 'tempfile'
describe Gitlab::Middleware::Multipart do describe Gitlab::Middleware::Multipart do
include_context 'multipart middleware context' 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 it 'opens top-level files' do
Tempfile.open('top-level') do |tempfile| Tempfile.open('top-level') do |tempfile|
rewritten = { 'file' => tempfile.path } 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') env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
expect_uploaded_file(tempfile, %w(file)) expect_uploaded_file(tempfile, %w(file))
...@@ -22,8 +22,8 @@ describe Gitlab::Middleware::Multipart do ...@@ -22,8 +22,8 @@ describe Gitlab::Middleware::Multipart do
it 'opens files one level deep' do it 'opens files one level deep' do
Tempfile.open('one-level') do |tempfile| 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 } 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') env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
expect_uploaded_file(tempfile, %w(user avatar)) expect_uploaded_file(tempfile, %w(user avatar))
...@@ -34,7 +34,7 @@ describe Gitlab::Middleware::Multipart do ...@@ -34,7 +34,7 @@ describe Gitlab::Middleware::Multipart do
it 'opens files two levels deep' do it 'opens files two levels deep' do
Tempfile.open('two-levels') do |tempfile| 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 } rewritten = { 'project[milestone][themesong]' => tempfile.path }
env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse') env = post_env(rewritten, in_params, Gitlab::Workhorse.secret, 'gitlab-workhorse')
...@@ -44,13 +44,61 @@ describe Gitlab::Middleware::Multipart do ...@@ -44,13 +44,61 @@ describe Gitlab::Middleware::Multipart do
end end
end end
def expect_uploaded_file(tempfile, path, remote: false) def expect_uploaded_file(tempfile, path)
expect(app).to receive(:call) do |env| expect(app).to receive(:call) do |env|
file = get_params(env).dig(*path) file = get_params(env).dig(*path)
expect(file).to be_a(::UploadedFile) expect(file).to be_a(::UploadedFile)
expect(file.path).to eq(tempfile.path)
expect(file.original_filename).to eq(original_filename) 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 end
end end
...@@ -67,18 +115,65 @@ describe Gitlab::Middleware::Multipart do ...@@ -67,18 +115,65 @@ describe Gitlab::Middleware::Multipart do
expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError) expect { middleware.call(env) }.to raise_error(JWT::InvalidIssuerError)
end 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 context 'with remote file' do
let(:remote_id) { 'someid' } 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' it_behaves_like 'multipart upload files'
end end
context 'with local file' do context 'with local file' do
let(:remote_id) { nil } 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' it_behaves_like 'multipart upload files'
end 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 it 'allows files in uploads/tmp directory' do
with_tmp_dir('public/uploads/tmp') do |dir, env| with_tmp_dir('public/uploads/tmp') do |dir, env|
expect(app).to receive(:call) do |env| expect(app).to receive(:call) do |env|
......
...@@ -4,7 +4,7 @@ require 'spec_helper' ...@@ -4,7 +4,7 @@ require 'spec_helper'
describe UploadedFile do describe UploadedFile do
let(:temp_dir) { Dir.tmpdir } 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 before do
FileUtils.touch(temp_file) FileUtils.touch(temp_file)
...@@ -16,13 +16,14 @@ describe UploadedFile do ...@@ -16,13 +16,14 @@ describe UploadedFile do
describe ".from_params" do describe ".from_params" do
let(:upload_path) { nil } let(:upload_path) { nil }
let(:file_path_override) { nil }
after do after do
FileUtils.rm_r(upload_path) if upload_path FileUtils.rm_r(upload_path) if upload_path
end end
subject do subject do
described_class.from_params(params, :file, upload_path) described_class.from_params(params, :file, upload_path, file_path_override)
end end
context 'when valid file is specified' do context 'when valid file is specified' do
...@@ -31,9 +32,7 @@ describe UploadedFile do ...@@ -31,9 +32,7 @@ describe UploadedFile do
{ 'file.path' => temp_file.path } { 'file.path' => temp_file.path }
end end
it "succeeds" do it { is_expected.not_to be_nil }
is_expected.not_to be_nil
end
it "generates filename from path" do it "generates filename from path" do
expect(subject.original_filename).to eq(::File.basename(temp_file.path)) expect(subject.original_filename).to eq(::File.basename(temp_file.path))
...@@ -41,33 +40,153 @@ describe UploadedFile do ...@@ -41,33 +40,153 @@ describe UploadedFile do
end end
context 'all parameters are specified' do context 'all parameters are specified' do
let(:params) do RSpec.shared_context 'filepath override' do
{ 'file.path' => temp_file.path, let(:temp_file_override) { Tempfile.new(%w[override override], temp_dir) }
'file.name' => 'dir/my file&.txt', let(:file_path_override) { temp_file_override.path }
'file.type' => 'my/type',
'file.sha256' => 'sha256', before do
'file.remote_id' => 'remote_id' } FileUtils.touch(temp_file_override)
end
after do
FileUtils.rm_f(temp_file_override)
end
end end
it "succeeds" do RSpec.shared_examples 'using the file path' do |filename:, content_type:, sha256:, path_suffix:|
is_expected.not_to be_nil 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 end
it "generates filename from path" do RSpec.shared_examples 'using the remote id' do |filename:, content_type:, sha256:, size:, remote_id:|
expect(subject.original_filename).to eq('my_file_.txt') it 'sets properly the attributes' do
expect(subject.content_type).to eq('my/type') expect(subject.original_filename).to eq(filename)
expect(subject.sha256).to eq('sha256') expect(subject.content_type).to eq('application/octet-stream')
expect(subject.remote_id).to eq('remote_id') 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 end
it 'handles a blank path' do context 'with a remote id' do
params['file.path'] = '' 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 context 'with a path and a remote id' do
params['file.size'] = 1.byte 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) } context 'with a path override and a remote id' do
.not_to raise_error 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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment