summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml5
-rw-r--r--lib/banzai/filter/autolink_filter.rb11
-rw-r--r--lib/banzai/filter/base_sanitization_filter.rb32
-rw-r--r--lib/banzai/filter/wiki_link_filter.rb15
-rw-r--r--lib/banzai/filter/wiki_link_filter/rewriter.rb8
-rw-r--r--lib/gitlab/utils/sanitize_node_link.rb61
-rw-r--r--spec/lib/banzai/filter/wiki_link_filter_spec.rb42
-rw-r--r--spec/lib/banzai/pipeline/wiki_pipeline_spec.rb79
-rw-r--r--spec/lib/gitlab/utils/sanitize_node_link_spec.rb72
9 files changed, 233 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/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&#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
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&#58;",
+ "javascript&#0058;",
+ "javascript&#x3A;",
+ "javascript&#x003A;",
+ "java\0script:",
+ " &#14; 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&#58;",
+ "javascript&#0058;",
+ " &#14; 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