Commit a2c767b9 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq

parents 96ff9c6b 5906fb2e
---
title: Prevent XSS injection in note imports
merge_request:
author:
type: security
---
title: Filter relative links in wiki for XSS
merge_request:
author:
type: security
...@@ -4,6 +4,8 @@ module Banzai ...@@ -4,6 +4,8 @@ module Banzai
module Filter module Filter
class WikiLinkFilter < HTML::Pipeline::Filter class WikiLinkFilter < HTML::Pipeline::Filter
class Rewriter class Rewriter
UNSAFE_SLUG_REGEXES = [/\Ajavascript:/i].freeze
def initialize(link_string, wiki:, slug:) def initialize(link_string, wiki:, slug:)
@uri = Addressable::URI.parse(link_string) @uri = Addressable::URI.parse(link_string)
@wiki_base_path = wiki && wiki.wiki_base_path @wiki_base_path = wiki && wiki.wiki_base_path
...@@ -35,6 +37,8 @@ module Banzai ...@@ -35,6 +37,8 @@ module Banzai
# Of the form `./link`, `../link`, or similar # Of the form `./link`, `../link`, or similar
def apply_hierarchical_link_rules! def apply_hierarchical_link_rules!
return if slug_considered_unsafe?
@uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.' @uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.'
end end
...@@ -54,6 +58,10 @@ module Banzai ...@@ -54,6 +58,10 @@ module Banzai
def repository_upload? def repository_upload?
@uri.relative? && @uri.path.starts_with?(Wikis::CreateAttachmentService::ATTACHMENT_PATH) @uri.relative? && @uri.path.starts_with?(Wikis::CreateAttachmentService::ATTACHMENT_PATH)
end end
def slug_considered_unsafe?
UNSAFE_SLUG_REGEXES.any? { |r| r.match?(@slug) }
end
end end
end end
end end
......
...@@ -4,6 +4,7 @@ module Gitlab ...@@ -4,6 +4,7 @@ module Gitlab
module ImportExport module ImportExport
class AttributeCleaner class AttributeCleaner
ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id'] ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + ['group_id']
PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_html\Z/).freeze
def self.clean(*args) def self.clean(*args)
new(*args).clean new(*args).clean
...@@ -24,7 +25,11 @@ module Gitlab ...@@ -24,7 +25,11 @@ module Gitlab
private private
def prohibited_key?(key) def prohibited_key?(key)
key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key) key =~ PROHIBITED_REFERENCES && !permitted_key?(key)
end
def permitted_key?(key)
ALLOWED_REFERENCES.include?(key)
end end
def excluded_key?(key) def excluded_key?(key)
......
...@@ -12,7 +12,7 @@ describe 'Import/Export - project export integration test', :js do ...@@ -12,7 +12,7 @@ describe 'Import/Export - project export integration test', :js do
let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } let(:export_path) { "#{Dir.tmpdir}/import_file_spec" }
let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } 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 let(:safe_list) do
{ {
token: [ProjectHook, Ci::Trigger, CommitStatus], token: [ProjectHook, Ci::Trigger, CommitStatus],
......
...@@ -70,5 +70,47 @@ describe Banzai::Filter::WikiLinkFilter do ...@@ -70,5 +70,47 @@ describe Banzai::Filter::WikiLinkFilter do
expect(filtered_link.attribute('href').value).to eq(invalid_link) expect(filtered_link.attribute('href').value).to eq(invalid_link)
end end
end end
context "when the slug is deemed unsafe or invalid" do
let(:link) { "alert(1);" }
invalid_slugs = [
"javascript:",
"JaVaScRiPt:",
"\u0001java\u0003script:",
"javascript :",
"javascript: ",
"javascript : ",
":javascript:",
"javascript&#58;",
"javascript&#0058;",
"javascript&#x3A;",
"javascript&#x003A;",
"java\0script:",
" &#14; javascript:"
]
invalid_slugs.each do |slug|
context "with the slug #{slug}" do
it "doesn't rewrite a (.) relative link" do
filtered_link = filter(
"<a href='.#{link}'>Link</a>",
project_wiki: wiki,
page_slug: slug).children[0]
expect(filtered_link.attribute('href').value).not_to include(slug)
end
it "doesn't rewrite a (..) relative link" do
filtered_link = filter(
"<a href='..#{link}'>Link</a>",
project_wiki: wiki,
page_slug: slug).children[0]
expect(filtered_link.attribute('href').value).not_to include(slug)
end
end
end
end
end end
end end
...@@ -18,7 +18,11 @@ describe Gitlab::ImportExport::AttributeCleaner do ...@@ -18,7 +18,11 @@ describe Gitlab::ImportExport::AttributeCleaner do
'notid' => 99, 'notid' => 99,
'import_source' => 'whatever', 'import_source' => 'whatever',
'import_type' => 'whatever', 'import_type' => 'whatever',
'non_existent_attr' => 'whatever' 'non_existent_attr' => 'whatever',
'some_html' => '<p>dodgy html</p>',
'legit_html' => '<p>legit html</p>',
'_html' => '<p>perfectly ordinary html</p>',
'cached_markdown_version' => 12345
} }
end end
......
...@@ -158,6 +158,8 @@ ...@@ -158,6 +158,8 @@
{ {
"id": 351, "id": 351,
"note": "Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi.", "note": "Quo reprehenderit aliquam qui dicta impedit cupiditate eligendi.",
"note_html": "<p>something else entirely</p>",
"cached_markdown_version": 917504,
"noteable_type": "Issue", "noteable_type": "Issue",
"author_id": 26, "author_id": 26,
"created_at": "2016-06-14T15:02:47.770Z", "created_at": "2016-06-14T15:02:47.770Z",
...@@ -2363,6 +2365,8 @@ ...@@ -2363,6 +2365,8 @@
{ {
"id": 671, "id": 671,
"note": "Sit voluptatibus eveniet architecto quidem.", "note": "Sit voluptatibus eveniet architecto quidem.",
"note_html": "<p>something else entirely</p>",
"cached_markdown_version": 917504,
"noteable_type": "MergeRequest", "noteable_type": "MergeRequest",
"author_id": 26, "author_id": 26,
"created_at": "2016-06-14T15:02:56.632Z", "created_at": "2016-06-14T15:02:56.632Z",
......
...@@ -58,6 +58,26 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do ...@@ -58,6 +58,26 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
expect(Milestone.find_by_description('test milestone').issues.count).to eq(2) expect(Milestone.find_by_description('test milestone').issues.count).to eq(2)
end end
context 'when importing a project with cached_markdown_version and note_html' do
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
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
it 'creates a valid pipeline note' do it 'creates a valid pipeline note' do
expect(Ci::Pipeline.find_by_sha('sha-notes').notes).not_to be_empty expect(Ci::Pipeline.find_by_sha('sha-notes').notes).not_to be_empty
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment