Repository: shapesecurity/salvation
Pull request #167 (2017-01-01 09:35:05)

fixes GH-166:
- handle `,` and `;` characters in source expression
- refactor path matching algorithm to reflect latest specification

michaelficarra: Needs a test for constructing a `HostSource` with a `,` or `;` in it and adding it to an existing policy. The policy should be safely escaped when rendered.

We should also make sure that path matches are escape-agnostic. What I meant by that is that unnecessarily-escaped characters in the path match their unescaped equivalents. It looks like you've implemented [§6.6.1.10](https://w3c.github.io/webappsec-csp/#match-paths) pretty directly, so it should already behave this way; we just need tests.
shekyan: > Needs a test for constructing a HostSource with a , or ; in it and adding it to an existing policy. The policy should be safely escaped when rendered.
I think https://github.com/shapesecurity/salvation/pull/167/files#diff-2bb2d7b2fa7ffdef22812c8d231f7337R1072 is minimally covers it but sure, will add explicit tests for that.
shekyan: Addressed comments
michaelficarra: We still need tests for invalid escapes in the paths. And tests for mixing them with valid escapes.

Patch:
diff --git a/src/main/java/com/shapesecurity/salvation/directiveValues/HostSource.java b/src/main/java/com/shapesecurity/salvation/directiveValues/HostSource.java
index 91d61b2..48cde48 100644
--- a/src/main/java/com/shapesecurity/salvation/directiveValues/HostSource.java
+++ b/src/main/java/com/shapesecurity/salvation/directiveValues/HostSource.java
@@ -9,8 +9,14 @@ import com.shapesecurity.salvation.interfaces.MatchesSource;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
 import java.util.Objects;
 
+
 public class HostSource implements SourceExpression, AncestorSource, MatchesSource {
     public static final HostSource WILDCARD = new HostSource(null, "*", Constants.EMPTY_PORT, null);
     private static final int WILDCARD_HASHCODE = 0x9F4E3EEA;
@@ -23,7 +29,7 @@ public class HostSource implements SourceExpression, AncestorSource, MatchesSour
         this.scheme = scheme;
         this.host = host;
         this.port = port;
-        this.path = path;
+        this.path = path != null ? path.replaceAll(";", "%3B").replaceAll(",", "%2C") : null;
     }
 
     @Override public boolean equals(@Nullable Object other) {
@@ -87,9 +93,8 @@ public class HostSource implements SourceExpression, AncestorSource, MatchesSour
                 (this.port == Constants.EMPTY_PORT ?
             uriUsesDefaultPort :
             (resource.port == Constants.EMPTY_PORT ? thisUsesDefaultPort : this.port == resource.port));
-        boolean pathMatches = this.path == null || (this.path.endsWith("/") ?
-            resource.path.startsWith(this.path) :
-            this.path.equals(resource.path));
+        boolean pathMatches = pathMatches(this.path, resource.path);
+
         return schemeMatches && hostMatches && portMatches && pathMatches;
     }
 
@@ -128,4 +133,68 @@ public class HostSource implements SourceExpression, AncestorSource, MatchesSour
             (isDefaultPort ? "" : ":" + (this.port == Constants.WILDCARD_PORT ? "*" : this.port)) +
             (this.path == null ? "" : this.path);
     }
+
+    public static boolean pathMatches(@Nullable String pathA, @Nullable String pathB) {
+
+        if (pathA == null || pathA.isEmpty()) {
+            return true;
+        }
+
+        if (pathB == null || pathB.isEmpty()) {
+            return false;
+        }
+
+        if (pathA.matches("/") && pathB.isEmpty()) {
+            return true;
+        }
+
+        boolean exactMatch = pathA.endsWith("/") ? false : true;
+
+        List<String> pathListA = splitBySpec(pathA, '/');
+        List<String> pathListB = splitBySpec(pathB, '/');
+
+        if (pathListA.size() > pathListB.size())
+            return false;
+
+        if (exactMatch && pathListA.size() != pathListB.size()) {
+            return false;
+        }
+
+        if (!exactMatch) {
+            pathListA.remove(pathListA.size() - 1);
+        }
+
+        Iterator it1 = pathListA.iterator();
+        Iterator it2 = pathListB.iterator();
+
+        while (it1.hasNext()) {
+            String a = decodeString((String) it1.next());
+            String b = decodeString((String) it2.next());
+            if (!a.equals(b)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    public static String decodeString(@Nonnull String s) {
+        try {
+            return URLDecoder.decode(s, "UTF-8");
+        } catch (UnsupportedEncodingException e) {
+            return s;
+        }
+    }
+
+    public static List<String> splitBySpec(@Nonnull String s, char delim) {
+        int off = 0;
+        int next;
+        ArrayList<String> list = new ArrayList<>();
+        while ((next = s.indexOf(delim, off)) != -1) {
+            list.add(s.substring(off, next));
+            off = next + 1;
+        }
+
+        list.add(s.substring(off, s.length()));
+        return list;
+    }
 }


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation/ParserTest.java b/src/test/java/com/shapesecurity/salvation/ParserTest.java
index 4544294..67bb8ad 100644
--- a/src/test/java/com/shapesecurity/salvation/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation/ParserTest.java
@@ -209,6 +209,19 @@ public class ParserTest extends CSPTest {
         assertEquals("script-src a 'nonce-1234'; style-src a 'nonce-1234'", p.show());
         p = parse("script-src 'nonce-1234'; style-src 'nonce-1234'; default-src a");
         assertEquals("script-src 'nonce-1234'; style-src 'nonce-1234'; default-src a", p.show());
+
+        p = parse("script-src http://example.org/cpproducts/index.htm;jsessionid=0C159CF2854B7519C5379634FAC36577");
+        assertEquals("script-src http://example.org/cpproducts/index.htm", p.show());
+
+        p = parse("script-src http://example.org/cpproducts/index.htm%3Bjsessionid=0C159CF2854B7519C5379634FAC36577");
+        assertEquals("script-src http://example.org/cpproducts/index.htm%3Bjsessionid=0C159CF2854B7519C5379634FAC36577", p.show());
+
+        p = parse("script-src http://example.org/cpproducts/index.htm,jsessionid=0C159CF2854B7519C5379634FAC36577");
+        assertEquals("script-src http://example.org/cpproducts/index.htm", p.show());
+
+        p = parse("script-src http://example.org/cpproducts/index.htm%2Cjsessionid=0C159CF2854B7519C5379634FAC36577");
+        assertEquals("script-src http://example.org/cpproducts/index.htm%2Cjsessionid=0C159CF2854B7519C5379634FAC36577", p.show());
+
     }
 
     @Test public void testAncestorSource() {
diff --git a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
index 62633ea..aedddd4 100644
--- a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
+++ b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
@@ -23,6 +23,7 @@ import com.shapesecurity.salvation.directives.ScriptSrcDirective;
 import com.shapesecurity.salvation.directives.StyleSrcDirective;
 import org.junit.Test;
 
+import java.util.Collections;
 import java.util.stream.Stream;
 
 import static org.junit.Assert.assertEquals;
@@ -389,6 +390,7 @@ public class PolicyQueryingTest extends CSPTest {
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com")));
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/")));
         assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/a")));
+        assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com////a")));
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/A")));
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/a/")));
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/a/b")));
@@ -439,8 +441,33 @@ public class PolicyQueryingTest extends CSPTest {
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/A/B/")));
         assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/a/b/c")));
         assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/a/b/C")));
+
+        p = Parser.parse("script-src example.com/a/b%3Bzzz%2Cqqq", "http://example.com");
+        assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/a/b%3Bzzz")));
+        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/a/b%3Bzzz%2Cqqq")));
+        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/a/b;zzz,qqq")));
+
+        p = Parser.parse("script-src example.com/%21/%24/%26/%27/%28/%29/%2A/%2C/%3A/%3B", "http://example.com");
+        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/!/$/&/'/(/)/*/,/:/;")));
+        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/%21/%24/%26/%27/%28/%29/%2A/%2C/%3A/%3B")));
+
+        // TODO: this is valid in Chrome
+//        p = Parser.parse("script-src example.com/%GG", "http://example.com");
+//        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/%GG")));
+//      // TODO: this is valid in Chrome
+//        p = Parser.parse("script-src example.com/%%GGpath", "http://example.com");
+//        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/%GG")));
+
+        // TODO: we should throw on this, as it isn't valid UTF-8 percent encoding
+//        p = Parser.parse("script-src example.com/%ef", "http://example.com");
+
+        p = Parser.parse("script-src example.com/%C3%AF/", "http://example.com");
+        assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com/%EF/")));
+        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/%C3%AF/")));
+        assertTrue(p.allowsScriptFromSource(URI.parse("http://example.com/%C3%AF/%65")));
     }
 
+
     @Test public void testLocalSchemes() {
         Policy p = Parser.parse("script-src *.example.com data: blob:; frame-ancestors data: about:", "http://example.com");
         assertTrue(p.allowsScriptFromSource(new GUID("data:")));
@@ -1062,6 +1089,34 @@ public class PolicyQueryingTest extends CSPTest {
         assertFalse(p.containsSourceExpression(ImgSrcDirective.class, x -> x.equals(new HostSource(null, "b", Constants.EMPTY_PORT, null))));
     }
 
+    @Test public void testDelimitersInHostSource() {
+        Policy p;
+        HostSource h;
+        ScriptSrcDirective d;
+
+        p = Parser.parse("script-src 'self'", "http://example.com");
+        h = new HostSource(null, "example.com", Constants.EMPTY_PORT, "/a;jsessionid=123");
+        d = new ScriptSrcDirective(Collections.singleton(h));
+        p.unionDirective(d);
+        assertEquals("script-src 'self' example.com/a%3Bjsessionid=123", p.show());
+
+        p = Parser.parse("script-src 'self'", "http://example.com");
+        h = new HostSource(null, "example.com", Constants.EMPTY_PORT, "/a,b");
+        d = new ScriptSrcDirective(Collections.singleton(h));
+        p.unionDirective(d);
+        assertEquals("script-src 'self' example.com/a%2Cb", p.show());
+
+        p = Parser.parse("script-src example.com/a%3Bjsessionid=123", "http://example.com");
+        assertTrue(p.containsSourceExpression(ScriptSrcDirective.class, x -> x.equals(new HostSource(null, "example.com", Constants.EMPTY_PORT, "/a%3Bjsessionid=123"))));
+        assertTrue(p.containsSourceExpression(ScriptSrcDirective.class, x -> x.equals(new HostSource(null, "example.com", Constants.EMPTY_PORT, "/a;jsessionid=123"))));
+        assertFalse(p.containsSourceExpression(ScriptSrcDirective.class, x -> x.equals(new HostSource(null, "example.com", Constants.EMPTY_PORT, null))));
+
+        p = Parser.parse("script-src example.com/a%2Cjsessionid=123", "http://example.com");
+        assertTrue(p.containsSourceExpression(ScriptSrcDirective.class, x -> x.equals(new HostSource(null, "example.com", Constants.EMPTY_PORT, "/a%2Cjsessionid=123"))));
+        assertTrue(p.containsSourceExpression(ScriptSrcDirective.class, x -> x.equals(new HostSource(null, "example.com", Constants.EMPTY_PORT, "/a,jsessionid=123"))));
+        assertFalse(p.containsSourceExpression(ScriptSrcDirective.class, x -> x.equals(new HostSource(null, "example.com", Constants.EMPTY_PORT, null))));
+    }
+
     @Test public void testSourceExpressionStream() {
         Policy p;
         Stream<SourceExpression> s;
diff --git a/src/test/java/com/shapesecurity/salvation/URITest.java b/src/test/java/com/shapesecurity/salvation/URITest.java
deleted file mode 100644
index 0953eca..0000000
--- a/src/test/java/com/shapesecurity/salvation/URITest.java
+++ /dev/null
@@ -1,20 +0,0 @@
-package com.shapesecurity.salvation;
-
-import com.shapesecurity.salvation.data.URI;
-import org.junit.Test;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
-public class URITest extends CSPTest {
-
-    @Test public void testURIandOrigins() {
-        URI u1 = URI.parse("http://a/123");
-        URI u2 = URI.parse("http://a:80/");
-        u1 = URI.parseWithOrigin(URI.parse("https://www"), "/34");
-        assertEquals("abs uri", "https://www/34", u1.show());
-        u1 = URI.parse("http://a:80");
-        u2 = URI.parse("http://a");
-        assertTrue("URIs are equal", u1.equals(u2));
-    }
-}
diff --git a/src/test/java/com/shapesecurity/salvation/UtilsTest.java b/src/test/java/com/shapesecurity/salvation/UtilsTest.java
new file mode 100644
index 0000000..e123d44
--- /dev/null
+++ b/src/test/java/com/shapesecurity/salvation/UtilsTest.java
@@ -0,0 +1,31 @@
+package com.shapesecurity.salvation;
+
+import com.shapesecurity.salvation.data.URI;
+import com.shapesecurity.salvation.directiveValues.HostSource;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class UtilsTest extends CSPTest {
+
+    @Test public void testURIandOrigins() {
+        URI u1 = URI.parse("http://a/123");
+        URI u2 = URI.parse("http://a:80/");
+        u1 = URI.parseWithOrigin(URI.parse("https://www"), "/34");
+        assertEquals("abs uri", "https://www/34", u1.show());
+        u1 = URI.parse("http://a:80");
+        u2 = URI.parse("http://a");
+        assertTrue("URIs are equal", u1.equals(u2));
+    }
+
+    @Test public void testSplitBySpec() {
+        assertEquals("[]", HostSource.splitBySpec("", '/').toString());
+        assertEquals("[, ]", HostSource.splitBySpec("/", '/').toString());
+        assertEquals("[a, ]", HostSource.splitBySpec("a/", '/').toString());
+        assertEquals("[, a]", HostSource.splitBySpec("/a", '/').toString());
+        assertEquals("[, a, ]", HostSource.splitBySpec("/a/", '/').toString());
+        assertEquals("[, a, b]", HostSource.splitBySpec("/a/b", '/').toString());
+        assertEquals("[, a, b, ]", HostSource.splitBySpec("/a/b/", '/').toString());
+    }
+}