CVE-2015-7579: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Cross-site scripting (XSS) vulnerability in the rails-html-sanitizer gem 1.0.2 for Ruby on Rails 4.2.x and 5.x allows remote attackers to inject arbitrary web script or HTML via an HTML entity that is mishandled by the Rails::Html::FullSanitizer class.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 6.1
Published: 2016-02-16T02:59Z
Repository: https://github.com/rails/rails-html-sanitizer
Language: Ruby
Files changed: lib/rails/html/sanitizer.rb, lib/rails/html/scrubbers.rb, test/sanitizer_test.rb, test/scrubbers_test.rb
Commit date: 2016-01-22 15:07:57 -0800
Commit message: Do not unescape already escaped HTML entities

The full sanitizer was using Loofah's #text method that automatically
escapes HTML entities. That behavior caused some problems where strings
that were not escaped in the older sanitizer started to be escaped. To
fix these problems we used the #text's `encode_special_chars` option as
`false` that not just skipped the HTML entities escaping but unescaped
already escaped entities.

This introduced a security bug because an attacker can pass escaped HTML
tags that will not be sanitized and will be returned as unescaped HTML
tags.

To fix it properly we introduced a new scrubber that will remove all
tags and keep just the text nodes of these tags without changing how
to escape the string.

CVE-2015-7579

Diff: diff --git a/lib/rails/html/sanitizer.rb b/lib/rails/html/sanitizer.rb
index f40bf6b..68ae6d2 100644
--- a/lib/rails/html/sanitizer.rb
+++ b/lib/rails/html/sanitizer.rb
@@ -11,10 +11,14 @@ module Rails
 
       def remove_xpaths(node, xpaths)
         node.xpath(*xpaths).remove
         node
       end
+
+      def properly_encode(fragment, options)
+        fragment.xml? ? fragment.to_xml(options) : fragment.to_html(options)
+      end
     end
 
     # === Rails::Html::FullSanitizer
     # Removes all tags but strips out scripts, forms and comments.
     #
@@ -24,13 +28,16 @@ module Rails
     class FullSanitizer < Sanitizer
       def sanitize(html, options = {})
         return unless html
         return html if html.empty?
 
-        Loofah.fragment(html).tap do |fragment|
-          remove_xpaths(fragment, XPATHS_TO_REMOVE)
-        end.text(options)
+        loofah_fragment = Loofah.fragment(html)
+
+        remove_xpaths(loofah_fragment, XPATHS_TO_REMOVE)
+        loofah_fragment.scrub!(TextOnlyScrubber.new)
+
+        properly_encode(loofah_fragment, encoding: 'UTF-8')
       end
     end
 
     # === Rails::Html::LinkSanitizer
     # Removes a tags and href attributes leaving only the link text
@@ -138,12 +145,8 @@ module Rails
       end
 
       def allowed_attributes(options)
         options[:attributes] || self.class.allowed_attributes
       end
-
-      def properly_encode(fragment, options)
-        fragment.xml? ? fragment.to_xml(options) : fragment.to_html(options)
-      end
     end
   end
 end
diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb
index 2c89c04..d6f8ce4 100644
--- a/lib/rails/html/scrubbers.rb
+++ b/lib/rails/html/scrubbers.rb
@@ -167,7 +167,27 @@ module Rails
 
       def scrub_attribute?(name)
         !super
       end
     end
+
+    # === Rails::Html::TextOnlyScrubber
+    #
+    # Rails::Html::TextOnlyScrubber allows you to permit text nodes.
+    #
+    # Unallowed elements will be stripped, i.e. element is removed but its subtree kept.
+    class TextOnlyScrubber < Loofah::Scrubber
+      def initialize
+        @direction = :bottom_up
+      end
+
+      def scrub(node)
+        if node.text?
+          CONTINUE
+        else
+          node.before node.children
+          node.remove
+        end
+      end
+    end
   end
 end
diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb
index 58c0054..8c1ea2c 100644
--- a/test/sanitizer_test.rb
+++ b/test/sanitizer_test.rb
@@ -102,13 +102,16 @@ class SanitizersTest < Minitest::Test
 
   def test_strip_tags_with_frozen_string
     assert_equal "Frozen string with no tags", full_sanitize("Frozen string with no tags".freeze)
   end
 
-  def test_full_sanitize_allows_turning_off_encoding_special_chars
+  def test_full_sanitize_respect_html_escaping_of_the_given_string
+    assert_equal 'test\r\nstring', full_sanitize('test\r\nstring')
     assert_equal '&amp;', full_sanitize('&')
-    assert_equal '&', full_sanitize('&', encode_special_chars: false)
+    assert_equal '&amp;', full_sanitize('&amp;')
+    assert_equal '&amp;amp;', full_sanitize('&amp;amp;')
+    assert_equal 'omg &lt;script&gt;BOM&lt;/script&gt;', full_sanitize('omg &lt;script&gt;BOM&lt;/script&gt;')
   end
 
   def test_strip_links_with_tags_in_tags
     expected = "a href='hello'&gt;all <b>day</b> long/a&gt;"
     input = "<<a>a href='hello'>all <b>day</b> long<</A>/a>"
diff --git a/test/scrubbers_test.rb b/test/scrubbers_test.rb
index 2a2838a..4b84263 100644
--- a/test/scrubbers_test.rb
+++ b/test/scrubbers_test.rb
@@ -141,10 +141,24 @@ class TargetScrubberTest < ScrubberTest
     html = '<tag remove="" other=""></tag><a remove="" other=""></a>'
     assert_scrubbed html, '<a other=""></a>'
   end
 end
 
+class TextOnlyScrubberTest < ScrubberTest
+  def setup
+    @scrubber = Rails::Html::TextOnlyScrubber.new
+  end
+
+  def test_removes_all_tags_and_keep_the_content
+    assert_scrubbed '<tag>hello</tag>', 'hello'
+  end
+
+  def test_skips_text_nodes
+    assert_node_skipped('some text')
+  end
+end
+
 class ReturningStopFromScrubNodeTest < ScrubberTest
   class ScrubStopper < Rails::Html::PermitScrubber
     def scrub_node(node)
       Loofah::Scrubber::STOP
     end
@@ -155,6 +169,6 @@ class ReturningStopFromScrubNodeTest < ScrubberTest
   end
 
   def test_returns_stop_from_scrub_if_scrub_node_does
     assert_scrub_stopped '<script>remove me</script>'
   end
-end
\ No newline at end of file
+end

Vulnerable code:         Loofah.fragment(html).tap do |fragment|
          remove_xpaths(fragment, XPATHS_TO_REMOVE)
        end.text(options)

      def properly_encode(fragment, options)
        fragment.xml? ? fragment.to_xml(options) : fragment.to_html(options)
      end
  def test_full_sanitize_allows_turning_off_encoding_special_chars
    assert_equal '&', full_sanitize('&', encode_special_chars: false)
end
Fixed code: 
      def properly_encode(fragment, options)
        fragment.xml? ? fragment.to_xml(options) : fragment.to_html(options)
      end
        loofah_fragment = Loofah.fragment(html)

        remove_xpaths(loofah_fragment, XPATHS_TO_REMOVE)
        loofah_fragment.scrub!(TextOnlyScrubber.new)

        properly_encode(loofah_fragment, encoding: 'UTF-8')

    # === Rails::Html::TextOnlyScrubber
    #
    # Rails::Html::TextOnlyScrubber allows you to permit text nodes.
    #
    # Unallowed elements will be stripped, i.e. element is removed but its subtree kept.
    class TextOnlyScrubber < Loofah::Scrubber
      def initialize
        @direction = :bottom_up
      end

      def scrub(node)
        if node.text?
          CONTINUE
        else
          node.before node.children
          node.remove
        end
      end
    end
  def test_full_sanitize_respect_html_escaping_of_the_given_string
    assert_equal 'test\r\nstring', full_sanitize('test\r\nstring')
    assert_equal '&amp;', full_sanitize('&amp;')
    assert_equal '&amp;amp;', full_sanitize('&amp;amp;')
    assert_equal 'omg &lt;script&gt;BOM&lt;/script&gt;', full_sanitize('omg &lt;script&gt;BOM&lt;/script&gt;')
class TextOnlyScrubberTest < ScrubberTest
  def setup
    @scrubber = Rails::Html::TextOnlyScrubber.new
  end

  def test_removes_all_tags_and_keep_the_content
    assert_scrubbed '<tag>hello</tag>', 'hello'
  end

  def test_skips_text_nodes
    assert_node_skipped('some text')
  end
end

end