Properly parse srcset attributes

mikesamuel · mikesamuel · commit 2ca428433034 · 2019-03-25T10:53:20.000-04:00
Fixes issue #112 Previously, extra URL handling for `src`, `href` and related attributes was naively applied to `srcset`. This failed safe but mangled content. I consolidated (in a previous commit) the code that ensures that URL and `style` attributes have extra checks done: the global URL and style policies respectively. This commit adds special handling for srcset which uses the global URL policy to vet each URL policy and leaves the metadata portion alone. Commas in the middle of URLs in a `srcset` value will be escaped to `%2c`.
diff --git a/change_log.md b/change_log.md
@@ -2,6 +2,8 @@
 
 Most recent at top.
   * Pending
+    * Properly parse `srcset` attribute values to apply URL policy to
+      each URL in turn.
     * Update dependency on guava version to 27.1-jre to avoid causing clients
       problems with CVE-2018-10237.  Specify Maven property `guava.version`
       to override.
diff --git a/src/main/java/org/owasp/html/HtmlPolicyBuilder.java b/src/main/java/org/owasp/html/HtmlPolicyBuilder.java
@@ -610,7 +610,7 @@ AttributePolicy makeGuard(AttributeGuardIntermediates intermediates) {
     for (String urlAttributeName : new String[] {
         "action", "archive", "background", "cite", "classid", "codebase", "data",
         "dsync", "formaction", "href", "icon", "longdesc", "manifest", "poster",
-        "profile", "src", "srcset", "usemap",
+        "profile", "src", "usemap",
     }) {
       b.put(urlAttributeName, identityGuard);
     }
@@ -634,6 +634,14 @@ public String apply(String url) {
             });
       }
 
+    });
+    b.put("srcset", new AttributeGuardMaker() {
+
+      @Override
+      AttributePolicy makeGuard(AttributeGuardIntermediates intermediates) {
+        return new SrcsetAttributePolicy(intermediates.urlAttributePolicy);
+      }
+
     });
     ATTRIBUTE_GUARDS = b.build();
   }
diff --git a/src/main/java/org/owasp/html/SrcsetAttributePolicy.java b/src/main/java/org/owasp/html/SrcsetAttributePolicy.java
@@ -0,0 +1,134 @@
+// Copyright (c) 2019, Mike Samuel
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions
+// are met:
+//
+// Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// Redistributions in binary form must reproduce the above copyright
+// notice, this list of conditions and the following disclaimer in the
+// documentation and/or other materials provided with the distribution.
+// Neither the name of the OWASP nor the names of its contributors may
+// be used to endorse or promote products derived from this software
+// without specific prior written permission.
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+// FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+// COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+// INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+// BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+// LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+// LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+// ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+// POSSIBILITY OF SUCH DAMAGE.
+
+package org.owasp.html;
+
+/**
+ * Applies a URL policy to all URLs in a srcset attribute value.
+ * <p>
+ * https://html.spec.whatwg.org/multipage/images.html#srcset-attributes
+ * explains srcset for images and other media content.
+ * <p>
+ * There is a pending draft to use it on <script> to allow loading
+ * scripts compatible with different versions of JS.
+ * <p>
+ * The general form of srcset is
+ * <pre>
+ * srcset ::== space* srcplus ([,] space* srcplus)*
+ * # Additionally, the URL may not start or end with a comma
+ * srcplus ::== URL (space+ metadata)?
+ * metadata ::== FLOAT [a-z]?
+ * </pre>
+ * <p>
+ * This policy applies the given attribute policy to URLs and emits metadata
+ * as given, but normalizing spaces.
+ */
+final class SrcsetAttributePolicy implements AttributePolicy {
+
+  private final AttributePolicy srcPolicy;
+
+  SrcsetAttributePolicy(AttributePolicy srcPolicy) {
+    this.srcPolicy = srcPolicy;
+  }
+
+  public String apply(String elementName, String attributeName, String value) {
+    StringBuilder sb = new StringBuilder();
+
+    int i = 0, n = value.length();
+    // Skip spaces.
+    while (i < n && Strings.isHtmlSpace(value.charAt(i))) {
+      ++i;
+    }
+
+    while (i < n) {
+      // Find URL end.
+      int urlStart = i;
+      while (i < n && !Strings.isHtmlSpace(value.charAt(i))) {
+        ++i;
+      }
+      int urlEnd = i;
+      // Find metadata end.
+      while (i < n && Strings.isHtmlSpace(value.charAt(i))) {
+        ++i;
+      }
+      int metadataStart = i;
+      if (urlEnd < i) {  // Space required before metadata.
+        int floatEnd = Strings.skipValidFloatingPointNumber(value, i);
+        if (floatEnd >= 0) {
+          i = floatEnd;
+          if (i < n) {
+            // Skip over width specifier 'w', or pixel density specifier 'x'.
+            // We make this optional to support the <script srcset> proposal.
+            int ch = value.charAt(i) | 32;
+            if ('a' <= ch && ch <= 'z') {
+              ++i;
+            }
+          }
+        }
+      }
+      int metadataEnd = i;
+
+      if (urlStart < urlEnd) {
+        if (value.charAt(urlStart) == ',' || value.charAt(urlEnd - 1) == ',') {
+          // These introduce lexical ambiguity and are called out in the spec.
+          return null;
+        }
+        String okUrl = srcPolicy.apply(
+            elementName, "src", value.substring(urlStart, urlEnd));
+        if (okUrl != null && !okUrl.isEmpty()) {
+          if (sb.length() != 0) {
+            sb.append(", ");
+          }
+          sb.append(okUrl.replace(",", "%2c"));
+          if (metadataStart < metadataEnd) {
+            sb.append(' ');
+            sb.append(value, metadataStart, metadataEnd);
+          }
+        }
+      }
+      // Skip space before comma
+      while (i < n && Strings.isHtmlSpace(value.charAt(i))) {
+        ++i;
+      }
+      if (i == n || value.charAt(i) != ',') {
+        break;
+      }
+      ++i;
+      while (i < n && Strings.isHtmlSpace(value.charAt(i))) {
+        ++i;
+      }
+    }
+
+    if (i < n  // Unexpected trailing content.
+        || sb.length() == 0) {  // No URLs found.
+      return null;
+    }
+    return sb.toString();
+  }
+
+}
diff --git a/src/main/java/org/owasp/html/Strings.java b/src/main/java/org/owasp/html/Strings.java
@@ -181,5 +181,86 @@ static String stripHtmlSpaces(String s) {
     return s.substring(i, n);
   }
 
+  /**
+   * Parses a valid floating point number per the HTML5 spec.
+   * https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-floating-point-number
+   *
+   * @param start the start of the floating point number on s.
+   * @return the end of the floating point number if valid or -1 if not.
+   */
+  static int skipValidFloatingPointNumber(String value, int start) {
+    // A string is a valid floating-point number if it consists of:
+    int i = start;
+    final int n = value.length();
+
+    if (i >= n) {
+      return -1;
+    }
+
+    // 1. Optionally, a U+002D HYPHEN-MINUS character (-).
+    if (value.charAt(i) == '-') {
+      ++i;
+    }
+    // 2. One or both of the following, in the given order:
+    boolean hasIntegerPart = false;
+    //    1. A series of one or more ASCII digits.
+    while (i < n) {
+      char ch = value.charAt(i);
+      if ('0' <= ch && ch <= '9') {
+        ++i;
+        hasIntegerPart = true;
+      } else {
+        break;
+      }
+    }
+    //    2. Both of the following, in the given order:
+    //       1. A single U+002E FULL STOP character (.).
+    //       2. A series of one or more ASCII digits.
+    if (i < n && value.charAt(i) == '.') {
+      ++i;
+      while (i < n) {
+        char ch = value.charAt(i);
+        if ('0' <= ch && ch <= '9') {
+          ++i;
+          hasIntegerPart = true;
+        } else {
+          break;
+        }
+      }
+    }
+    if (!hasIntegerPart) {
+      return -1;
+    }
+    // 3. Optionally:
+    //    1. Either a U+0065 LATIN SMALL LETTER E character (e)
+    //       or a U+0045 LATIN CAPITAL LETTER E character (E).
+    if (i < n && (value.charAt(i) | 32) == 'e') {
+      ++i;
+      //    2. Optionally, a U+002D HYPHEN-MINUS character (-) or
+      //       U+002B PLUS SIGN character (+).
+      if (i < n) {
+        char ch = value.charAt(i);
+        if (ch == '+' || ch == '-') {
+          ++i;
+        }
+      }
+      //    3. A series of one or more ASCII digits.
+      boolean hasExponent = false;
+      while (i < n) {
+        char ch = value.charAt(i);
+        if ('0' <= ch && ch <= '9') {
+          ++i;
+          hasExponent = true;
+        } else {
+          break;
+        }
+      }
+      if (!hasExponent) {
+        return -1;
+      }
+    }
+    return i;
+  }
+
   private Strings() { /* uninstantiable */ }
 }
diff --git a/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java b/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java
@@ -30,6 +30,7 @@
 
 import java.util.List;
 import java.util.Locale;
+import java.util.regex.Pattern;
 
 import org.junit.Test;
 
@@ -350,6 +351,81 @@ public String apply(String elementName, List<String> attrs) {
             + "<IMAGE>"));
   }
 
+  @Test
+  public static final void testImgSrcsetSyntax() {
+    assertEquals(
+        ""
+        + "<img srcset=\"http://example.com/foo.png\" />\n"
+        + "<img srcset=\"http://example.com/foo.png 640w\" />\n"
+        + "<img srcset=\"http://example.com/foo.png 48x\" />\n"
+        + "<img srcset=\"http://example.com/foo.png .123x\" />\n"
+        + "<img srcset=\"http://example.com/foo.png .123e2x\" />\n"
+        + "<img srcset=\"http://example.com/foo.png 123.456x\" />\n"
+        + "<img srcset=\"/big.png 64w, /little.png\" />\n"
+        + "<img srcset=\"/big.png 64w, /little.png\" />\n"
+        + "<img srcset=\"/big.png 64w, /little.png\" />\n"
+        + "<img srcset=\"foo%2cbar.png\" />\n"
+        + "empty: \n"
+        + "only space: \n"
+        + "only comma: \n"
+        + "comma at end: <img srcset=\"foo.png\" />\n"
+        + "comma stuck to url: \n"
+        + "commas inside: <img srcset=\"foo.png%2c%2cbar.png\" />\n"
+        + "double commas 1: \n"
+        + "double commas 2: \n"
+        + "bad url: <img srcset=\"foo.png 1w\" />\n",
+
+        apply(
+            new HtmlPolicyBuilder()
+            .allowElements("img")
+            .allowAttributes("srcset").onElements("img")
+            .allowStandardUrlProtocols(),
+            ""
+            + "<img srcset=\"http://example.com/foo.png\" />\n"
+            + "<img srcset=\"http://example.com/foo.png 640w\" />\n"
+            + "<img srcset=\"http://example.com/foo.png 48x\" />\n"
+            + "<img srcset=\"http://example.com/foo.png .123x\" />\n"
+            + "<img srcset=\"http://example.com/foo.png .123e2x\" />\n"
+            + "<img srcset=\"http://example.com/foo.png 123.456x\" />\n"
+            + "<img srcset=\"/big.png 64w, /little.png\" />\n"
+            + "<img srcset=\" /big.png 64w , /little.png\" />\n"
+            + "<img srcset=\"\t\t/big.png 64w\r\n,/little.png\t\t\" />\n"
+            + "<img srcset=\"foo,bar.png\" />\n"
+            + "empty: <img srcset=\"\" />\n"
+            + "only space: <img srcset=\"  \" />\n"
+            + "only comma: <img srcset=\",\" />\n"
+            + "comma at end: <img srcset=\"foo.png ,\" />\n"  // ok
+            + "comma stuck to url: <img srcset=\"bar.png,\" />\n"  // not ok
+            + "commas inside: <img srcset=\"foo.png,,bar.png\" />\n"  // escaped
+            + "double commas 1: <img srcset=\"a ,, b\" />\n"  // not ok
+            + "double commas 2: <img srcset=\"a , , b\" />\n"  // not ok
+            + "bad url: <img srcset=\"foo.png 1w, javascript:evil()\" />\n"
+            ));
+  }
+
+  @Test
+  public static final void testUrlChecksLayer() {
+    assertEquals(
+        ""
+        + "<img src=\"http://example.com/OK.png\" />\n"
+        + "\n"
+        + "<img srcset=\"http://example.com/bar.png#OK 1w\" />",
+
+        apply(
+            new HtmlPolicyBuilder()
+            .allowElements("img")
+            .allowAttributes("src", "srcset")
+                .matching(Pattern.compile(".*OK.*"))
+                .onElements("img")
+            .allowStandardUrlProtocols(),
+            ""
+            + "<img src=\"http://example.com/OK.png\" />\n"
+            + "<img src=\"http://example.com/\" />\n"
+            + "<img srcset=\"http://example.com/bar.png#OK 1w, javascript:alert%28%27OK%27%29\">"
+            )
+        );
+  }
+
   @Test
   public static final void testDuplicateAttributesDoNotReachElementPolicy() {
     final int[] idCount = new int[1];
