From ebf16ada856efb85424a98848c141f21e609886a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Mon, 4 Mar 2019 18:36:34 +0000 Subject: Arbitrary file read via MergeRequestDiff --- app/models/merge_request.rb | 2 +- app/models/merge_request_diff.rb | 2 ++ app/validators/sha_validator.rb | 9 +++++ .../security-fj-diff-import-file-read-fix.yml | 5 +++ lib/gitlab/import_export/merge_request_parser.rb | 11 ++++++ .../merge_request/user_sees_versions_spec.rb | 6 +++- .../import_export/merge_request_parser_spec.rb | 16 +++++++++ spec/models/merge_request_diff_spec.rb | 14 +++++++- spec/validators/sha_validator_spec.rb | 40 ++++++++++++++++++++++ ..._head_pipeline_for_merge_request_worker_spec.rb | 2 +- 10 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 app/validators/sha_validator.rb create mode 100644 changelogs/unreleased/security-fj-diff-import-file-read-fix.yml create mode 100644 spec/validators/sha_validator_spec.rb diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1468ae1c34a..ad4d5be48d4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -71,7 +71,7 @@ class MergeRequest < ActiveRecord::Base serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize - after_create :ensure_merge_request_diff, unless: :importing? + after_create :ensure_merge_request_diff after_update :clear_memoized_shas after_update :reload_diff_if_branch_changed after_save :ensure_metrics diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index e286a4e57f2..351a662ae83 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -22,6 +22,8 @@ class MergeRequestDiff < ActiveRecord::Base has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) } + validates :base_commit_sha, :head_commit_sha, :start_commit_sha, sha: true + state_machine :state, initial: :empty do event :clean do transition any => :without_files diff --git a/app/validators/sha_validator.rb b/app/validators/sha_validator.rb new file mode 100644 index 00000000000..085fca4d65d --- /dev/null +++ b/app/validators/sha_validator.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class ShaValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return if value.blank? || value.match(/\A\h{40}\z/) + + record.errors.add(attribute, 'is not a valid SHA') + end +end diff --git a/changelogs/unreleased/security-fj-diff-import-file-read-fix.yml b/changelogs/unreleased/security-fj-diff-import-file-read-fix.yml new file mode 100644 index 00000000000..e98d4e89712 --- /dev/null +++ b/changelogs/unreleased/security-fj-diff-import-file-read-fix.yml @@ -0,0 +1,5 @@ +--- +title: Fix arbitrary file read via diffs during import +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/merge_request_parser.rb b/lib/gitlab/import_export/merge_request_parser.rb index 040a70d6775..deb2f59f05f 100644 --- a/lib/gitlab/import_export/merge_request_parser.rb +++ b/lib/gitlab/import_export/merge_request_parser.rb @@ -20,6 +20,17 @@ module Gitlab create_target_branch unless branch_exists?(@merge_request.target_branch) end + # The merge_request_diff associated with the current @merge_request might + # be invalid. Than means, when the @merge_request object is saved, the + # @merge_request.merge_request_diff won't. This can leave the merge request + # in an invalid state, because a merge request must have an associated + # merge request diff. + # In this change, if the associated merge request diff is invalid, we set + # it to nil. This change, in association with the after callback + # :ensure_merge_request_diff in the MergeRequest class, makes that + # when the merge request is going to be created and it doesn't have + # one, a default one will be generated. + @merge_request.merge_request_diff = nil unless @merge_request.merge_request_diff&.valid? @merge_request end diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb index aa91ade46ca..5c45e363997 100644 --- a/spec/features/merge_request/user_sees_versions_spec.rb +++ b/spec/features/merge_request/user_sees_versions_spec.rb @@ -1,7 +1,11 @@ require 'rails_helper' describe 'Merge request > User sees versions', :js do - let(:merge_request) { create(:merge_request, importing: true) } + let(:merge_request) do + create(:merge_request).tap do |mr| + mr.merge_request_diff.destroy + end + end let(:project) { merge_request.source_project } let(:user) { project.creator } let!(:merge_request_diff1) { merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') } diff --git a/spec/lib/gitlab/import_export/merge_request_parser_spec.rb b/spec/lib/gitlab/import_export/merge_request_parser_spec.rb index 68eaa70e6b6..4b234411a44 100644 --- a/spec/lib/gitlab/import_export/merge_request_parser_spec.rb +++ b/spec/lib/gitlab/import_export/merge_request_parser_spec.rb @@ -41,4 +41,20 @@ describe Gitlab::ImportExport::MergeRequestParser do expect(parsed_merge_request).to eq(merge_request) end + + context 'when the merge request has diffs' do + let(:merge_request) do + build(:merge_request, source_project: forked_project, target_project: project) + end + + context 'when the diff is invalid' do + let(:merge_request_diff) { build(:merge_request_diff, merge_request: merge_request, base_commit_sha: 'foobar') } + + it 'sets the diff to nil' do + expect(merge_request_diff).to be_invalid + expect(merge_request_diff.merge_request).to eq merge_request + expect(parsed_merge_request.merge_request_diff).to be_nil + end + end + end end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 1849d3bac12..e530e0302f5 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -3,6 +3,18 @@ require 'spec_helper' describe MergeRequestDiff do let(:diff_with_commits) { create(:merge_request).merge_request_diff } + describe 'validations' do + subject { diff_with_commits } + + it 'checks sha format of base_commit_sha, head_commit_sha and start_commit_sha' do + subject.base_commit_sha = subject.head_commit_sha = subject.start_commit_sha = 'foobar' + + expect(subject.valid?).to be false + expect(subject.errors.count).to eq 3 + expect(subject.errors).to all(include('is not a valid SHA')) + end + end + describe 'create new record' do subject { diff_with_commits } @@ -78,7 +90,7 @@ describe MergeRequestDiff do it 'returns persisted diffs if cannot compare with diff refs' do expect(diff).to receive(:load_diffs).and_call_original - diff.update!(head_commit_sha: 'invalid-sha') + diff.update!(head_commit_sha: Digest::SHA1.hexdigest(SecureRandom.hex)) diff.diffs.diff_files end diff --git a/spec/validators/sha_validator_spec.rb b/spec/validators/sha_validator_spec.rb new file mode 100644 index 00000000000..b9242ef931e --- /dev/null +++ b/spec/validators/sha_validator_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe ShaValidator do + let(:validator) { described_class.new(attributes: [:base_commit_sha]) } + let(:merge_diff) { build(:merge_request_diff) } + + subject { validator.validate_each(merge_diff, :base_commit_sha, value) } + + context 'with empty value' do + let(:value) { nil } + + it 'does not add any error if value is empty' do + subject + + expect(merge_diff.errors).to be_empty + end + end + + context 'with valid sha' do + let(:value) { Digest::SHA1.hexdigest(SecureRandom.hex) } + + it 'does not add any error if value is empty' do + subject + + expect(merge_diff.errors).to be_empty + end + end + + context 'with invalid sha' do + let(:value) { 'foo' } + + it 'adds error to the record' do + expect(merge_diff.errors).to be_empty + + subject + + expect(merge_diff.errors).not_to be_empty + end + end +end diff --git a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb index 963237ceadf..f29e49f202a 100644 --- a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb +++ b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb @@ -18,7 +18,7 @@ describe UpdateHeadPipelineForMergeRequestWorker do context 'when merge request sha does not equal pipeline sha' do before do - merge_request.merge_request_diff.update(head_commit_sha: 'different_sha') + merge_request.merge_request_diff.update(head_commit_sha: Digest::SHA1.hexdigest(SecureRandom.hex)) end it 'does not update head pipeline' do -- cgit v1.2.3