From 727f0ede46301a52f404cc3bdc2c33f4fbb9fb78 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Tue, 2 Jul 2019 18:01:55 -0700 Subject: Extract SanitizeNodeLink and apply to WikiLinkFilter The SanitizationFilter was running before the WikiFilter. Since WikiFilter can modify links, we could see links that _should_ be stopped by SanatizationFilter being rendered on the page. I (kerrizor) had previously addressed the bug in: https://gitlab.com/gitlab-org/gitlab-ee/commit/7bc971915bbeadb950bb0e1f13510bf3038229a4 However, an additional exploit was discovered after that was merged. Working through the issue, we couldn't simply shuffle the order of filters, due to some implicit assumptions about the order of filters, so instead we've extracted the logic that sanitizes a Nokogiri-generated Node object, and applied it to the WikiLinkFilter as well. On moving filters around: Once we start moving around filters, we get cascading failures; fix one, another one crops up. Many of the existing filters in the WikiPipeline chain seem to assume that other filters have already done their work, and thus operate on a "transform anything that's left" basis; WikiFilter, for instance, assumes any link it finds in the markdown should be prepended with the wiki_base_path.. but if it does that, it also turns `href="@user"` into `href="/path/to/wiki/@user"`, which the UserReferenceFilter doesn't see as a user reference it needs to transform into a user profile link. This is true for all the reference filters in the WikiPipeline. --- ...-60143-patch-additional-xss-vector-in-wikis.yml | 5 ++ lib/banzai/filter/autolink_filter.rb | 11 +-- lib/banzai/filter/sanitization_filter.rb | 33 +-------- lib/banzai/filter/wiki_link_filter.rb | 15 +++- lib/banzai/filter/wiki_link_filter/rewriter.rb | 8 --- lib/gitlab/utils/sanitize_node_link.rb | 61 +++++++++++++++++ spec/lib/banzai/filter/wiki_link_filter_spec.rb | 42 ------------ spec/lib/banzai/pipeline/wiki_pipeline_spec.rb | 79 ++++++++++++++++++++++ spec/lib/gitlab/utils/sanitize_node_link_spec.rb | 72 ++++++++++++++++++++ 9 files changed, 233 insertions(+), 93 deletions(-) create mode 100644 changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml create mode 100644 lib/gitlab/utils/sanitize_node_link.rb create mode 100644 spec/lib/gitlab/utils/sanitize_node_link_spec.rb 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/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index a4a06eae7b7..8c1b8d7da27 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -7,8 +7,8 @@ module Banzai # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist. class SanitizationFilter < HTML::Pipeline::SanitizationFilter include Gitlab::Utils::StrongMemoize + extend Gitlab::Utils::SanitizeNodeLink - UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze TABLE_ALIGNMENT_PATTERN = /text-align: (?center|left|right)/.freeze def whitelist @@ -51,7 +51,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) @@ -69,35 +69,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/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 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( - "Link", - 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( - "Link", - 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("foo"), + attr: "href", + node_to_check: -> (doc) { doc.children.first } + }, + img: { + doc: HTML::Pipeline.parse(""), + attr: "src", + node_to_check: -> (doc) { doc.children.first } + }, + video: { + doc: HTML::Pipeline.parse(""), + 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("foo") } + 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 -- cgit v1.2.3