Repository: shapesecurity/salvation
Pull request #137 (2016-04-27 04:46:32)

Fixes GH-135
- do not treat `default-src *` equivalent to an empty policy
- fix assumption in query API that empty policy is most permissive


Patch:
diff --git a/src/main/java/com/shapesecurity/salvation/data/Policy.java b/src/main/java/com/shapesecurity/salvation/data/Policy.java
index 41c4886..2ffb524 100644
--- a/src/main/java/com/shapesecurity/salvation/data/Policy.java
+++ b/src/main/java/com/shapesecurity/salvation/data/Policy.java
@@ -81,9 +81,7 @@ public class Policy implements Show {
         DefaultSrcDirective defaultSrcDirective = this.getDirectiveByType(DefaultSrcDirective.class);
         Set<SourceExpression> defaultSources;
         if (defaultSrcDirective == null) {
-            defaultSources = new LinkedHashSet<>();
-            defaultSources.add(HostSource.WILDCARD);
-            this.directives.put(DefaultSrcDirective.class, new DefaultSrcDirective(defaultSources));
+            return;
         } else {
             defaultSources = defaultSrcDirective.values().collect(Collectors.toCollection(LinkedHashSet::new));
         }
@@ -172,49 +170,40 @@ public class Policy implements Show {
         DefaultSrcDirective defaultSrcDirective = this.getDirectiveByType(DefaultSrcDirective.class);
 
         Set<SourceExpression> defaultSources;
-        if (defaultSrcDirective == null) {
-            defaultSources = new LinkedHashSet<>();
-            defaultSources.add(HostSource.WILDCARD);
-        } else {
+
+        if (defaultSrcDirective != null) {
             defaultSources = defaultSrcDirective.values().collect(Collectors.toCollection(LinkedHashSet::new));
-        }
 
-        // * remove source directives that are equivalent to default-src
-        this.eliminateRedundantSourceExpression(defaultSources, ScriptSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, StyleSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, ImgSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, ChildSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, ConnectSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, FontSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, MediaSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, ObjectSrcDirective.class);
-        this.eliminateRedundantSourceExpression(defaultSources, ManifestSrcDirective.class);
-
-        // * remove default-src nonces if the policy contains both script-src and style-src directives
-        if (this.directives.containsKey(ScriptSrcDirective.class) && this.directives
-            .containsKey(StyleSrcDirective.class)) {
-            defaultSources.removeIf(x -> x instanceof NonceSource);
-            defaultSrcDirective = new DefaultSrcDirective(defaultSources);
-            this.directives.put(DefaultSrcDirective.class, defaultSrcDirective);
-        }
-
-        // * remove unnecessary default-src directives if all source directives exist
-        if (this.directives.containsKey(ScriptSrcDirective.class) &&
-            this.directives.containsKey(StyleSrcDirective.class) &&
-            this.directives.containsKey(ImgSrcDirective.class) &&
-            this.directives.containsKey(ChildSrcDirective.class) &&
-            this.directives.containsKey(ConnectSrcDirective.class) &&
-            this.directives.containsKey(FontSrcDirective.class) &&
-            this.directives.containsKey(MediaSrcDirective.class) &&
-            this.directives.containsKey(ObjectSrcDirective.class) &&
-            this.directives.containsKey(ManifestSrcDirective.class)) {
-            this.directives.remove(DefaultSrcDirective.class);
-        }
-
-        // remove `default-src *`
-        if (defaultSources.size() == 1) {
-            SourceExpression first = defaultSources.iterator().next();
-            if (first instanceof HostSource && ((HostSource) first).isWildcard()) {
+
+            // * remove source directives that are equivalent to default-src
+            this.eliminateRedundantSourceExpression(defaultSources, ScriptSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, StyleSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, ImgSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, ChildSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, ConnectSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, FontSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, MediaSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, ObjectSrcDirective.class);
+            this.eliminateRedundantSourceExpression(defaultSources, ManifestSrcDirective.class);
+
+            // * remove default-src nonces if the policy contains both script-src and style-src directives
+            if (this.directives.containsKey(ScriptSrcDirective.class) && this.directives
+                .containsKey(StyleSrcDirective.class)) {
+                defaultSources.removeIf(x -> x instanceof NonceSource);
+                defaultSrcDirective = new DefaultSrcDirective(defaultSources);
+                this.directives.put(DefaultSrcDirective.class, defaultSrcDirective);
+            }
+
+            // * remove unnecessary default-src directives if all source directives exist
+            if (this.directives.containsKey(ScriptSrcDirective.class) &&
+                this.directives.containsKey(StyleSrcDirective.class) &&
+                this.directives.containsKey(ImgSrcDirective.class) &&
+                this.directives.containsKey(ChildSrcDirective.class) &&
+                this.directives.containsKey(ConnectSrcDirective.class) &&
+                this.directives.containsKey(FontSrcDirective.class) &&
+                this.directives.containsKey(MediaSrcDirective.class) &&
+                this.directives.containsKey(ObjectSrcDirective.class) &&
+                this.directives.containsKey(ManifestSrcDirective.class)) {
                 this.directives.remove(DefaultSrcDirective.class);
             }
         }
@@ -318,7 +307,7 @@ public class Policy implements Show {
             return true;
         DefaultSrcDirective defaultSrcDirective = this.getDirectiveByType(DefaultSrcDirective.class);
         if (defaultSrcDirective == null) {
-            return true;
+            return false;
         }
         return defaultSrcDirective.matchesHash(algorithm, hashValue);
     }
@@ -328,7 +317,7 @@ public class Policy implements Show {
             return true;
         DefaultSrcDirective defaultSrcDirective = this.getDirectiveByType(DefaultSrcDirective.class);
         if (defaultSrcDirective == null) {
-            return true;
+            return false;
         }
         return defaultSrcDirective.matchesNonce(nonce);
     }
@@ -342,7 +331,7 @@ public class Policy implements Show {
     private boolean defaultsAllowSource(@Nonnull URI source) {
         DefaultSrcDirective defaultSrcDirective = this.getDirectiveByType(DefaultSrcDirective.class);
         if (defaultSrcDirective == null) {
-            return true;
+            return false;
         }
         return defaultSrcDirective.matchesSource(this.origin, source);
     }


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation/ParserTest.java b/src/test/java/com/shapesecurity/salvation/ParserTest.java
index 11719ec..89715a2 100644
--- a/src/test/java/com/shapesecurity/salvation/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation/ParserTest.java
@@ -23,10 +23,6 @@ public class ParserTest extends CSPTest {
     @Test public void testEmptyPolicy() {
         Policy p = parse("");
         assertNotNull("empty policy should not be null", p);
-        assertTrue("resource is allowed", p.allowsScriptFromSource(URI.parse("https://www.def.am")));
-        assertTrue("resource is allowed", p.allowsScriptWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value(
-            "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
-        assertTrue("resource is allowed", p.allowsScriptWithNonce(new Base64Value("0gQAAA==")));
     }
 
     @Test public void testDuplicates() {
@@ -188,11 +184,11 @@ public class ParserTest extends CSPTest {
         assertEquals("directive-name, host-source http://a:*", "script-src http://a:*",
             parseAndShow("script-src http://a:*"));
 
-        assertEquals("optimisation", "", parseAndShow("script-src example.com *"));
-        assertEquals("optimisation", "", parseAndShow("script-src 'self' *"));
+        assertEquals("optimisation", "script-src *", parseAndShow("script-src example.com *"));
+        assertEquals("optimisation", "script-src *", parseAndShow("script-src 'self' *"));
         assertEquals("optimisation", "script-src 'unsafe-inline'; style-src 'unsafe-inline'",
             parseAndShow("script-src 'unsafe-inline'; style-src 'unsafe-inline';"));
-        assertEquals("optimisation with network scheme", "", parseAndShow("script-src 'self' * ftp:"));
+        assertEquals("optimisation with network scheme", "script-src *", parseAndShow("script-src 'self' * ftp:"));
         assertEquals("optimisation with other scheme", "script-src data: *", parseAndShow("script-src 'self' * data:"));
         assertEquals("optimisation with other scheme", "script-src custom: *", parseAndShow("script-src 'self' * custom:"));
         assertEquals("optimisation with mixed schemes", "script-src custom: blob: *", parseAndShow("script-src 'self' * custom: ftp: blob:"));
@@ -258,17 +254,25 @@ public class ParserTest extends CSPTest {
         assertEquals("policy show", "", a.show());
 
         Policy b = parse("style-src *");
-        assertEquals("policy show", "", b.show());
+        assertEquals("policy show", "style-src *", b.show());
+
+        assertFalse("policy equality", a.equals(b));
+
+        a = parse("style-src 'self' *");
+        assertEquals("policy show", "style-src *", a.show());
+
+        b = parse("style-src *");
+        assertEquals("policy show", "style-src *", b.show());
 
         assertTrue("policy equality", a.equals(b));
 
         Policy c = parse("script-src *");
         b.union(c);
-        assertEquals("policy union", "", b.show());
+        assertEquals("policy union", "style-src *; script-src *", b.show());
 
         Policy d = parse("script-src abc");
         b.union(d);
-        assertEquals("policy union", "", b.show());
+        assertEquals("policy union", "style-src *; script-src *", b.show());
 
         a.setOrigin(URI.parse("http://qwe.zz:80"));
         assertEquals("policy origin", "http://qwe.zz", a.getOrigin().show());
@@ -817,8 +821,7 @@ public class ParserTest extends CSPTest {
 
         notices.clear();
         p = parseWithNotices("script-src *, ", notices);
-        // it actually optimised away
-        assertEquals(0, p.getDirectives().size());
+        assertEquals(1, p.getDirectives().size());
         assertEquals(1, notices.size());
         assertEquals("Expecting end of policy but found \",\".", notices.get(0).message);
     }
diff --git a/src/test/java/com/shapesecurity/salvation/PolicyMergeTest.java b/src/test/java/com/shapesecurity/salvation/PolicyMergeTest.java
index 2920cba..839420f 100644
--- a/src/test/java/com/shapesecurity/salvation/PolicyMergeTest.java
+++ b/src/test/java/com/shapesecurity/salvation/PolicyMergeTest.java
@@ -47,7 +47,7 @@ public class PolicyMergeTest extends CSPTest {
         p1 = parse("default-src *; script-src");
         p2 = parse("default-src; script-src b");
         p1.union(p2);
-        assertEquals("script-src b", p1.show());
+        assertEquals("default-src *; script-src b", p1.show());
 
         p1 = parse("default-src a");
         p2 = parse("default-src; script-src b");
@@ -83,6 +83,11 @@ public class PolicyMergeTest extends CSPTest {
     @Test public void testIntersect() {
         Policy p1, p2;
 
+        p1 = parse("default-src 'none';");
+        p2 = parse("default-src *;");
+        p1.intersect(p2);
+        assertEquals("default-src", p1.show());
+
         p1 = parse("default-src a; script-src b");
         p2 = parse("default-src c; img-src d");
         p1.intersect(p2);
@@ -126,7 +131,7 @@ public class PolicyMergeTest extends CSPTest {
         p1 = parse("default-src *; script-src *; style-src *:80");
         p2 = parse("default-src 'self'; script-src a");
         p1.intersect(p2);
-        assertEquals("style-src; default-src 'self'; script-src a", p1.show());
+        assertEquals("default-src 'self'; style-src; script-src a", p1.show());
 
         p1 = parse("default-src 'self'; script-src a");
         p2 = parse("default-src *; script-src *; style-src *:80");
@@ -313,7 +318,7 @@ public class PolicyMergeTest extends CSPTest {
         set.add(HostSource.WILDCARD);
         DefaultSrcDirective d3 = new DefaultSrcDirective(set);
         p.unionDirective(d3);
-        assertEquals("script-src a; report-uri http://example.com/z", p.show());
+        assertEquals("default-src *; script-src a; report-uri http://example.com/z", p.show());
 
         set.clear();
         p = Parser.parse("", "http://example.com");
@@ -324,7 +329,7 @@ public class PolicyMergeTest extends CSPTest {
 
         p.unionDirective(scriptSrcDirective);
         p.unionDirective(styleSrcDirective);
-        assertEquals("script-src 'nonce-Q-ecAIccSGatv6lJrCBVARPr' *; style-src 'nonce-Q-ecAIccSGatv6lJrCBVARPr' *",
+        assertEquals("script-src 'nonce-Q-ecAIccSGatv6lJrCBVARPr'; style-src 'nonce-Q-ecAIccSGatv6lJrCBVARPr'",
             p.show());
     }
 
diff --git a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
index 697d731..a7304ac 100644
--- a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
+++ b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
@@ -486,6 +486,19 @@ public class PolicyQueryingTest extends CSPTest {
         assertFalse(p.allowsScriptFromSource(new GUID("custom.scheme:")));
     }
 
+    @Test public void testEmptyPolicy() {
+        Policy p;
+
+        p = Parser.parse("", "http://example.com");
+        assertFalse(p.allowsScriptFromSource(URI.parse("http://example.com")));
+        assertFalse(p.allowsScriptFromSource(URI.parse("wss://example.com")));
+        assertFalse(p.allowsScriptWithNonce(new Base64Value("1234")));
+        assertFalse(p.allowsScriptWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value(
+            "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
+        assertFalse(p.allowsScriptFromSource(new GUID("custom.scheme:")));
+        assertFalse(p.allowsScriptFromSource(new GUID("data:")));
+    }
+
     @Test public void testHasSomeEffect() {
         Policy p = Parser.parse("", "http://example.com");
         assertFalse(p.hasSomeEffect());