Merge pull request #172 from flavorjones/171-xss-vulnerability

171 xss vulnerability

Author: flavorjones

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+## 2.3.1 / 2019-10-22
+
+### Security
+
+Address CVE-2019-15587: Unsanitized JavaScript may occur in sanitized output when a crafted SVG element is republished.
+
+This CVE's public notice is at https://github.com/flavorjones/loofah/issues/171
+
+
 ## 2.3.0 / 2019-09-28
 
 ### Features

lib/loofah/html5/safelist.rb

@@ -1,4 +1,4 @@
-require 'set'
+require "set"
 
 module Loofah
   module HTML5 # :nodoc:
@@ -45,7 +45,6 @@ module HTML5 # :nodoc:
     #
     # </html5_license>
     module SafeList
-
       ACCEPTABLE_ELEMENTS = Set.new([
                                       "a",
                                       "abbr",
@@ -361,7 +360,6 @@ module SafeList
                                  "baseProfile",
                                  "bbox",
                                  "begin",
-                                 "by",
                                  "calcMode",
                                  "cap-height",
                                  "class",
@@ -468,7 +466,6 @@ module SafeList
                                  "systemLanguage",
                                  "target",
                                  "text-anchor",
-                                 "to",
                                  "transform",
                                  "type",
                                  "u1",
@@ -478,7 +475,6 @@ module SafeList
                                  "unicode",
                                  "unicode-range",
                                  "units-per-em",
-                                 "values",
                                  "version",
                                  "viewBox",
                                  "visibility",

test/integration/test_ad_hoc.rb

@@ -1,7 +1,6 @@
 require "helper"
 
 class IntegrationTestAdHoc < Loofah::TestCase
-
   context "blank input string" do
     context "fragment" do
       it "return a blank string" do
@@ -33,9 +32,9 @@ def test_removal_of_illegal_attribute
       html = "<p class=bar foo=bar abbr=bar />"
       sane = Nokogiri::HTML(Loofah.scrub_fragment(html, :escape).to_xml)
       node = sane.xpath("//p").first
-      assert node.attributes['class']
-      assert node.attributes['abbr']
-      assert_nil node.attributes['foo']
+      assert node.attributes["class"]
+      assert node.attributes["abbr"]
+      assert_nil node.attributes["foo"]
     end
 
     def test_removal_of_illegal_url_in_href
@@ -45,14 +44,14 @@ def test_removal_of_illegal_url_in_href
     HTML
       sane = Nokogiri::HTML(Loofah.scrub_fragment(html, :escape).to_xml)
       nodes = sane.xpath("//a")
-      assert_nil nodes.first.attributes['href']
-      assert nodes.last.attributes['href']
+      assert_nil nodes.first.attributes["href"]
+      assert nodes.last.attributes["href"]
     end
 
     def test_css_sanitization
       html = "<p style='background-color: url(\"http://foo.com/\") ; background-color: #000 ;' />"
       sane = Nokogiri::HTML(Loofah.scrub_fragment(html, :escape).to_xml)
-      assert_match %r/#000/,    sane.inner_html
+      assert_match %r/#000/, sane.inner_html
       refute_match %r/foo\.com/, sane.inner_html
     end
 
@@ -75,7 +74,7 @@ def test_fragment_with_text_nodes_leading_and_trailing
     def test_whitewash_on_fragment
       html = "safe<frameset rows=\"*\"><frame src=\"http://example.com\"></frameset> <b>description</b>"
       whitewashed = Loofah.scrub_document(html, :whitewash).xpath("/html/body/*").to_s
-      assert_equal "<p>safe</p><b>description</b>", whitewashed.gsub("\n","")
+      assert_equal "<p>safe</p><b>description</b>", whitewashed.gsub("\n", "")
     end
 
     def test_fragment_whitewash_on_microsofty_markup
@@ -86,11 +85,11 @@ def test_fragment_whitewash_on_microsofty_markup
     def test_document_whitewash_on_microsofty_markup
       whitewashed = Loofah.document(MSWORD_HTML).scrub!(:whitewash)
       assert_match %r(<p>Foo <b>BOLD</b></p>), whitewashed.to_s
-      assert_equal "<p>Foo <b>BOLD</b></p>",   whitewashed.xpath("/html/body/*").to_s
+      assert_equal "<p>Foo <b>BOLD</b></p>", whitewashed.xpath("/html/body/*").to_s
     end
 
     def test_return_empty_string_when_nothing_left
-      assert_equal "", Loofah.scrub_document('<script>test</script>', :prune).text
+      assert_equal "", Loofah.scrub_document("<script>test</script>", :prune).text
     end
 
     def test_nested_script_cdata_tags_should_be_scrubbed
@@ -145,21 +144,20 @@ def test_dont_remove_whitespace_between_tags
       #
       #    https://git.gnome.org/browse/libxml2/tree/HTMLtree.c?h=v2.9.2#n714
       #
-      {tag: "a",   attr: "href"},
-      {tag: "div", attr: "href"},
-      {tag: "a",   attr: "action"},
-      {tag: "div", attr: "action"},
-      {tag: "a",   attr: "src"},
-      {tag: "div", attr: "src"},
-      {tag: "a",   attr: "name"},
+      { tag: "a", attr: "href" },
+      { tag: "div", attr: "href" },
+      { tag: "a", attr: "action" },
+      { tag: "div", attr: "action" },
+      { tag: "a", attr: "src" },
+      { tag: "div", attr: "src" },
+      { tag: "a", attr: "name" },
       #
       #  note that div+name is _not_ affected by the libxml2 issue.
       #  but we test it anyway to ensure our logic isn't modifying
       #  attributes that don't need modifying.
       #
-      {tag: "div", attr: "name", unescaped: true},
+      { tag: "div", attr: "name", unescaped: true },
     ].each do |config|
-
       define_method "test_uri_escaping_of_#{config[:attr]}_attr_in_#{config[:tag]}_tag" do
         html = %{<#{config[:tag]} #{config[:attr]}='examp<!--" unsafeattr=foo()>-->le.com'>test</#{config[:tag]}>}
 
@@ -190,14 +188,32 @@ def test_dont_remove_whitespace_between_tags
       end
     end
 
-    # see:
-    # - https://github.com/flavorjones/loofah/issues/154
-    # - https://hackerone.com/reports/429267
-    context "xss protection from svg xmlns:xlink animate attribute" do
-      it "sanitizes appropriate attributes" do
-        html = %Q{<svg><a xmlns:xlink=http://www.w3.org/1999/xlink xlink:href=?><circle r=400 /><animate attributeName=xlink:href begin=0 from=javascript:alert(1) to=%26>}
+    context "xss protection from svg animate attributes" do
+      # see recommendation from https://html5sec.org/#137
+      # to sanitize "to", "from", "values", and "by" attributes
+
+      it "sanitizes 'from', 'to', and 'by' attributes" do
+        # for CVE-2018-16468
+        # see:
+        # - https://github.com/flavorjones/loofah/issues/154
+        # - https://hackerone.com/reports/429267
+        html = %Q{<svg><a xmlns:xlink=http://www.w3.org/1999/xlink xlink:href=?><circle r=400 /><animate attributeName=xlink:href begin=0 from=javascript:alert(1) to=%26 by=5>}
+
         sanitized = Loofah.scrub_fragment(html, :escape)
         assert_nil sanitized.at_css("animate")["from"]
+        assert_nil sanitized.at_css("animate")["to"]
+        assert_nil sanitized.at_css("animate")["by"]
+      end
+
+      it "sanitizes 'values' attribute" do
+        # for CVE-2019-15587
+        # see:
+        # - https://github.com/flavorjones/loofah/issues/171
+        # - https://hackerone.com/reports/709009
+        html = %Q{<svg> <animate href="#foo" attributeName="href" values="javascript:alert('xss')"/> <a id="foo"> <circle r=400 /> </a> </svg>}
+
+        sanitized = Loofah.scrub_fragment(html, :escape)
+        assert_nil sanitized.at_css("animate")["values"]
       end
     end
   end