From e8c911577fe09b83793f7ffc95123642ab07668d Mon Sep 17 00:00:00 2001 From: Marius Balteanu Date: Wed, 11 Aug 2021 21:49:27 +0000 Subject: [PATCH] Relax allowed protocols in links by denying specific protocols for CommonMark text formatting (#32424). Patch by Martin Cizek. git-svn-id: http://svn.redmine.org/redmine/trunk@21161 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- lib/redmine/helpers/url.rb | 13 ++++++ .../common_mark/sanitization_filter.rb | 28 ++++++++++--- test/unit/lib/redmine/helpers/url_test.rb | 41 +++++++++++++++++++ .../common_mark/sanitization_filter_test.rb | 24 ++++++++--- 4 files changed, 96 insertions(+), 10 deletions(-) diff --git a/lib/redmine/helpers/url.rb b/lib/redmine/helpers/url.rb index 0c6cbdecd..6f1f853e4 100644 --- a/lib/redmine/helpers/url.rb +++ b/lib/redmine/helpers/url.rb @@ -22,6 +22,7 @@ require 'uri' module Redmine module Helpers module URL + # safe for resources fetched without user interaction? def uri_with_safe_scheme?(uri, schemes = ['http', 'https', 'ftp', 'mailto', nil]) # URLs relative to the current document or document root (without a protocol # separator, should be harmless @@ -32,6 +33,18 @@ module Redmine rescue URI::Error false end + + # safe to render links to given uri? + def uri_with_link_safe_scheme?(uri) + # regexp adapted from Sanitize (we need to catch even invalid protocol specs) + return true unless uri =~ /\A\s*([^\/#]*?)(?:\:|�*58|�*3a)/i + + # absolute scheme + scheme = $1.downcase + return false unless /\A[a-z][a-z0-9\+\.\-]*\z/.match?(scheme) # RFC 3986 + + %w(data javascript vbscript).none?(scheme) + end end end end diff --git a/lib/redmine/wiki_formatting/common_mark/sanitization_filter.rb b/lib/redmine/wiki_formatting/common_mark/sanitization_filter.rb index a76201dfd..df09fd9c8 100644 --- a/lib/redmine/wiki_formatting/common_mark/sanitization_filter.rb +++ b/lib/redmine/wiki_formatting/common_mark/sanitization_filter.rb @@ -22,6 +22,11 @@ module Redmine module CommonMark # sanitizes rendered HTML using the Sanitize gem class SanitizationFilter < HTML::Pipeline::SanitizationFilter + include Redmine::Helpers::URL + RELAXED_PROTOCOL_ATTRS = { + "a" => %w(href).freeze, + }.freeze + def whitelist @@whitelist ||= customize_whitelist(super.deep_dup) end @@ -72,11 +77,24 @@ module Redmine node.remove_attribute("id") } - # allow the same set of URL schemes for links as is the default in - # Redmine::Helpers::URL#uri_with_safe_scheme? - whitelist[:protocols]["a"]["href"] = [ - 'http', 'https', 'ftp', 'mailto', :relative - ] + # https://github.com/rgrove/sanitize/issues/209 + whitelist[:protocols].delete("a") + whitelist[:transformers].push lambda{|env| + node = env[:node] + return if node.type != Nokogiri::XML::Node::ELEMENT_NODE + + name = env[:node_name] + return unless RELAXED_PROTOCOL_ATTRS.include?(name) + + RELAXED_PROTOCOL_ATTRS[name].each do |attr| + next unless node.has_attribute?(attr) + + node[attr] = node[attr].strip + unless !node[attr].empty? && uri_with_link_safe_scheme?(node[attr]) + node.remove_attribute(attr) + end + end + } whitelist end diff --git a/test/unit/lib/redmine/helpers/url_test.rb b/test/unit/lib/redmine/helpers/url_test.rb index 013a7ecac..a9c917e9c 100644 --- a/test/unit/lib/redmine/helpers/url_test.rb +++ b/test/unit/lib/redmine/helpers/url_test.rb @@ -33,4 +33,45 @@ class URLTest < ActiveSupport::TestCase assert_not uri_with_safe_scheme?("httpx://example.com/") assert_not uri_with_safe_scheme?("mailto:root@") end + + LINK_SAFE_URIS = [ + "http://example.com/", + "https://example.com/", + "ftp://example.com/", + "foo://example.org", + "mailto:foo@example.org", + " http://example.com/", + "", + "/javascript:alert(\'filename\')", + ] + + def test_uri_with_link_safe_scheme_should_recognize_safe_uris + LINK_SAFE_URIS.each do |uri| + assert uri_with_link_safe_scheme?(uri), "'#{uri}' should be safe" + end + end + + LINK_UNSAFE_URIS = [ + "javascript:alert(\'XSS\');", + "javascript :alert(\'XSS\');", + "javascript: alert(\'XSS\');", + "javascript : alert(\'XSS\');", + ":javascript:alert(\'XSS\');", + "javascript:", + "javascript:", + "javascript:", + "javascript:", + "java\0script:alert(\"XSS\")", + "java\script:alert(\"XSS\")", + " \x0e javascript:alert(\'XSS\');", + "data:image/png;base64,foobar", + "vbscript:foobar", + "data:text/html;base64,foobar", + ] + + def test_uri_with_link_safe_scheme_should_recognize_unsafe_uris + LINK_UNSAFE_URIS.each do |uri| + assert_not uri_with_link_safe_scheme?(uri), "'#{uri}' should not be safe" + end + end end diff --git a/test/unit/lib/redmine/wiki_formatting/common_mark/sanitization_filter_test.rb b/test/unit/lib/redmine/wiki_formatting/common_mark/sanitization_filter_test.rb index 72ef52a63..a1de2b974 100644 --- a/test/unit/lib/redmine/wiki_formatting/common_mark/sanitization_filter_test.rb +++ b/test/unit/lib/redmine/wiki_formatting/common_mark/sanitization_filter_test.rb @@ -71,6 +71,25 @@ if Object.const_defined?(:CommonMarker) assert_equal %(foo), filter(input) end + def test_should_allow_links_with_safe_url_schemes + %w(http https ftp ssh foo).each do |scheme| + input = %(foo) + assert_equal input, filter(input) + end + end + + def test_should_allow_mailto_links + input = %(bar) + assert_equal input, filter(input) + end + + def test_should_remove_empty_link + input = %(bar) + assert_equal %(bar), filter(input) + input = %(bar) + assert_equal %(bar), filter(input) + end + # samples taken from the Sanitize test suite # rubocop:disable Layout/LineLength STRINGS = [ @@ -194,11 +213,6 @@ if Object.const_defined?(:CommonMarker) 'XSS', 'XSS' ], - - 'invalid URIs' => [ - 'link', - 'link' - ], } PROTOCOLS.each do |name, strings| -- 2.39.5