Last year, I reported on HackerOne a vulnerability affecting the Rails 4.2 and 5.0 sanitizer. The vulnerability has the CVE-2015-7580. The vulnerability is now disclosed and I can write about it.

Rails::Html::WhiteListSanitizer is used by helper methods such as sanitize, sanitize_css, strip_tags and strip_links.

Rails::Html::WhiteListSanitizer is supposed to remove nasty HTML tags from the content you pass to it and return a sanitized content:

white_list_sanitizer = Rails::Html::WhiteListSanitizer.new
white_list_sanitizer.sanitize('Hello <script>alert("evil")</script> World!')
 => "Hello alert(\"evil\") World!"

This is different than the default behaviour in views which is to encode HTML content to make it safe:

include ERB::Util
html_escape 'Hello <script>alert("evil")</script> World!'
 => "Hello &lt;script&gt;alert(&quot;evil&quot;)&lt;/script&gt; World!"

However, in the vulnerable version, the sanitizer was having issues with partial/malformed HTML tags. While writing tests for a method using the sanitizer, I discovered that not all the tags are removed when sanitizing invalid HTML fragments:

<%= sanitize '<script><script></script>', tags: %w(em) %>

would render <script>.

<%= sanitize '<script><script></script>alert("XSS");<script><</script>/</script><script>script></script>', tags: %w(em) %>

would render <script>alert("XSS");</script>. Clearly, not good.

The patch is quite small:

diff --git a/lib/rails/html/scrubbers.rb b/lib/rails/html/scrubbers.rb
index 1384a2f..6d82a20 100644
--- a/lib/rails/html/scrubbers.rb
+++ b/lib/rails/html/scrubbers.rb
@@ -60,6 +60,11 @@ module Rails
       end

       def scrub(node)
+        if node.cdata?
+          text = node.document.create_text_node node.text
+          node.replace text
+          return CONTINUE
+        end
         return CONTINUE if skip_node?(node)

         unless keep_node?(node)
@@ -76,7 +81,7 @@ module Rails
       end

       def skip_node?(node)
-        node.text? || node.cdata?
+        node.text?
       end

       def scrub_attribute?(name)
diff --git a/test/sanitizer_test.rb b/test/sanitizer_test.rb
index 06d70e4..3bfc7cb 100644
--- a/test/sanitizer_test.rb
+++ b/test/sanitizer_test.rb
@@ -11,6 +11,16 @@ class SanitizersTest < Minitest::Test
     end
   end

+  def test_sanitize_nested_script
+    sanitizer = Rails::Html::WhiteListSanitizer.new
+    assert_equal '&lt;script&gt;alert("XSS");&lt;/script&gt;', sanitizer.sanitize('<script><script></script>alert("XSS");<script><</script>/</script><script>script></script>', tags: %w(em))
+  end
+
+  def test_sanitize_nested_script_in_style
+    sanitizer = Rails::Html::WhiteListSanitizer.new
+    assert_equal '&lt;script&gt;alert("XSS");&lt;/script&gt;', sanitizer.sanitize('<style><script></style>alert("XSS");<style><</style>/</style><style>script></style>', tags: %w(em))
+  end
+
   class XpathRemovalTestSanitizer < Rails::Html::Sanitizer
     def sanitize(html, options = {})
       fragment = Loofah.fragment(html)

If you’re using these helpers, please upgrade or patch as soon as possible.