From 40892389b050f5b0e8cf0526776d4d8a59a01cd3 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 8 Jan 2018 15:42:30 +0000 Subject: Merge branch 'fix/import-rce-10-2' into 'security-10-2' [10.2] Fix RCE via project import mechanism See merge request gitlab/gitlabhq!2293 (cherry picked from commit 836918b04ed739fe07b239d0e4eab58296218c8c) cec9a6ae Fix RCE via project import mechanism --- changelogs/unreleased/fix-import-rce.yml | 5 ++ lib/gitlab/import_export/file_importer.rb | 6 ++- lib/gitlab/import_export/saver.rb | 2 +- lib/gitlab/import_export/shared.rb | 14 +++++- .../lib/gitlab/import_export/file_importer_spec.rb | 57 +++++++++++++++++----- 5 files changed, 68 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/fix-import-rce.yml diff --git a/changelogs/unreleased/fix-import-rce.yml b/changelogs/unreleased/fix-import-rce.yml new file mode 100644 index 00000000000..c47e45fac31 --- /dev/null +++ b/changelogs/unreleased/fix-import-rce.yml @@ -0,0 +1,5 @@ +--- +title: Fix RCE via project import mechanism +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/file_importer.rb b/lib/gitlab/import_export/file_importer.rb index 989342389bc..5c971564a73 100644 --- a/lib/gitlab/import_export/file_importer.rb +++ b/lib/gitlab/import_export/file_importer.rb @@ -17,12 +17,16 @@ module Gitlab def import mkdir_p(@shared.export_path) + remove_symlinks! + wait_for_archived_file do decompress_archive end rescue => e @shared.error(e) false + ensure + remove_symlinks! end private @@ -43,7 +47,7 @@ module Gitlab raise Projects::ImportService::Error.new("Unable to decompress #{@archive_file} into #{@shared.export_path}") unless result - remove_symlinks! + result end def remove_symlinks! diff --git a/lib/gitlab/import_export/saver.rb b/lib/gitlab/import_export/saver.rb index 6130c124dd1..2daeba90a51 100644 --- a/lib/gitlab/import_export/saver.rb +++ b/lib/gitlab/import_export/saver.rb @@ -37,7 +37,7 @@ module Gitlab end def archive_file - @archive_file ||= File.join(@shared.export_path, '..', Gitlab::ImportExport.export_filename(project: @project)) + @archive_file ||= File.join(@shared.archive_path, Gitlab::ImportExport.export_filename(project: @project)) end end end diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index 9fd0b709ef2..d03cbc880fd 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -9,7 +9,11 @@ module Gitlab end def export_path - @export_path ||= Gitlab::ImportExport.export_path(relative_path: opts[:relative_path]) + @export_path ||= Gitlab::ImportExport.export_path(relative_path: relative_path) + end + + def archive_path + @archive_path ||= Gitlab::ImportExport.export_path(relative_path: relative_archive_path) end def error(error) @@ -21,6 +25,14 @@ module Gitlab private + def relative_path + File.join(opts[:relative_path], SecureRandom.hex) + end + + def relative_archive_path + File.join(opts[:relative_path], '..') + end + def error_out(message, caller) Rails.logger.error("Import/Export error raised on #{caller}: #{message}") end diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb index 162b776e107..5cdc5138fda 100644 --- a/spec/lib/gitlab/import_export/file_importer_spec.rb +++ b/spec/lib/gitlab/import_export/file_importer_spec.rb @@ -12,30 +12,61 @@ describe Gitlab::ImportExport::FileImporter do stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0) allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) allow_any_instance_of(Gitlab::ImportExport::CommandLineUtil).to receive(:untar_zxf).and_return(true) - + allow(SecureRandom).to receive(:hex).and_return('abcd') setup_files - - described_class.import(archive_file: '', shared: shared) end after do FileUtils.rm_rf(export_path) end - it 'removes symlinks in root folder' do - expect(File.exist?(symlink_file)).to be false - end + context 'normal run' do + before do + described_class.import(archive_file: '', shared: shared) + end - it 'removes hidden symlinks in root folder' do - expect(File.exist?(hidden_symlink_file)).to be false - end + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end - it 'removes symlinks in subfolders' do - expect(File.exist?(subfolder_symlink_file)).to be false + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end + + it 'creates the file in the right subfolder' do + expect(shared.export_path).to include('test/abcd') + end end - it 'does not remove a valid file' do - expect(File.exist?(valid_file)).to be true + context 'error' do + before do + allow_any_instance_of(described_class).to receive(:wait_for_archived_file).and_raise(StandardError) + described_class.import(archive_file: '', shared: shared) + end + + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end + + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end end def setup_files -- cgit v1.2.1