Allow picture, source, srcset, sizes with a dedicated srcset scrubber

lib/loofah/html5/safelist.rb

@@ -114,13 +114,15 @@ module SafeList
         "option",
         "output",
         "p",
+        "picture",
         "pre",
         "q",
         "s",
         "samp",
         "section",
         "select",
         "small",
+        "source",
         "span",
         "strike",
         "strong",
@@ -295,8 +297,10 @@ module SafeList
         "selected",
         "shape",
         "size",
+        "sizes",
         "span",
         "src",
+        "srcset",
         "start",
         "style",
         "summary",
@@ -591,6 +595,10 @@ module SafeList
         "xml:base",
       ])
 
+      ATTR_VAL_IS_SRCSET = Set.new([
+        "srcset",
+      ])
+
       SVG_ATTR_VAL_ALLOWS_REF = Set.new([
         "clip-path",
         "color-profile",

lib/loofah/html5/scrub.rb

@@ -3,6 +3,7 @@
 require "cgi/escape"
 require "cgi/util" if RUBY_VERSION < "3.5"
 require "crass"
+require "strscan"
 
 module Loofah
   module HTML5 # :nodoc:
@@ -43,6 +44,10 @@ def scrub_attributes(node)
               next if scrub_uri_attribute(attr_node)
             end
 
+            if SafeList::ATTR_VAL_IS_SRCSET.include?(attr_name)
+              next if scrub_srcset_attribute(attr_node)
+            end
+
             if SafeList::SVG_ATTR_VAL_ALLOWS_REF.include?(attr_name)
               scrub_attribute_that_allows_local_ref(attr_node)
             end
@@ -173,6 +178,49 @@ def scrub_uri_attribute(attr_node)
           end
         end
 
+        #
+        #  "srcset" is a list of "URL descriptor" candidates, not a single URL,
+        #  so each candidate URL is validated independently via allowed_uri?
+        #  (fail-closed). See the srcset tests for CVE-2024-8372.
+        #
+        def scrub_srcset_attribute(attr_node)
+          disallowed = srcset_candidate_urls(attr_node.value).any? do |url|
+            !allowed_uri?(url)
+          end
+
+          if disallowed
+            attr_node.remove
+            true
+          else
+            false
+          end
+        end
+
+        #  Splits a "srcset" value into its candidate URLs. Per the WHATWG srcset
+        #  parsing rules a URL token is delimited by ASCII whitespace, not by
+        #  commas, so a comma inside a data: URL stays part of that URL.
+        def srcset_candidate_urls(value)
+          urls = []
+          scanner = StringScanner.new(value.to_s)
+
+          until scanner.eos?
+            scanner.skip(/[\t\n\f\r ,]+/)
+            break if scanner.eos?
+
+            url = scanner.scan(/[^\t\n\f\r ]+/)
+
+            if url.end_with?(",")
+              url = url.sub(/,+\z/, "")
+            else
+              scanner.skip(/[^,]*/)
+            end
+
+            urls << url unless url.empty?
+          end
+
+          urls
+        end
+
         #
         #  libxml2 >= 2.9.2 fails to escape comments within some attributes.
         #

test/html5/test_sanitizer.rb

@@ -84,6 +84,8 @@ def assert_completes_in_reasonable_time(&block)
         outputs << "<table><tbody><tr><#{tag_name} title='1'>foo</#{tag_name}></tr></tbody></table>" # libgumbo
       when "colgroup", "tbody", "tfoot", "thead"
         outputs << "foo<table><#{tag_name} title='1'></#{tag_name}></table>" # libgumbo
+      when "source"
+        outputs << "<source title='1'>foo" # libgumbo treats <source> as void
       when "br"
         outputs << "<br title='1'>foo<br>"
       end
@@ -200,6 +202,48 @@ def test_should_allow_contenteditable
     check_sanitization(input, output)
   end
 
+  def test_should_allow_picture_source_responsive_image
+    input = %(<picture><source srcset="a.avif"><img src="a.jpg" alt="x"></picture>)
+    check_sanitization(
+      input,
+      %(<picture><source srcset="a.avif"><img src="a.jpg" alt="x"></source></picture>), # libxml
+      %(<picture><source srcset="a.avif"><img src="a.jpg" alt="x"></picture>), # libgumbo
+      %(<picture><source srcset="a.avif"><img alt="x" src="a.jpg"></picture>), # nekohtml (jruby)
+    )
+  end
+
+  def test_should_allow_srcset_with_multiple_safe_urls
+    input = %(<img src="ok.jpg" srcset="https://example.com/a.jpg 1x, https://example.com/b.jpg 2x">)
+    output = %(<img src="ok.jpg" srcset="https://example.com/a.jpg 1x, https://example.com/b.jpg 2x">)
+    check_sanitization(input, output)
+  end
+
+  def test_should_allow_srcset_with_safe_data_uri
+    input = %(<img srcset="data:image/png;base64,iVBORw0K 1x">)
+    output = %(<img srcset="data:image/png;base64,iVBORw0K 1x">)
+    check_sanitization(input, output)
+
+    # the comma inside the data: URL must stay part of that URL even when a
+    # second candidate follows (candidate commas only separate after the URL)
+    input = %(<img srcset="data:image/png;base64,iVBORw0K 1x, https://example.com/b.jpg 2x">)
+    output = %(<img srcset="data:image/png;base64,iVBORw0K 1x, https://example.com/b.jpg 2x">)
+    check_sanitization(input, output)
+  end
+
+  def test_should_disallow_srcset_with_unsafe_uris
+    input = %(<img src="ok.jpg" srcset="javascript:alert(1) 1x">)
+    output = %(<img src="ok.jpg">)
+    check_sanitization(input, output)
+
+    input = %(<img src="ok.jpg" srcset="https://example.com/a.jpg 1x, javascript:alert(1) 2x">)
+    output = %(<img src="ok.jpg">)
+    check_sanitization(input, output)
+
+    input = %(<img srcset="data:text/html,xx 1x">)
+    output = %(<img>)
+    check_sanitization(input, output)
+  end
+
   ##
   ##  libxml2 downcases attributes, so this is moot.
   ##

test/integration/test_ad_hoc.rb

@@ -315,6 +315,36 @@ def test_dont_remove_whitespace_between_tags
           end
         end
 
+        context "xss protection from srcset attribute" do
+          it "sanitizes a disallowed scheme in any srcset candidate" do
+            # for CVE-2024-8372
+            # see: https://nvd.nist.gov/vuln/detail/CVE-2024-8372
+            html = %{<img src="https://example.com/ok.jpg" srcset="https://example.com/a.jpg 1x, javascript:alert(1) 2x">}
+
+            sanitized = scrub_fragment(html, :escape)
+
+            assert_nil sanitized.at_css("img")["srcset"]
+            assert sanitized.at_css("img")["src"]
+          end
+
+          it "sanitizes a disallowed data: mediatype in srcset" do
+            # for CVE-2024-8372 (data:image/svg+xml vector)
+            html = %{<img src="https://example.com/ok.jpg" srcset="data:image/svg+xml;base64,PHN2Zz4= 1x">}
+
+            sanitized = scrub_fragment(html, :escape)
+
+            assert_nil sanitized.at_css("img")["srcset"]
+          end
+
+          it "sanitizes a disallowed scheme in a <source> srcset" do
+            html = %{<picture><source srcset="https://example.com/a.avif 1x, vbscript:msgbox 2x"><img src="https://example.com/ok.jpg"></picture>}
+
+            sanitized = scrub_fragment(html, :escape)
+
+            assert_nil sanitized.at_css("source")["srcset"]
+          end
+        end
+
         #
         #  brought up by https://github.com/flavorjones/loofah/issues/80
         #