From e46ef9dcd91e25bc41a90b332e7df153d9f62858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 25 Nov 2015 15:23:22 -0200 Subject: [PATCH] Define a less permissive list of tags and attributes And use it by default. The new sanitizer were being a lot more permissive that we had in Rails until the version 4.2. This was also allowing arbritary data attributes by default what can lead to CSRF and XSS attacks. Now data attributes need to be explicitly allowed. CVE-2015-7578 --- lib/rails/html/sanitizer.rb | 4 ++++ lib/rails/html/scrubbers.rb | 25 +++++++++++++++++++++++++ test/sanitizer_test.rb | 36 ++++++++++++++++++++++++++---------- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb index 8be10d5..f40bf6b 100644 --- a/lib/rails/html/sanitizer.rb +++ b/lib/rails/html/sanitizer.rb @@ -97,6 +97,10 @@ module Rails attr_accessor :allowed_tags attr_accessor :allowed_attributes end + self.allowed_tags = Set.new(%w(strong em b i p code pre tt samp kbd var sub + sup dfn cite big small address hr br div span h1 h2 h3 h4 h5 h6 ul ol li dl dt dd abbr + acronym a img blockquote del ins)) + self.allowed_attributes = Set.new(%w(href src width height alt cite datetime title class name xml:lang abbr)) def initialize @permit_scrubber = PermitScrubber.new diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb index 1384a2f..2c89c04 100644 --- a/lib/rails/html/scrubbers.rb +++ b/lib/rails/html/scrubbers.rb @@ -100,6 +100,7 @@ module Rails if @attributes node.attribute_nodes.each do |attr| attr.remove if scrub_attribute?(attr.name) + scrub_attribute(node, attr) end scrub_css_attribute(node) @@ -123,6 +124,30 @@ module Rails end var end + + def scrub_attribute(node, attr_node) + attr_name = if attr_node.namespace + "#{attr_node.namespace.prefix}:#{attr_node.node_name}" + else + attr_node.node_name + end + + if Loofah::HTML5::WhiteList::ATTR_VAL_IS_URI.include?(attr_name) + # this block lifted nearly verbatim from HTML5 sanitization + val_unescaped = CGI.unescapeHTML(attr_node.value).gsub(Loofah::HTML5::Scrub::CONTROL_CHARACTERS,'').downcase + if val_unescaped =~ /^[a-z0-9][-+.a-z0-9]*:/ && ! Loofah::HTML5::WhiteList::ALLOWED_PROTOCOLS.include?(val_unescaped.split(Loofah::HTML5::WhiteList::PROTOCOL_SEPARATOR)[0]) + attr_node.remove + end + end + if Loofah::HTML5::WhiteList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name) + attr_node.value = attr_node.value.gsub(/url\s*\(\s*[^#\s][^)]+?\)/m, ' ') if attr_node.value + end + if Loofah::HTML5::WhiteList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == 'xlink:href' && attr_node.value =~ /^\s*[^#\s].*/m + attr_node.remove + end + + node.remove_attribute(attr_node.name) if attr_name == 'src' && attr_node.value !~ /[^[:space:]]/ + end end # === Rails::Html::TargetScrubber diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb index 06d70e4..5f636ba 100644 --- a/test/sanitizer_test.rb +++ b/test/sanitizer_test.rb @@ -152,7 +152,7 @@ class SanitizersTest < Minitest::Test end def test_sanitize_script - assert_sanitized "a b cd e f", "a b cd e f" + assert_sanitized "a b cd e f", "a b cblah blah blahd e f" end def test_sanitize_js_handlers @@ -173,17 +173,23 @@ class SanitizersTest < Minitest::Test tags = Loofah::HTML5::WhiteList::ALLOWED_ELEMENTS - %w(script form) tags.each do |tag_name| define_method "test_should_allow_#{tag_name}_tag" do - assert_sanitized "start <#{tag_name} title=\"1\" onclick=\"foo\">foo bar baz end", %(start <#{tag_name} title="1">foo bar baz end) + scope_allowed_tags(tags) do + assert_sanitized "start <#{tag_name} title=\"1\" onclick=\"foo\">foo bar baz end", %(start <#{tag_name} title="1">foo bar baz end) + end end end def test_should_allow_anchors - assert_sanitized %(), %() + assert_sanitized %(), %(baz) end def test_video_poster_sanitization - assert_sanitized %(), %() - assert_sanitized %(), %() + scope_allowed_tags(%w(video)) do + scope_allowed_attributes %w(src poster) do + assert_sanitized %(), %() + assert_sanitized %(), %() + end + end end # RFC 3986, sec 4.2 @@ -309,7 +315,7 @@ class SanitizersTest < Minitest::Test end def test_should_not_fall_for_xss_image_hack_with_uppercase_tags - assert_sanitized %(">), "\">" + assert_sanitized %(">), %(alert("XSS")">) end [%(), @@ -453,6 +459,16 @@ class SanitizersTest < Minitest::Test end end + def test_sanitize_data_attributes + assert_sanitized %(foo), %(foo) + assert_sanitized %(Launch the missiles), %(Launch the missiles) + end + + def test_allow_data_attribute_if_requested + text = %(foo) + assert_equal %(foo), white_list_sanitize(text, attributes: ['data-foo']) + end + protected def xpath_sanitize(input, options = {}) @@ -484,18 +500,18 @@ protected end def scope_allowed_tags(tags) + old_tags = Rails::Html::WhiteListSanitizer.allowed_tags Rails::Html::WhiteListSanitizer.allowed_tags = tags yield Rails::Html::WhiteListSanitizer.new - ensure - Rails::Html::WhiteListSanitizer.allowed_tags = nil + Rails::Html::WhiteListSanitizer.allowed_tags = old_tags end def scope_allowed_attributes(attributes) + old_attributes = Rails::Html::WhiteListSanitizer.allowed_attributes Rails::Html::WhiteListSanitizer.allowed_attributes = attributes yield Rails::Html::WhiteListSanitizer.new - ensure - Rails::Html::WhiteListSanitizer.allowed_attributes = nil + Rails::Html::WhiteListSanitizer.allowed_attributes = old_attributes end end -- 2.4.0