diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-10 20:42:50 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-10 20:43:16 +0000 |
commit | b55e13ec164336d1e5d5bbdbca939edcc31d557f (patch) | |
tree | b3283adcd5aa0cd6ac55afc75793189e7507e4ae | |
parent | ceaba594d07563402ac901c958b9a6e27c4964dc (diff) | |
download | gitlab-ce-b55e13ec164336d1e5d5bbdbca939edcc31d557f.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-6-stable-ee
-rw-r--r-- | app/services/bulk_imports/archive_extraction_service.rb | 74 | ||||
-rw-r--r-- | lib/bulk_imports/common/pipelines/uploads_pipeline.rb | 39 | ||||
-rw-r--r-- | lib/gitlab/import_export/command_line_util.rb | 4 | ||||
-rw-r--r-- | spec/fixtures/symlink_export.tar | bin | 0 -> 10240 bytes | |||
-rw-r--r-- | spec/initializers/action_cable_subscription_adapter_identifier_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/bulk_imports/common/pipelines/uploads_pipeline_spec.rb | 24 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/command_line_util_spec.rb | 35 | ||||
-rw-r--r-- | spec/services/bulk_imports/archive_extraction_service_spec.rb | 60 |
8 files changed, 229 insertions, 13 deletions
diff --git a/app/services/bulk_imports/archive_extraction_service.rb b/app/services/bulk_imports/archive_extraction_service.rb new file mode 100644 index 00000000000..9fc828b8e34 --- /dev/null +++ b/app/services/bulk_imports/archive_extraction_service.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +# Archive Extraction Service allows extraction of contents +# from `tar` archives with an additional handling (removal) +# of file symlinks. +# +# @param tmpdir [String] A path where archive is located +# and where its contents are extracted. +# Tmpdir directory must be located under `Dir.tmpdir`. +# `BulkImports::Error` is raised if any other directory path is used. +# +# @param filename [String] Name of the file to extract contents from. +# +# @example +# dir = Dir.mktmpdir +# filename = 'things.tar' +# BulkImports::ArchiveExtractionService.new(tmpdir: dir, filename: filename).execute +# Dir.glob(File.join(dir, '**', '*')) +# => ['/path/to/tmp/dir/extracted_file_1', '/path/to/tmp/dir/extracted_file_2', '/path/to/tmp/dir/extracted_file_3'] +module BulkImports + class ArchiveExtractionService + include Gitlab::ImportExport::CommandLineUtil + + def initialize(tmpdir:, filename:) + @tmpdir = tmpdir + @filename = filename + @filepath = File.join(@tmpdir, @filename) + end + + def execute + validate_filepath + validate_tmpdir + validate_symlink + + extract_archive + remove_symlinks + tmpdir + end + + private + + attr_reader :tmpdir, :filename, :filepath + + def validate_filepath + Gitlab::Utils.check_path_traversal!(filepath) + end + + def validate_tmpdir + raise(BulkImports::Error, 'Invalid target directory') unless File.expand_path(tmpdir).start_with?(Dir.tmpdir) + end + + def validate_symlink + raise(BulkImports::Error, 'Invalid file') if symlink?(filepath) + end + + def symlink?(filepath) + File.lstat(filepath).symlink? + end + + def extract_archive + untar_xf(archive: filepath, dir: tmpdir) + end + + def extracted_files + Dir.glob(File.join(tmpdir, '**', '*')) + end + + def remove_symlinks + extracted_files.each do |path| + FileUtils.rm(path) if symlink?(path) + end + end + end +end diff --git a/lib/bulk_imports/common/pipelines/uploads_pipeline.rb b/lib/bulk_imports/common/pipelines/uploads_pipeline.rb index 49c16209661..2ac4e533c1d 100644 --- a/lib/bulk_imports/common/pipelines/uploads_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/uploads_pipeline.rb @@ -5,16 +5,16 @@ module BulkImports module Pipelines class UploadsPipeline include Pipeline - include Gitlab::ImportExport::CommandLineUtil - FILENAME = 'uploads.tar.gz' AVATAR_PATTERN = %r{.*\/#{BulkImports::UploadsExportService::AVATAR_PATH}\/(?<identifier>.*)}.freeze AvatarLoadingError = Class.new(StandardError) - def extract(context) - download_service(tmp_dir, context).execute - untar_zxf(archive: File.join(tmp_dir, FILENAME), dir: tmp_dir) + def extract(_context) + download_service.execute + decompression_service.execute + extraction_service.execute + upload_file_paths = Dir.glob(File.join(tmp_dir, '**', '*')) BulkImports::Pipeline::ExtractedData.new(data: upload_file_paths) @@ -29,6 +29,7 @@ module BulkImports return unless dynamic_path return if File.directory?(file_path) + return if File.lstat(file_path).symlink? named_captures = dynamic_path.named_captures.symbolize_keys @@ -36,20 +37,40 @@ module BulkImports end def after_run(_) - FileUtils.remove_entry(tmp_dir) + FileUtils.remove_entry(tmp_dir) if Dir.exist?(tmp_dir) end private - def download_service(tmp_dir, context) + def download_service BulkImports::FileDownloadService.new( configuration: context.configuration, - relative_url: context.entity.relation_download_url_path('uploads'), + relative_url: context.entity.relation_download_url_path(relation), dir: tmp_dir, - filename: FILENAME + filename: targz_filename ) end + def decompression_service + BulkImports::FileDecompressionService.new(dir: tmp_dir, filename: targz_filename) + end + + def extraction_service + BulkImports::ArchiveExtractionService.new(tmpdir: tmp_dir, filename: tar_filename) + end + + def relation + BulkImports::FileTransfer::BaseConfig::UPLOADS_RELATION + end + + def tar_filename + "#{relation}.tar" + end + + def targz_filename + "#{tar_filename}.gz" + end + def tmp_dir @tmp_dir ||= Dir.mktmpdir('bulk_imports') end diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index fdc4c22001f..3da9083e743 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -18,6 +18,10 @@ module Gitlab tar_with_options(archive: archive, dir: dir, options: 'cf') end + def untar_xf(archive:, dir:) + untar_with_options(archive: archive, dir: dir, options: 'xf') + end + def gzip(dir:, filename:) gzip_with_options(dir: dir, filename: filename) end diff --git a/spec/fixtures/symlink_export.tar b/spec/fixtures/symlink_export.tar Binary files differnew file mode 100644 index 00000000000..111874c729c --- /dev/null +++ b/spec/fixtures/symlink_export.tar diff --git a/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb b/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb index 12988b851ef..074df9adc21 100644 --- a/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb +++ b/spec/initializers/action_cable_subscription_adapter_identifier_spec.rb @@ -4,6 +4,12 @@ require 'spec_helper' RSpec.describe 'ActionCableSubscriptionAdapterIdentifier override' do describe '#identifier' do + let!(:original_config) { ::ActionCable::Server::Base.config.cable } + + after do + ::ActionCable::Server::Base.config.cable = original_config + end + context 'when id key is nil on cable.yml' do it 'does not override server config id with action cable pid' do config = { diff --git a/spec/lib/bulk_imports/common/pipelines/uploads_pipeline_spec.rb b/spec/lib/bulk_imports/common/pipelines/uploads_pipeline_spec.rb index 0f6238e10dc..3b5ea131d0d 100644 --- a/spec/lib/bulk_imports/common/pipelines/uploads_pipeline_spec.rb +++ b/spec/lib/bulk_imports/common/pipelines/uploads_pipeline_spec.rb @@ -70,8 +70,10 @@ RSpec.describe BulkImports::Common::Pipelines::UploadsPipeline do describe '#extract' do it 'downloads & extracts upload paths' do allow(Dir).to receive(:mktmpdir).and_return(tmpdir) - expect(pipeline).to receive(:untar_zxf) - file_download_service = instance_double("BulkImports::FileDownloadService") + + download_service = instance_double("BulkImports::FileDownloadService") + decompression_service = instance_double("BulkImports::FileDecompressionService") + extraction_service = instance_double("BulkImports::ArchiveExtractionService") expect(BulkImports::FileDownloadService) .to receive(:new) @@ -80,9 +82,13 @@ RSpec.describe BulkImports::Common::Pipelines::UploadsPipeline do relative_url: "/#{entity.pluralized_name}/test/export_relations/download?relation=uploads", dir: tmpdir, filename: 'uploads.tar.gz') - .and_return(file_download_service) + .and_return(download_service) + expect(BulkImports::FileDecompressionService).to receive(:new).with(dir: tmpdir, filename: 'uploads.tar.gz').and_return(decompression_service) + expect(BulkImports::ArchiveExtractionService).to receive(:new).with(tmpdir: tmpdir, filename: 'uploads.tar').and_return(extraction_service) - expect(file_download_service).to receive(:execute) + expect(download_service).to receive(:execute) + expect(decompression_service).to receive(:execute) + expect(extraction_service).to receive(:execute) extracted_data = pipeline.extract(context) @@ -106,6 +112,16 @@ RSpec.describe BulkImports::Common::Pipelines::UploadsPipeline do expect { pipeline.load(context, uploads_dir_path) }.not_to change { portable.uploads.count } end end + + context 'when path is a symlink' do + it 'does not upload the file' do + symlink = File.join(tmpdir, 'symlink') + + FileUtils.ln_s(File.join(tmpdir, upload_file_path), symlink) + + expect { pipeline.load(context, symlink) }.not_to change { portable.uploads.count } + end + end end end diff --git a/spec/lib/gitlab/import_export/command_line_util_spec.rb b/spec/lib/gitlab/import_export/command_line_util_spec.rb index 59c4e1083ae..31512259bb1 100644 --- a/spec/lib/gitlab/import_export/command_line_util_spec.rb +++ b/spec/lib/gitlab/import_export/command_line_util_spec.rb @@ -101,4 +101,39 @@ RSpec.describe Gitlab::ImportExport::CommandLineUtil do end end end + + describe '#untar_xf' do + let(:archive_dir) { Dir.mktmpdir } + + after do + FileUtils.remove_entry(archive_dir) + end + + it 'extracts archive without decompression' do + filename = 'archive.tar.gz' + archive_file = File.join(archive_dir, 'archive.tar') + + FileUtils.copy_file(archive, File.join(archive_dir, filename)) + subject.gunzip(dir: archive_dir, filename: filename) + + result = subject.untar_xf(archive: archive_file, dir: archive_dir) + + expect(result).to eq(true) + expect(File.exist?(archive_file)).to eq(true) + expect(File.exist?(File.join(archive_dir, 'project.json'))).to eq(true) + expect(Dir.exist?(File.join(archive_dir, 'uploads'))).to eq(true) + end + + context 'when something goes wrong' do + it 'raises an error' do + expect(Gitlab::Popen).to receive(:popen).and_return(['Error', 1]) + + klass = Class.new do + include Gitlab::ImportExport::CommandLineUtil + end.new + + expect { klass.untar_xf(archive: 'test', dir: 'test') }.to raise_error(Gitlab::ImportExport::Error, 'System call failed') + end + end + end end diff --git a/spec/services/bulk_imports/archive_extraction_service_spec.rb b/spec/services/bulk_imports/archive_extraction_service_spec.rb new file mode 100644 index 00000000000..aa823d88010 --- /dev/null +++ b/spec/services/bulk_imports/archive_extraction_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BulkImports::ArchiveExtractionService do + let_it_be(:tmpdir) { Dir.mktmpdir } + let_it_be(:filename) { 'symlink_export.tar' } + let_it_be(:filepath) { File.join(tmpdir, filename) } + + before do + FileUtils.copy_file(File.join('spec', 'fixtures', filename), filepath) + end + + after(:all) do + FileUtils.remove_entry(tmpdir) + end + + subject(:service) { described_class.new(tmpdir: tmpdir, filename: filename) } + + describe '#execute' do + it 'extracts files from archive and removes symlinks' do + file = File.join(tmpdir, 'project.json') + folder = File.join(tmpdir, 'uploads') + symlink = File.join(tmpdir, 'uploads', 'link.gitignore') + + expect(service).to receive(:untar_xf).with(archive: filepath, dir: tmpdir).and_call_original + + service.execute + + expect(File.exist?(file)).to eq(true) + expect(Dir.exist?(folder)).to eq(true) + expect(File.exist?(symlink)).to eq(false) + end + + context 'when dir is not in tmpdir' do + it 'raises an error' do + ['/etc', '/usr', '/', '/home', '', '/some/other/path', Rails.root].each do |path| + expect { described_class.new(tmpdir: path, filename: 'filename').execute } + .to raise_error(BulkImports::Error, 'Invalid target directory') + end + end + end + + context 'when archive file is a symlink' do + it 'raises an error' do + FileUtils.ln_s(File.join(tmpdir, filename), File.join(tmpdir, 'symlink')) + + expect { described_class.new(tmpdir: tmpdir, filename: 'symlink').execute } + .to raise_error(BulkImports::Error, 'Invalid file') + end + end + + context 'when filepath is being traversed' do + it 'raises an error' do + expect { described_class.new(tmpdir: File.join(tmpdir, '../../../'), filename: 'name').execute } + .to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path') + end + end + end +end |