diff options
| -rw-r--r-- | changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml | 5 | ||||
| -rw-r--r-- | lib/banzai/filter/autolink_filter.rb | 11 | ||||
| -rw-r--r-- | lib/banzai/filter/base_sanitization_filter.rb | 32 | ||||
| -rw-r--r-- | lib/banzai/filter/sanitization_filter.rb | 2 | ||||
| -rw-r--r-- | lib/banzai/filter/wiki_link_filter.rb | 15 | ||||
| -rw-r--r-- | lib/banzai/filter/wiki_link_filter/rewriter.rb | 8 | ||||
| -rw-r--r-- | lib/gitlab/utils/sanitize_node_link.rb | 61 | ||||
| -rw-r--r-- | spec/lib/banzai/filter/wiki_link_filter_spec.rb | 42 | ||||
| -rw-r--r-- | spec/lib/banzai/pipeline/wiki_pipeline_spec.rb | 79 | ||||
| -rw-r--r-- | spec/lib/gitlab/utils/sanitize_node_link_spec.rb | 72 |
10 files changed, 235 insertions, 92 deletions
diff --git a/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml b/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml new file mode 100644 index 00000000000..a8a26d5fc56 --- /dev/null +++ b/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml @@ -0,0 +1,5 @@ +--- +title: Patch XSS issue in wiki links +merge_request: +author: +type: security diff --git a/lib/banzai/filter/autolink_filter.rb b/lib/banzai/filter/autolink_filter.rb index 56214043d87..5f2cbc24c60 100644 --- a/lib/banzai/filter/autolink_filter.rb +++ b/lib/banzai/filter/autolink_filter.rb @@ -18,6 +18,7 @@ module Banzai # class AutolinkFilter < HTML::Pipeline::Filter include ActionView::Helpers::TagHelper + include Gitlab::Utils::SanitizeNodeLink # Pattern to match text that should be autolinked. # @@ -72,19 +73,11 @@ module Banzai private - # Return true if any of the UNSAFE_PROTOCOLS strings are included in the URI scheme - def contains_unsafe?(scheme) - return false unless scheme - - scheme = scheme.strip.downcase - Banzai::Filter::SanitizationFilter::UNSAFE_PROTOCOLS.any? { |protocol| scheme.include?(protocol) } - end - def autolink_match(match) # start by stripping out dangerous links begin uri = Addressable::URI.parse(match) - return match if contains_unsafe?(uri.scheme) + return match unless safe_protocol?(uri.scheme) rescue Addressable::URI::InvalidURIError return match end diff --git a/lib/banzai/filter/base_sanitization_filter.rb b/lib/banzai/filter/base_sanitization_filter.rb index 420e92cb1e8..2dabca3552d 100644 --- a/lib/banzai/filter/base_sanitization_filter.rb +++ b/lib/banzai/filter/base_sanitization_filter.rb @@ -11,6 +11,7 @@ module Banzai # Extends HTML::Pipeline::SanitizationFilter with common rules. class BaseSanitizationFilter < HTML::Pipeline::SanitizationFilter include Gitlab::Utils::StrongMemoize + extend Gitlab::Utils::SanitizeNodeLink UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze @@ -40,7 +41,7 @@ module Banzai # Allow any protocol in `a` elements # and then remove links with unsafe protocols whitelist[:protocols].delete('a') - whitelist[:transformers].push(self.class.remove_unsafe_links) + whitelist[:transformers].push(self.class.method(:remove_unsafe_links)) # Remove `rel` attribute from `a` elements whitelist[:transformers].push(self.class.remove_rel) @@ -54,35 +55,6 @@ module Banzai end class << self - def remove_unsafe_links - lambda do |env| - node = env[:node] - - return unless node.name == 'a' - return unless node.has_attribute?('href') - - begin - node['href'] = node['href'].strip - uri = Addressable::URI.parse(node['href']) - - return unless uri.scheme - - # Remove all invalid scheme characters before checking against the - # list of unsafe protocols. - # - # See https://tools.ietf.org/html/rfc3986#section-3.1 - scheme = uri.scheme - .strip - .downcase - .gsub(/[^A-Za-z0-9\+\.\-]+/, '') - - node.remove_attribute('href') if UNSAFE_PROTOCOLS.include?(scheme) - rescue Addressable::URI::InvalidURIError - node.remove_attribute('href') - end - end - end - def remove_rel lambda do |env| if env[:node_name] == 'a' diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index f57e57890f8..ca6e6541cbb 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -7,6 +7,8 @@ module Banzai # Extends Banzai::Filter::BaseSanitizationFilter with specific rules. class SanitizationFilter < Banzai::Filter::BaseSanitizationFilter # Styles used by Markdown for table alignment + include Gitlab::Utils::StrongMemoize + TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/.freeze def customize_whitelist(whitelist) diff --git a/lib/banzai/filter/wiki_link_filter.rb b/lib/banzai/filter/wiki_link_filter.rb index 1728a442533..18947679b69 100644 --- a/lib/banzai/filter/wiki_link_filter.rb +++ b/lib/banzai/filter/wiki_link_filter.rb @@ -8,15 +8,19 @@ module Banzai # Context options: # :project_wiki class WikiLinkFilter < HTML::Pipeline::Filter + include Gitlab::Utils::SanitizeNodeLink + def call return doc unless project_wiki? - doc.search('a:not(.gfm)').each { |el| process_link_attr(el.attribute('href')) } - doc.search('video').each { |el| process_link_attr(el.attribute('src')) } + doc.search('a:not(.gfm)').each { |el| process_link(el.attribute('href'), el) } + + doc.search('video').each { |el| process_link(el.attribute('src'), el) } + doc.search('img').each do |el| attr = el.attribute('data-src') || el.attribute('src') - process_link_attr(attr) + process_link(attr, el) end doc @@ -24,6 +28,11 @@ module Banzai protected + def process_link(link_attr, node) + process_link_attr(link_attr) + remove_unsafe_links({ node: node }, remove_invalid_links: false) + end + def project_wiki? !context[:project_wiki].nil? end diff --git a/lib/banzai/filter/wiki_link_filter/rewriter.rb b/lib/banzai/filter/wiki_link_filter/rewriter.rb index 77b5053f38c..f4cc8beeb52 100644 --- a/lib/banzai/filter/wiki_link_filter/rewriter.rb +++ b/lib/banzai/filter/wiki_link_filter/rewriter.rb @@ -4,8 +4,6 @@ module Banzai module Filter class WikiLinkFilter < HTML::Pipeline::Filter class Rewriter - UNSAFE_SLUG_REGEXES = [/\Ajavascript:/i].freeze - def initialize(link_string, wiki:, slug:) @uri = Addressable::URI.parse(link_string) @wiki_base_path = wiki && wiki.wiki_base_path @@ -37,8 +35,6 @@ module Banzai # Of the form `./link`, `../link`, or similar def apply_hierarchical_link_rules! - return if slug_considered_unsafe? - @uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.' end @@ -58,10 +54,6 @@ module Banzai def repository_upload? @uri.relative? && @uri.path.starts_with?(Wikis::CreateAttachmentService::ATTACHMENT_PATH) end - - def slug_considered_unsafe? - UNSAFE_SLUG_REGEXES.any? { |r| r.match?(@slug) } - end end end end diff --git a/lib/gitlab/utils/sanitize_node_link.rb b/lib/gitlab/utils/sanitize_node_link.rb new file mode 100644 index 00000000000..620d71a7814 --- /dev/null +++ b/lib/gitlab/utils/sanitize_node_link.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require_dependency 'gitlab/utils' + +module Gitlab + module Utils + module SanitizeNodeLink + UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze + ATTRS_TO_SANITIZE = %w(href src data-src).freeze + + def remove_unsafe_links(env, remove_invalid_links: true) + node = env[:node] + + sanitize_node(node: node, remove_invalid_links: remove_invalid_links) + + # HTML entities such as <video></video> have scannable attrs in + # children elements, which also need to be sanitized. + # + node.children.each do |child_node| + sanitize_node(node: child_node, remove_invalid_links: remove_invalid_links) + end + end + + # Remove all invalid scheme characters before checking against the + # list of unsafe protocols. + # + # See https://tools.ietf.org/html/rfc3986#section-3.1 + # + def safe_protocol?(scheme) + return false unless scheme + + scheme = scheme + .strip + .downcase + .gsub(/[^A-Za-z\+\.\-]+/, '') + + UNSAFE_PROTOCOLS.none?(scheme) + end + + private + + def sanitize_node(node:, remove_invalid_links: true) + ATTRS_TO_SANITIZE.each do |attr| + next unless node.has_attribute?(attr) + + begin + node[attr] = node[attr].strip + uri = Addressable::URI.parse(node[attr]) + + next unless uri.scheme + next if safe_protocol?(uri.scheme) + + node.remove_attribute(attr) + rescue Addressable::URI::InvalidURIError + node.remove_attribute(attr) if remove_invalid_links + end + end + end + end + end +end diff --git a/spec/lib/banzai/filter/wiki_link_filter_spec.rb b/spec/lib/banzai/filter/wiki_link_filter_spec.rb index cce1cd0b284..b9059b85fdc 100644 --- a/spec/lib/banzai/filter/wiki_link_filter_spec.rb +++ b/spec/lib/banzai/filter/wiki_link_filter_spec.rb @@ -70,47 +70,5 @@ describe Banzai::Filter::WikiLinkFilter do expect(filtered_link.attribute('href').value).to eq(invalid_link) 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:", - "javascript:", - "javascript:", - "javascript:", - "java\0script:", - "  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 diff --git a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb index 64ca3ec345d..95c0c9f9a17 100644 --- a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb @@ -177,6 +177,85 @@ describe Banzai::Pipeline::WikiPipeline do end end end + + describe "checking slug validity when assembling links" do + context "with a valid slug" do + let(:valid_slug) { "http://example.com" } + + it "includes the slug in a (.) relative link" do + output = described_class.to_html( + "[Link](./alert(1);)", + project: project, + project_wiki: project_wiki, + page_slug: valid_slug + ) + + expect(output).to include(valid_slug) + end + + it "includeds the slug in a (..) relative link" do + output = described_class.to_html( + "[Link](../alert(1);)", + project: project, + project_wiki: project_wiki, + page_slug: valid_slug + ) + + expect(output).to include(valid_slug) + end + end + + context "when the slug is deemed unsafe or invalid" do + invalid_slugs = [ + "javascript:", + "JaVaScRiPt:", + "\u0001java\u0003script:", + "javascript :", + "javascript: ", + "javascript : ", + ":javascript:", + "javascript:", + "javascript:", + "javascript:", + "javascript:", + "java\0script:", + "  javascript:" + ] + + invalid_js_links = [ + "alert(1);", + "alert(document.location);" + ] + + invalid_slugs.each do |slug| + context "with the invalid slug #{slug}" do + invalid_js_links.each do |link| + it "doesn't include a prohibited slug in a (.) relative link '#{link}'" do + output = described_class.to_html( + "[Link](./#{link})", + project: project, + project_wiki: project_wiki, + page_slug: slug + ) + + expect(output).not_to include(slug) + end + + it "doesn't include a prohibited slug in a (..) relative link '#{link}'" do + output = described_class.to_html( + "[Link](../#{link})", + project: project, + project_wiki: project_wiki, + page_slug: slug + ) + + expect(output).not_to include(slug) + end + end + end + end + end + end end describe 'videos' do diff --git a/spec/lib/gitlab/utils/sanitize_node_link_spec.rb b/spec/lib/gitlab/utils/sanitize_node_link_spec.rb new file mode 100644 index 00000000000..064c2707d06 --- /dev/null +++ b/spec/lib/gitlab/utils/sanitize_node_link_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' + +describe Gitlab::Utils::SanitizeNodeLink do + let(:klass) do + struct = Struct.new(:value) + struct.include(described_class) + + struct + end + + subject(:object) { klass.new(:value) } + + invalid_schemes = [ + "javascript:", + "JaVaScRiPt:", + "\u0001java\u0003script:", + "javascript :", + "javascript: ", + "javascript : ", + ":javascript:", + "javascript:", + "javascript:", + "  javascript:" + ] + + invalid_schemes.each do |scheme| + context "with the scheme: #{scheme}" do + describe "#remove_unsafe_links" do + tags = { + a: { + doc: HTML::Pipeline.parse("<a href='#{scheme}alert(1);'>foo</a>"), + attr: "href", + node_to_check: -> (doc) { doc.children.first } + }, + img: { + doc: HTML::Pipeline.parse("<img src='#{scheme}alert(1);'>"), + attr: "src", + node_to_check: -> (doc) { doc.children.first } + }, + video: { + doc: HTML::Pipeline.parse("<video><source src='#{scheme}alert(1);'></video>"), + attr: "src", + node_to_check: -> (doc) { doc.children.first.children.filter("source").first } + } + } + + tags.each do |tag, opts| + context "<#{tag}> tags" do + it "removes the unsafe link" do + node = opts[:node_to_check].call(opts[:doc]) + + expect { object.remove_unsafe_links({ node: node }, remove_invalid_links: true) } + .to change { node[opts[:attr]] } + + expect(node[opts[:attr]]).to be_blank + end + end + end + end + + describe "#safe_protocol?" do + let(:doc) { HTML::Pipeline.parse("<a href='#{scheme}alert(1);'>foo</a>") } + let(:node) { doc.children.first } + let(:uri) { Addressable::URI.parse(node['href'])} + + it "returns false" do + expect(object.safe_protocol?(scheme)).to be_falsy + end + end + end + end +end |