issue 72: CSS url handling unimplemented. Implemented it and enabled via .allowUrlsInStyles(...) to prevent changing the meaning of existing policies

mikesamuel · mikesamuel · commit f76a8a710277 · 2016-06-13T16:05:09.000-04:00
diff --git a/src/main/java/org/owasp/html/HtmlPolicyBuilder.java b/src/main/java/org/owasp/html/HtmlPolicyBuilder.java
@@ -36,6 +36,7 @@
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.NotThreadSafe;
 
+import com.google.common.base.Function;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -176,6 +177,9 @@ public class HtmlPolicyBuilder {
       HtmlStreamEventProcessor.Processors.IDENTITY;
   private HtmlStreamEventProcessor preprocessor =
       HtmlStreamEventProcessor.Processors.IDENTITY;
+  private CssSchema stylingPolicySchema = null;
+  private AttributePolicy styleUrlPolicy =
+      AttributePolicy.REJECT_ALL_ATTRIBUTE_POLICY;
   private boolean requireRelNofollowOnLinks;
 
   /**
@@ -444,8 +448,34 @@ public HtmlPolicyBuilder allowStyling() {
    */
   public HtmlPolicyBuilder allowStyling(CssSchema whitelist) {
     invalidateCompiledState();
-    allowAttributesGlobally(
-        new StylingPolicy(whitelist), ImmutableList.of("style"));
+
+    // Allow the style attribute, and then we will fix it up later.  This allows
+    // us to attach the final URL policy to the style attribute policy, while
+    // still not allowing styles when allowStyling is followed by a call to
+    // disallowAttributesGlobally("style").
+    this.allowAttributesGlobally(
+        AttributePolicy.IDENTITY_ATTRIBUTE_POLICY, ImmutableList.of("style"));
+
+    this.stylingPolicySchema =
+        this.stylingPolicySchema == null
+        ? whitelist
+        : CssSchema.union(stylingPolicySchema, whitelist);
+    return this;
+  }
+
+  /**
+   * Allow URLs in CSS styles.
+   * URLs in CSS are typically loaded without user-interaction, the way links
+   * are, so a greater degree of scrutiny is warranted.
+   *
+   * @param newStyleUrlPolicy receives URLs from the CSS that pass the allowed
+   *     protocol policies, and may return null to veto the URL or the URL
+   *     to use.  URLs will be reported as content in {@code <img src=...>}.
+   */
+  public HtmlPolicyBuilder allowUrlsInStyles(
+      AttributePolicy newStyleUrlPolicy) {
+    this.invalidateCompiledState();
+    this.styleUrlPolicy = newStyleUrlPolicy;
     return this;
   }
 
@@ -583,8 +613,9 @@ private ImmutableMap<String, ElementAndAttributePolicies> compilePolicies() {
     // allowing the attribute "href" globally with the identity policy but
     // not white-listing any protocols, effectively disallows the "href"
     // attribute globally.
+    StylingPolicy stylingPolicy = null;
     {
-      AttributePolicy urlAttributePolicy;
+      final AttributePolicy urlAttributePolicy;
       if (allowedProtocols.size() == 3
           && allowedProtocols.contains("mailto")
           && allowedProtocols.contains("http")
@@ -594,6 +625,19 @@ private ImmutableMap<String, ElementAndAttributePolicies> compilePolicies() {
         urlAttributePolicy = new FilterUrlByProtocolAttributePolicy(
             allowedProtocols);
       }
+
+      if (this.stylingPolicySchema != null) {
+        final AttributePolicy styleUrlPolicyFinal = AttributePolicy.Util.join(
+            styleUrlPolicy, urlAttributePolicy);
+        stylingPolicy = new StylingPolicy(
+            stylingPolicySchema,
+            new Function<String, String>() {
+              public String apply(String url) {
+                return styleUrlPolicyFinal.apply("img", "src", url);
+              }
+            });
+      }
+
       Set<String> toGuard = Sets.newLinkedHashSet(URL_ATTRIBUTE_NAMES);
       for (String urlAttributeName : URL_ATTRIBUTE_NAMES) {
         if (globalAttrPolicies.containsKey(urlAttributeName)) {
@@ -636,6 +680,9 @@ private ImmutableMap<String, ElementAndAttributePolicies> compilePolicies() {
         // twice.  ImmutableMap.Builder hates that.
         if (globalAttrPolicies.containsKey(attributeName)) { continue; }
         AttributePolicy policy = ape.getValue();
+        if ("style".equals(attributeName)) {
+          policy = AttributePolicy.Util.join(policy, stylingPolicy);
+        }
         if (!AttributePolicy.REJECT_ALL_ATTRIBUTE_POLICY.equals(policy)) {
           attrs.put(attributeName, policy);
         }
@@ -645,6 +692,9 @@ private ImmutableMap<String, ElementAndAttributePolicies> compilePolicies() {
         String attributeName = ape.getKey();
         AttributePolicy policy = AttributePolicy.Util.join(
             elAttrPolicies.get(attributeName), ape.getValue());
+        if ("style".equals(attributeName)) {
+          policy = AttributePolicy.Util.join(policy, stylingPolicy);
+        }
         if (!AttributePolicy.REJECT_ALL_ATTRIBUTE_POLICY.equals(policy)) {
           attrs.put(attributeName, policy);
         }
diff --git a/src/main/java/org/owasp/html/StylingPolicy.java b/src/main/java/org/owasp/html/StylingPolicy.java
@@ -33,6 +33,7 @@
 import javax.annotation.Nullable;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
 import com.google.common.collect.Lists;
 
 /**
@@ -44,9 +45,11 @@
 final class StylingPolicy implements AttributePolicy {
 
   final CssSchema cssSchema;
+  final Function<String, String> urlRewriter;
 
-  StylingPolicy(CssSchema cssSchema) {
+  StylingPolicy(CssSchema cssSchema, Function<String, String> urlRewriter) {
     this.cssSchema = cssSchema;
+    this.urlRewriter = urlRewriter;
   }
 
   public @Nullable String apply(
@@ -85,11 +88,27 @@ private void closeQuotedIdents() {
         }
       }
 
+      private void sanitizeAndAppendUrl(String urlContent) {
+        if (urlContent.length() < 1024) {
+          String rewrittenUrl = urlRewriter.apply(urlContent);
+          if (rewrittenUrl != null && !rewrittenUrl.isEmpty()) {
+            if (hasTokens) { sanitizedCss.append(' '); }
+            sanitizedCss.append("url('").append(rewrittenUrl).append("')");
+            hasTokens = true;
+          }
+        }
+      }
+
       public void url(String token) {
         closeQuotedIdents();
-        //if ((schema.bits & CssSchema.BIT_URL) != 0) {
-        // TODO: sanitize the URL.
-        //}
+        if (cssProperty != null) {
+          if ((cssProperty.bits & CssSchema.BIT_URL) != 0) {
+            String urlContent = CssGrammar.cssContent(
+                Strings.stripHtmlSpaces(  // TODO: css spaces
+                    token.substring(4, token.length() - 1)));
+            sanitizeAndAppendUrl(urlContent);
+          }
+        }
       }
 
       public void startProperty(String propertyName) {
@@ -134,7 +153,7 @@ && isAlphanumericOrSpace(token, 1, token.length() - 1)) {
             emitToken(Strings.toLowerCase(token));
           } else if (meaning == CssSchema.BIT_URL) {
             // convert to a URL token and hand-off to the appropriate method
-            // url("url(" + token + ")");  // TODO: %-encode properly
+            sanitizeAndAppendUrl(CssGrammar.cssContent(token));
           }
         }
       }
@@ -227,5 +246,4 @@ public boolean equals(Object o) {
   public int hashCode() {
     return cssSchema.hashCode();
   }
-
 }
diff --git a/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java b/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java
@@ -479,6 +479,95 @@ public String toString() {
 
   }
 
+  @Test
+  public static final void testBackgroundImageWithUrl() {
+    PolicyFactory policy = new HtmlPolicyBuilder()
+        .allowStandardUrlProtocols()
+        .allowStyling()
+        .allowUrlsInStyles(AttributePolicy.IDENTITY_ATTRIBUTE_POLICY)
+        .allowElements("div")
+        .toFactory();
+    String unsafeHtml = policy.sanitize(
+        "<html><head><title>test</title></head><body>" +
+        "<div style='"
+        + "color: red; background-image: "
+        + "url(http://example.com/foo.png)" +
+        "'>div content" +
+        "</div></body></html>");
+    String safeHtml = policy.sanitize(unsafeHtml);
+    String expected =
+        "<div style=\""
+        + "color:red;background-image:"
+        + "url(&#39;http://example.com/foo.png&#39;)"
+        + "\">div content</div>";
+    assertEquals(expected, safeHtml);
+  }
+
+  @Test
+  public static final void testBackgroundImageWithImageFunction() {
+    PolicyFactory policy = new HtmlPolicyBuilder()
+        .allowStandardUrlProtocols()
+        .allowStyling()
+        .allowUrlsInStyles(AttributePolicy.IDENTITY_ATTRIBUTE_POLICY)
+        .allowElements("div")
+        .toFactory();
+    String unsafeHtml = policy.sanitize(
+        "<html><head><title>test</title></head><body>" +
+        "<div style='" +
+        "color: red; background-image: " +
+        "image(\"blue sky.png\", blue)'>" +
+        "div content" +
+        "</div></body></html>");
+    String safeHtml = policy.sanitize(unsafeHtml);
+    String expected =
+        "<div style=\""
+        + "color:red;background-image:"
+        + "image( url(&#39;blue%20sky.png&#39;) , blue )"
+        + "\">div content</div>";
+    assertEquals(expected, safeHtml);
+  }
+
+  @Test
+  public static final void testBackgroundWithUrls() {
+    HtmlPolicyBuilder builder = new HtmlPolicyBuilder()
+        .allowStandardUrlProtocols()
+        .allowStyling()
+        .allowElements("div");
+
+    PolicyFactory noUrlsPolicy = builder.toFactory();
+    PolicyFactory urlsPolicy = builder
+        .allowUrlsInStyles(AttributePolicy.IDENTITY_ATTRIBUTE_POLICY)
+        .toFactory();
+
+    String unsafeHtml =
+        "<div style=\"background:&quot;//evil.org/foo.png&quot;\"></div>";
+
+    String safeWithUrls =
+        "<div style=\"background:url(&#39;//evil.org/foo.png&#39;)\"></div>";
+    String safeWithoutUrls = "<div></div>";
+
+    assertEquals(safeWithoutUrls, noUrlsPolicy.sanitize(unsafeHtml));
+    assertEquals(safeWithUrls, urlsPolicy.sanitize(unsafeHtml));
+  }
+
+  @Test
+  public static final void testBackgroundsThatViolateGlobalUrlPolicy() {
+    PolicyFactory policy = new HtmlPolicyBuilder()
+        .allowStandardUrlProtocols()
+        .allowStyling()
+        .allowElements("div")
+        .allowUrlsInStyles(AttributePolicy.IDENTITY_ATTRIBUTE_POLICY)
+        .toFactory();
+
+    String unsafeHtml =
+        "<div style=\"background:'javascript:alert(1337)'\"></div>";
+    String safeHtml = "<div></div>";
+
+    assertEquals(safeHtml, policy.sanitize(unsafeHtml));
+
+  }
+
+
 
   private static String apply(HtmlPolicyBuilder b) {
     return apply(b, EXAMPLE);
diff --git a/src/test/java/org/owasp/html/SanitizersTest.java b/src/test/java/org/owasp/html/SanitizersTest.java
@@ -324,7 +324,7 @@ public static final void testAndOrdering() {
         + "g"
         + "<img src=\"http://example.org\"/>oogle</a>";
     String want = ""
-        + "xss<a href=\"http://www.google.de\" style=\"color:red\""
+        + "xss<a href=\"http://www.google.de\" style=\"color:red;\""
         + " rel=\"nofollow\">"
         + "g"
         + "<img src=\"http://example.org\" />oogle</a>";
diff --git a/src/test/java/org/owasp/html/StylingPolicyTest.java b/src/test/java/org/owasp/html/StylingPolicyTest.java
@@ -32,6 +32,8 @@
 
 import org.junit.Test;
 
+import com.google.common.base.Function;
+
 import junit.framework.TestCase;
 
 @SuppressWarnings("javadoc")
@@ -65,8 +67,10 @@ public static final void testColors() {
     assertSanitizedCss(null, "color: 000");
     assertSanitizedCss(null, "background-color: 000");
     // Not colors.
-    assertSanitizedCss(null, "background: \"pwned.jpg\"");
-    assertSanitizedCss(null, "background: url(pwned.jpg)");
+    assertSanitizedCss(
+        "background:url('pic.jpg#sanitized')", "background: \"pic.jpg\"");
+    assertSanitizedCss(
+        "background:url('pic.jpg#sanitized')", "background: url(pic.jpg)");
     assertSanitizedCss(null, "color:#urlabc");
     assertSanitizedCss(null, "color:#urlabcd");
   }
@@ -211,7 +215,7 @@ public static final void testBoxProperties() {
   }
 
   @Test
-  public static final void testUrls() {
+  public static final void testLongUrls() {
     // Test that a long URL does not blow out the stack or consume quadratic
     // amounts of processor as when the CSS lexer was implemented as a bunch of
     // regular expressions.
@@ -291,9 +295,42 @@ public static final void testUrls() {
     assertSanitizedCss(null, longUrl);
   }
 
+  @Test
+  public static final void testUrls() {
+    assertSanitizedCss(
+        "background-image:url('foo.gif#sanitized')",
+        "background-image: \"foo.gif\"");
+    assertSanitizedCss(
+        "background-image:url('foo.gif#sanitized')",
+        "background-image: 'foo.gif'");
+    assertSanitizedCss(
+        "background-image:url('foo.gif#sanitized')",
+        "background-image:url(foo.gif)");
+    assertSanitizedCss(
+        "background-image:url('foo.gif#sanitized')",
+        "background-image : url( foo.gif )");
+    assertSanitizedCss(
+        "background-image:url('foo.gif#sanitized')",
+        "background-image: url('foo.gif')");
+    assertSanitizedCss(
+        "background-image:url('foo.gif#sanitized')",
+        "background-image: URL( \"foo.gif\" )");
+  }
+
   private static void assertSanitizedCss(
       @Nullable String expectedCss, String css) {
-    StylingPolicy stylingPolicy = new StylingPolicy(CssSchema.DEFAULT);
+    StylingPolicy stylingPolicy = new StylingPolicy(
+        CssSchema.DEFAULT,
+        new Function<String, String>() {
+          public String apply(String url) {
+            String safeUrl =
+                StandardUrlAttributePolicy.INSTANCE.apply("img", "src", url);
+            if (safeUrl != null) {
+              return safeUrl + "#sanitized";
+            }
+            return null;
+          }
+        });
     assertEquals(expectedCss, stylingPolicy.sanitizeCssProperties(css));
   }
 }
