From 57f082d969eae04112172495bb099e634069856c Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 5 Dec 2018 14:31:43 +0100 Subject: [PATCH] Fix path disclosure on Project Import --- app/services/projects/import_error_filter.rb | 14 +++++++ app/services/projects/import_service.rb | 12 +++++- .../security-import-path-logging.yml | 5 +++ lib/gitlab/import_export/shared.rb | 39 +++++++++++++------ spec/lib/gitlab/import_export/shared_spec.rb | 31 +++++++++++++++ .../import_export/version_checker_spec.rb | 2 +- .../projects/import_error_filter_spec.rb | 17 ++++++++ spec/services/projects/import_service_spec.rb | 4 +- 8 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 app/services/projects/import_error_filter.rb create mode 100644 changelogs/unreleased/security-import-path-logging.yml create mode 100644 spec/lib/gitlab/import_export/shared_spec.rb create mode 100644 spec/services/projects/import_error_filter_spec.rb diff --git a/app/services/projects/import_error_filter.rb b/app/services/projects/import_error_filter.rb new file mode 100644 index 00000000000..a0fc5149bb4 --- /dev/null +++ b/app/services/projects/import_error_filter.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Projects + # Used by project imports, it removes any potential paths + # included in an error message that could be stored in the DB + class ImportErrorFilter + ERROR_MESSAGE_FILTER = /[^\s]*#{File::SEPARATOR}[^\s]*(?=(\s|\z))/ + FILTER_MESSAGE = '[FILTERED]' + + def self.filter_message(message) + message.gsub(ERROR_MESSAGE_FILTER, FILTER_MESSAGE) + end + end +end diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 0c426faa22d..afd32c0d968 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -24,8 +24,16 @@ module Projects import_data success - rescue => e + rescue Gitlab::UrlBlocker::BlockedUrlError => e + Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type }) + error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{e.message}") + rescue => e + message = Projects::ImportErrorFilter.filter_message(e.message) + + Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type }) + + error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{message}") end private @@ -35,7 +43,7 @@ module Projects begin Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS) rescue Gitlab::UrlBlocker::BlockedUrlError => e - raise Error, "Blocked import URL: #{e.message}" + raise e, "Blocked import URL: #{e.message}" end end diff --git a/changelogs/unreleased/security-import-path-logging.yml b/changelogs/unreleased/security-import-path-logging.yml new file mode 100644 index 00000000000..2ba2d88d82a --- /dev/null +++ b/changelogs/unreleased/security-import-path-logging.yml @@ -0,0 +1,5 @@ +--- +title: Fix path disclosure on project import error +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index c13e6c1d83b..947caaaefee 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -8,6 +8,7 @@ module Gitlab def initialize(project) @project = project @errors = [] + @logger = Gitlab::Import::Logger.build end def active_export_count @@ -23,19 +24,16 @@ module Gitlab end def error(error) - error_out(error.message, caller[0].dup) - add_error_message(error.message) + log_error(message: error.message, caller: caller[0].dup) + log_debug(backtrace: error.backtrace&.join("\n")) - # Debug: - if error.backtrace - Rails.logger.error("Import/Export backtrace: #{error.backtrace.join("\n")}") - else - Rails.logger.error("No backtrace found") - end + Gitlab::Sentry.track_acceptable_exception(error, extra: log_base_data) + + add_error_message(error.message) end - def add_error_message(error_message) - @errors << error_message + def add_error_message(message) + @errors << filtered_error_message(message) end def after_export_in_progress? @@ -52,8 +50,25 @@ module Gitlab @project.disk_path end - def error_out(message, caller) - Rails.logger.error("Import/Export error raised on #{caller}: #{message}") + def log_error(details) + @logger.error(log_base_data.merge(details)) + end + + def log_debug(details) + @logger.debug(log_base_data.merge(details)) + end + + def log_base_data + { + importer: 'Import/Export', + import_jid: @project&.import_state&.import_jid, + project_id: @project&.id, + project_path: @project&.full_path + } + end + + def filtered_error_message(message) + Projects::ImportErrorFilter.filter_message(message) end def after_export_lock_file diff --git a/spec/lib/gitlab/import_export/shared_spec.rb b/spec/lib/gitlab/import_export/shared_spec.rb new file mode 100644 index 00000000000..f2d750c6595 --- /dev/null +++ b/spec/lib/gitlab/import_export/shared_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' +require 'fileutils' + +describe Gitlab::ImportExport::Shared do + let(:project) { build(:project) } + subject { project.import_export_shared } + + describe '#error' do + let(:error) { StandardError.new('Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file') } + + it 'filters any full paths' do + subject.error(error) + + expect(subject.errors).to eq(['Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]']) + end + + it 'calls the error logger with the full message' do + expect(subject).to receive(:log_error).with(hash_including(message: error.message)) + + subject.error(error) + end + + it 'calls the debug logger with a backtrace' do + error.set_backtrace('backtrace') + + expect(subject).to receive(:log_debug).with(hash_including(backtrace: 'backtrace')) + + subject.error(error) + end + end +end diff --git a/spec/lib/gitlab/import_export/version_checker_spec.rb b/spec/lib/gitlab/import_export/version_checker_spec.rb index 49d857d9483..76f8253ec9b 100644 --- a/spec/lib/gitlab/import_export/version_checker_spec.rb +++ b/spec/lib/gitlab/import_export/version_checker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' include ImportExport::CommonUtil describe Gitlab::ImportExport::VersionChecker do - let(:shared) { Gitlab::ImportExport::Shared.new(nil) } + let!(:shared) { Gitlab::ImportExport::Shared.new(nil) } describe 'bundle a project Git repo' do let(:version) { Gitlab::ImportExport.version } diff --git a/spec/services/projects/import_error_filter_spec.rb b/spec/services/projects/import_error_filter_spec.rb new file mode 100644 index 00000000000..312b658de89 --- /dev/null +++ b/spec/services/projects/import_error_filter_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::ImportErrorFilter do + it 'filters any full paths' do + message = 'Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file' + + expect(described_class.filter_message(message)).to eq('Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]') + end + + it 'filters any relative paths ignoring single slash ones' do + message = 'Error importing into my/project Permission denied @ unlink_internal - ../file/ and folder/../file' + + expect(described_class.filter_message(message)).to eq('Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED] and [FILTERED]') + end +end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 06f865dc848..a005defb3ee 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -136,12 +136,12 @@ describe Projects::ImportService do end it 'fails if repository import fails' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository /a/b/c')) result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository" + expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository [FILTERED]" end context 'when repository import scheduled' do