From 802af09bdc222e0f3b164124f12a56a53dbe8675 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 24 Apr 2019 14:31:20 +1200 Subject: [PATCH 1/9] Add disallowed fields to AttributeCleaner --- lib/gitlab/import_export/attribute_cleaner.rb | 14 ++++++++++++- .../import_export/attribute_cleaner_spec.rb | 6 +++++- .../project_tree_restorer_spec.rb | 20 +++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 93b37b7bc5f..1faa3c1614f 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -24,7 +24,19 @@ module Gitlab private def prohibited_key?(key) - key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key) + return false if allowed_reference?(key) + + return true if 'cached_markdown_version'.equal?(key) + + prohibited_suffices = %w(_id _html) + prohibited_suffices.each do |suffix| + return true if key.end_with?(suffix) + end + false + end + + def allowed_reference?(key) + ALLOWED_REFERENCES.include?(key) end def excluded_key?(key) diff --git a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb index 536cc359d39..99669285d5b 100644 --- a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb +++ b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb @@ -18,7 +18,11 @@ describe Gitlab::ImportExport::AttributeCleaner do 'notid' => 99, 'import_source' => 'whatever', 'import_type' => 'whatever', - 'non_existent_attr' => 'whatever' + 'non_existent_attr' => 'whatever', + 'some_html' => '

dodgy html

', + 'legit_html' => '

legit html

', + '_html' => '

perfectly ordinary html

', + 'cached_markdown_version' => 12345 } end diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 6084dc96410..be090bfed25 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -10,6 +10,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do create(:user, username: 'bernard_willms'), create(:user, username: 'saul_will') ] + @markdown_classes = [AbuseReport, Appearance, ApplicationSetting, BroadcastMessage, Issue, Label, MergeRequest, Milestone, Namespace, Project, Release, ResourceLabelEvent, Snippet, UserStatus] RSpec::Mocks.with_temporary_scope do @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @@ -21,6 +22,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch) + @markdown_classes.each {|klass| allow_any_instance_of(klass).to receive(:latest_cached_markdown_version).and_return(434343)} project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) @@ -58,6 +60,24 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect(Milestone.find_by_description('test milestone').issues.count).to eq(2) end + context 'when importing a project with cached_markdown_version and note_html' do + let!(:issue) { Issue.find_by(description: 'Aliquam enim illo et possimus.') } + let(:note1) { issue.notes.select {|n| n.note.match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/)}.first } + let(:note2) { issue.notes.select {|n| n.note.match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/)}.first } + + it 'does not import the note_html' do + expect(note1.note_html).to match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/) + end + + it 'does not set the old cached_markdown_version' do + expect(note2.cached_markdown_version).not_to eq(121212) + end + + it 'does not import the note_html' do + expect(note2.note_html).to match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/) + end + end + it 'creates a valid pipeline note' do expect(Ci::Pipeline.find_by_sha('sha-notes').notes).not_to be_empty end -- GitLab From 92388917aa05e2b8eba5c9906b168140399bbd8d Mon Sep 17 00:00:00 2001 From: charlieablett Date: Fri, 26 Apr 2019 09:40:00 +1200 Subject: [PATCH 2/9] Tighten up prohibited_key method --- lib/gitlab/import_export/attribute_cleaner.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 1faa3c1614f..f476905b6db 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -28,11 +28,10 @@ module Gitlab return true if 'cached_markdown_version'.equal?(key) - prohibited_suffices = %w(_id _html) - prohibited_suffices.each do |suffix| - return true if key.end_with?(suffix) + prohibited_suffixes = %w(_id _html) + prohibited_suffixes.any? do |suffix| + true if key.end_with?(suffix) end - false end def allowed_reference?(key) -- GitLab From 32e24b65e8da361c9ad77bc26228980c0965b3d3 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 30 Apr 2019 17:07:51 +1200 Subject: [PATCH 3/9] Refactor AttributeCleaner` for readability --- lib/gitlab/import_export/attribute_cleaner.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index f476905b6db..b27a2a9fb65 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -24,14 +24,9 @@ module Gitlab private def prohibited_key?(key) - return false if allowed_reference?(key) + return false if permitted_key?(key) - return true if 'cached_markdown_version'.equal?(key) - - prohibited_suffixes = %w(_id _html) - prohibited_suffixes.any? do |suffix| - true if key.end_with?(suffix) - end + 'cached_markdown_version' == key || PROHIBITED_SUFFIXES.any? {|suffix| key.end_with?(suffix)} end def allowed_reference?(key) -- GitLab From a46e2de902b8756feb0d3db6396811a045b9c01c Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 30 Apr 2019 17:18:33 +1200 Subject: [PATCH 4/9] Refactor AttributeCleaner` for readability --- lib/gitlab/import_export/attribute_cleaner.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index b27a2a9fb65..76d0053996d 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -4,6 +4,7 @@ module Gitlab module ImportExport class AttributeCleaner ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id'] + PROHIBITED_SUFFIXES = %w(_id _html).freeze def self.clean(*args) new(*args).clean @@ -29,7 +30,7 @@ module Gitlab 'cached_markdown_version' == key || PROHIBITED_SUFFIXES.any? {|suffix| key.end_with?(suffix)} end - def allowed_reference?(key) + def permitted_key?(key) ALLOWED_REFERENCES.include?(key) end @@ -40,4 +41,4 @@ module Gitlab end end end -end +end \ No newline at end of file -- GitLab From 94db1bfbb98f47eb8710e7b3d55972e44cbf2d96 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 30 Apr 2019 17:27:28 +1200 Subject: [PATCH 5/9] Add newline to AttributeCleaner --- lib/gitlab/import_export/attribute_cleaner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 76d0053996d..7f67f63f26b 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -41,4 +41,4 @@ module Gitlab end end end -end \ No newline at end of file +end -- GitLab From d900ece1f7fbde93b903ba7b798646458cdee832 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 30 Apr 2019 17:43:01 +1200 Subject: [PATCH 6/9] Ensure Issue & MR note_html cannot be imported --- spec/lib/gitlab/import_export/project.json | 4 +++ .../project_tree_restorer_spec.rb | 26 +++++++++---------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 4a7accc4c52..fb7bddb386c 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -158,6 +158,8 @@ { "id": 351, "note": "Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi.", + "note_html": "

something else entirely

", + "cached_markdown_version": 917504, "noteable_type": "Issue", "author_id": 26, "created_at": "2016-06-14T15:02:47.770Z", @@ -2363,6 +2365,8 @@ { "id": 671, "note": "Sit voluptatibus eveniet architecto quidem.", + "note_html": "

something else entirely

", + "cached_markdown_version": 917504, "noteable_type": "MergeRequest", "author_id": 26, "created_at": "2016-06-14T15:02:56.632Z", diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index be090bfed25..db4ea3d0675 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -10,7 +10,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do create(:user, username: 'bernard_willms'), create(:user, username: 'saul_will') ] - @markdown_classes = [AbuseReport, Appearance, ApplicationSetting, BroadcastMessage, Issue, Label, MergeRequest, Milestone, Namespace, Project, Release, ResourceLabelEvent, Snippet, UserStatus] RSpec::Mocks.with_temporary_scope do @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @@ -22,7 +21,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch) - @markdown_classes.each {|klass| allow_any_instance_of(klass).to receive(:latest_cached_markdown_version).and_return(434343)} project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project) @@ -61,20 +59,20 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end context 'when importing a project with cached_markdown_version and note_html' do - let!(:issue) { Issue.find_by(description: 'Aliquam enim illo et possimus.') } - let(:note1) { issue.notes.select {|n| n.note.match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/)}.first } - let(:note2) { issue.notes.select {|n| n.note.match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/)}.first } - - it 'does not import the note_html' do - expect(note1.note_html).to match(/Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi/) - end - - it 'does not set the old cached_markdown_version' do - expect(note2.cached_markdown_version).not_to eq(121212) + context 'for an Issue' do + it 'does not import note_html' do + note_content = 'Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi' + issue_note = Issue.find_by(description: 'Aliquam enim illo et possimus.').notes.select { |n| n.note.match(/#{note_content}/)}.first + expect(issue_note.note_html).to match(/#{note_content}/) + end end - it 'does not import the note_html' do - expect(note2.note_html).to match(/Est reprehenderit quas aut aspernatur autem recusandae voluptatem/) + context 'for a Merge Request' do + it 'does not import note_html' do + note_content = 'Sit voluptatibus eveniet architecto quidem' + merge_request_note = MergeRequest.find_by(title: 'MR1').notes.select { |n| n.note.match(/#{note_content}/)}.first + expect(merge_request_note.note_html).to match(/#{note_content}/) + end end end -- GitLab From fc55d7bc97b18f9a7b58e815493f2b3d926f6130 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Tue, 23 Apr 2019 21:46:26 +1200 Subject: [PATCH 7/9] Add changelog entry --- .../security-58856-persistent-xss-in-note-objects.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml diff --git a/changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml b/changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml new file mode 100644 index 00000000000..d9ad5af256a --- /dev/null +++ b/changelogs/unreleased/security-58856-persistent-xss-in-note-objects.yml @@ -0,0 +1,5 @@ +--- +title: Prevent XSS injection in note imports +merge_request: +author: +type: security -- GitLab From c267cc8064bbf1bec3f817e6290bd688e847af5f Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 1 May 2019 10:38:41 +1200 Subject: [PATCH 8/9] Add `html` to sensitive words --- lib/gitlab/import_export/attribute_cleaner.rb | 2 +- spec/features/projects/import_export/export_file_spec.rb | 2 +- spec/lib/gitlab/import_export/project_tree_restorer_spec.rb | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 7f67f63f26b..7bdef2b6cdb 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -4,7 +4,7 @@ module Gitlab module ImportExport class AttributeCleaner ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id'] - PROHIBITED_SUFFIXES = %w(_id _html).freeze + PROHIBITED_SUFFIXES = %w[_id _html].freeze def self.clean(*args) new(*args).clean diff --git a/spec/features/projects/import_export/export_file_spec.rb b/spec/features/projects/import_export/export_file_spec.rb index f76f9ba7577..9d74a96ab3d 100644 --- a/spec/features/projects/import_export/export_file_spec.rb +++ b/spec/features/projects/import_export/export_file_spec.rb @@ -12,7 +12,7 @@ describe 'Import/Export - project export integration test', :js do let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } - let(:sensitive_words) { %w[pass secret token key encrypted] } + let(:sensitive_words) { %w[pass secret token key encrypted html] } let(:safe_list) do { token: [ProjectHook, Ci::Trigger, CommitStatus], diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index db4ea3d0675..9d2b69ea798 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -63,6 +63,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it 'does not import note_html' do note_content = 'Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi' issue_note = Issue.find_by(description: 'Aliquam enim illo et possimus.').notes.select { |n| n.note.match(/#{note_content}/)}.first + expect(issue_note.note_html).to match(/#{note_content}/) end end @@ -71,6 +72,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it 'does not import note_html' do note_content = 'Sit voluptatibus eveniet architecto quidem' merge_request_note = MergeRequest.find_by(title: 'MR1').notes.select { |n| n.note.match(/#{note_content}/)}.first + expect(merge_request_note.note_html).to match(/#{note_content}/) end end -- GitLab From 7ea15f0d30d17da91e0f4840acaeb9721ed2a60a Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 1 May 2019 12:15:29 +1200 Subject: [PATCH 9/9] Change `prohibited_key` to use regexes --- lib/gitlab/import_export/attribute_cleaner.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index 7bdef2b6cdb..c28a1674018 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -4,7 +4,7 @@ module Gitlab module ImportExport class AttributeCleaner ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id'] - PROHIBITED_SUFFIXES = %w[_id _html].freeze + PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_html\Z/).freeze def self.clean(*args) new(*args).clean @@ -25,9 +25,7 @@ module Gitlab private def prohibited_key?(key) - return false if permitted_key?(key) - - 'cached_markdown_version' == key || PROHIBITED_SUFFIXES.any? {|suffix| key.end_with?(suffix)} + key =~ PROHIBITED_REFERENCES && !permitted_key?(key) end def permitted_key?(key) -- GitLab