Repository: shapesecurity/salvation
Pull request #136 (2016-04-25 04:09:27)

fixes GH-130
- remove 'unsafe-inline' if hash or nonce is present in the same source list
- warn about disabled 'unsafe-inline' in a source list containing hash or source
- tests

shekyan: Amended commit to address comments
shekyan: Addressed comments
michaelficarra: Beautiful. Thanks.

Patch:
diff --git a/src/main/java/com/shapesecurity/salvation/Parser.java b/src/main/java/com/shapesecurity/salvation/Parser.java
index 53ed5b6..5d20bc5 100644
--- a/src/main/java/com/shapesecurity/salvation/Parser.java
+++ b/src/main/java/com/shapesecurity/salvation/Parser.java
@@ -44,6 +44,9 @@ public class Parser {
         new DirectiveValueParseException("Invalid uri-reference");
     private static final DirectiveParseException NON_EMPTY_VALUE_TOKEN_LIST =
         new DirectiveParseException("Non-empty directive-value list");
+
+    private static final String unsafeInlineWarningMessage = "The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source.";
+    private enum SeenStates {SEEN_HASH, SEEN_NONE, SEEN_NONCE, SEEN_UNSAFE_INLINE};
     @Nonnull protected final Token[] tokens;
     @Nonnull private final Origin origin;
     protected int index = 0;
@@ -351,12 +354,18 @@ public class Parser {
     @Nonnull private Set<SourceExpression> parseSourceList() throws DirectiveParseException {
         Set<SourceExpression> sourceExpressions = new LinkedHashSet<>();
         boolean parseException = false;
-        boolean seenNone = false;
+        Set<SeenStates> seenStates = new HashSet<>();
         while (this.hasNext(SubDirectiveValueToken.class)) {
             try {
-                SourceExpression se = this.parseSourceExpression(seenNone, !sourceExpressions.isEmpty());
+                SourceExpression se = this.parseSourceExpression(seenStates, !sourceExpressions.isEmpty());
                 if (se == None.INSTANCE) {
-                    seenNone = true;
+                    seenStates.add(SeenStates.SEEN_NONE);
+                } else if (se == KeywordSource.UnsafeInline) {
+                    seenStates.add(SeenStates.SEEN_UNSAFE_INLINE);
+                } else if (se instanceof HashSource) {
+                    seenStates.add(SeenStates.SEEN_HASH);
+                } else if (se instanceof NonceSource) {
+                    seenStates.add(SeenStates.SEEN_NONCE);
                 }
                 sourceExpressions.add(se);
             } catch (DirectiveValueParseException e) {
@@ -369,10 +378,10 @@ public class Parser {
         return sourceExpressions;
     }
 
-    @Nonnull private SourceExpression parseSourceExpression(boolean seenNone, boolean seenSome)
+    @Nonnull private SourceExpression parseSourceExpression(Set<SeenStates> seenStates, boolean seenSome)
         throws DirectiveValueParseException {
         Token token = this.advance();
-        if (seenNone || seenSome && token.value.equalsIgnoreCase("'none'")) {
+        if (seenStates.contains(SeenStates.SEEN_NONE) || seenSome && token.value.equalsIgnoreCase("'none'")) {
             this.error(token, "'none' must not be combined with any other source-expression.");
             throw INVALID_SOURCE_EXPR;
         }
@@ -382,6 +391,9 @@ public class Parser {
             case "'self'":
                 return KeywordSource.Self;
             case "'unsafe-inline'":
+                if (seenStates.contains(SeenStates.SEEN_HASH) || seenStates.contains(SeenStates.SEEN_NONCE))  {
+                    this.warn(token, unsafeInlineWarningMessage);
+                }
                 return KeywordSource.UnsafeInline;
             case "'unsafe-eval'":
                 return KeywordSource.UnsafeEval;
@@ -394,6 +406,9 @@ public class Parser {
                     String nonce = token.value.substring(7, token.value.length() - 1);
                     NonceSource nonceSource = new NonceSource(nonce);
                     nonceSource.validationErrors().forEach(str -> this.warn(token, str));
+                    if (seenStates.contains(SeenStates.SEEN_UNSAFE_INLINE)) {
+                        this.warn(token, unsafeInlineWarningMessage);
+                    }
                     return nonceSource;
                 } else if (token.value.toLowerCase().startsWith("'sha")) {
                     HashSource.HashAlgorithm algorithm;
@@ -433,6 +448,9 @@ public class Parser {
                         this.error(token, e.getMessage());
                         throw INVALID_SOURCE_EXPR;
                     }
+                    if (seenStates.contains(SeenStates.SEEN_UNSAFE_INLINE)) {
+                        this.warn(token, unsafeInlineWarningMessage);
+                    }
                     return hashSource;
                 } else if (token.value.matches("^" + Constants.schemePart + ":$")) {
                     return new SchemeSource(token.value.substring(0, token.value.length() - 1));
diff --git a/src/main/java/com/shapesecurity/salvation/data/Policy.java b/src/main/java/com/shapesecurity/salvation/data/Policy.java
index e249f82..41c4886 100644
--- a/src/main/java/com/shapesecurity/salvation/data/Policy.java
+++ b/src/main/java/com/shapesecurity/salvation/data/Policy.java
@@ -144,6 +144,9 @@ public class Policy implements Show {
                         .filter(x -> !(x instanceof HostSource))
                         // * remove network-schemes in source list that contains *
                         .filter(x -> !(x instanceof SchemeSource) || !((SchemeSource) x).matchesNetworkScheme())
+                        // remove 'unsafe-inline' if source list contains hash or nonce
+                        .filter(x -> !((x == KeywordSource.UnsafeInline) &&
+                            (sourceListDirective.containsNonceSource() || sourceListDirective.containsHashSource())))
                         .collect(Collectors.toCollection(LinkedHashSet::new));
                     newSources.add(star.get());
                     this.directives.put(entry.getKey(), sourceListDirective.construct(newSources));
diff --git a/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java b/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java
index b297d91..535631c 100644
--- a/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java
+++ b/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java
@@ -1,10 +1,7 @@
 package com.shapesecurity.salvation.directives;
 
 import com.shapesecurity.salvation.data.*;
-import com.shapesecurity.salvation.directiveValues.HashSource;
-import com.shapesecurity.salvation.directiveValues.HostSource;
-import com.shapesecurity.salvation.directiveValues.KeywordSource;
-import com.shapesecurity.salvation.directiveValues.SourceExpression;
+import com.shapesecurity.salvation.directiveValues.*;
 import com.shapesecurity.salvation.interfaces.MatchesHash;
 import com.shapesecurity.salvation.interfaces.MatchesNonce;
 import com.shapesecurity.salvation.interfaces.MatchesSource;
@@ -39,6 +36,14 @@ public abstract class SourceListDirective extends Directive<SourceExpression>
             .anyMatch(x -> ((MatchesNonce) x).matchesNonce(nonce));
     }
 
+    public boolean containsHashSource() {
+        return this.values().anyMatch(x -> (x instanceof HashSource));
+    }
+
+    public boolean containsNonceSource() {
+        return this.values().anyMatch(x -> (x instanceof NonceSource));
+    }
+
     @Nonnull public Directive<SourceExpression> resolveSelf(@Nonnull Origin origin) {
         return this.bind(dv -> {
             if (dv == KeywordSource.Self) {


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation/LocationTest.java b/src/test/java/com/shapesecurity/salvation/LocationTest.java
index f4c88c4..18c984c 100644
--- a/src/test/java/com/shapesecurity/salvation/LocationTest.java
+++ b/src/test/java/com/shapesecurity/salvation/LocationTest.java
@@ -410,6 +410,27 @@ public class LocationTest extends CSPTest {
             notices.get(1).toString());
     }
 
+    @Test public void testUnsafeInlineWarnings() {
+        ArrayList<Notice> notices = new ArrayList<>();
+        ParserWithLocation
+            .parse("script-src 'unsafe-inline' 'nonce-123'", URI.parse("https://origin"), notices);
+        assertEquals(2, notices.size());
+        Notice notice = notices.get(0);
+        assertEquals(
+            "1:28: Invalid base64-value (should be multiple of 4 bytes: 3). Consider using RFC4648 compliant base64 encoding implementation.",
+            notice.show());
+        assertEquals(
+            "Warning: Invalid base64-value (should be multiple of 4 bytes: 3). Consider using RFC4648 compliant base64 encoding implementation.",
+            notice.toString());
+        notice = notices.get(1);
+        assertEquals(
+            "1:28: The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source.",
+            notice.show());
+        assertEquals(
+            "Warning: The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source.",
+            notice.toString());
+    }
+
     @Test public void testNoticeHelpers() {
         ArrayList<Notice> notices = new ArrayList<>();
         ParserWithLocation.parse(
diff --git a/src/test/java/com/shapesecurity/salvation/ParserTest.java b/src/test/java/com/shapesecurity/salvation/ParserTest.java
index 0c922a9..11719ec 100644
--- a/src/test/java/com/shapesecurity/salvation/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation/ParserTest.java
@@ -196,7 +196,10 @@ public class ParserTest extends CSPTest {
         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:"));
-
+        assertEquals("optimisation", "script-src 'unsafe-inline' *", parseAndShow("script-src example.com * 'unsafe-inline'"));
+        assertEquals("optimisation", "script-src 'nonce-123' *", parseAndShow("script-src example.com * 'unsafe-inline' 'nonce-123'"));
+        assertEquals("optimisation", "script-src 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols=' *", parseAndShow("script-src example.com * 'unsafe-inline' 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols='"));
+        assertEquals("optimisation", "script-src 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols=' 'nonce-123' *", parseAndShow("script-src example.com * 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols=' 'nonce-123' 'unsafe-inline'"));
     }
 
     @Test public void testAncestorSource() {
@@ -818,7 +821,37 @@ public class ParserTest extends CSPTest {
         assertEquals(0, p.getDirectives().size());
         assertEquals(1, notices.size());
         assertEquals("Expecting end of policy but found \",\".", notices.get(0).message);
+    }
 
+    @Test public void testUnsafeInlineWithHashNonce() {
+        Policy p;
+        ArrayList<Notice> notices = new ArrayList<>();
+        p = parseWithNotices("default-src 'unsafe-inline' 'nonce-abcdef'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source.", notices.get(1).message);
 
+        notices.clear();
+        p = parseWithNotices("default-src 'Unsafe-INLINE' 'nonce-abcd'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source.", notices.get(1).message);
+
+        notices.clear();
+        p = parseWithNotices("default-src 'unsafe-inline' 'sha512-vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg=='", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(1, notices.size());
+        assertEquals("The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source.", notices.get(0).message);
+
+        notices.clear();
+        p = parseWithNotices("default-src 'unsafe-inline' 'unsafe-redirect'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(1, notices.size());
+        assertEquals("'unsafe-redirect' has been removed from CSP as of version 2.0.", notices.get(0).message);
+
+        notices.clear();
+        p = parseWithNotices("default-src 'unsafe-inline'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(0, notices.size());
     }
 }
diff --git a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
index f705474..697d731 100644
--- a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
+++ b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
@@ -136,6 +136,8 @@ public class PolicyQueryingTest extends CSPTest {
         assertTrue("style hash is allowed",
             p.allowsStyleWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value("cGl6ZGE=")));
 
+        p = Parser.parse("default-src * 'unsafe-inline' 'nonce-123'", "https://abc.com");
+        assertFalse("inline script is not allowed", p.allowsUnsafeInlineScript());
     }
 
     @Test public void testAllowsPlugin() {
@@ -172,6 +174,14 @@ public class PolicyQueryingTest extends CSPTest {
                 "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
         assertFalse("style hash is not allowed", p.allowsStyleWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value(
             "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
+
+        p = Parser.parse("default-src * 'unsafe-inline' 'sha512-vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg=='", "https://abc.com");
+        assertTrue("script hash is allowed", p.allowsScriptWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value(
+            "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
+        assertFalse("unknown script is not allowed", p.allowsScriptWithHash(HashSource.HashAlgorithm.SHA256, new Base64Value(
+            "7HY1KLziIDGNSsu67SifYO1B69r1EFEfvPg3McqyIcM=")));
+        assertFalse("unknown script is not allowed", p.allowsUnsafeInlineScript());
+        assertFalse("unknown style is not allowed", p.allowsUnsafeInlineStyle());
     }
 
     @Test public void testAllowsNonce() {
@@ -188,6 +198,12 @@ public class PolicyQueryingTest extends CSPTest {
         p = Parser.parse("default-src 'none'", "https://abc.com");
         assertFalse("script nonce is not allowed", p.allowsScriptWithNonce(new Base64Value("0gQAAA==")));
         assertFalse("style nonce is not allowed", p.allowsStyleWithNonce(new Base64Value("0gQAAA==")));
+
+        p = Parser.parse("default-src * 'unsafe-inline' 'nonce-0gQAAA=='", "https://abc.com");
+        assertTrue("script nonce is allowed", p.allowsScriptWithNonce(new Base64Value("0gQAAA==")));
+        assertFalse("script wrong nonce is not allowed", p.allowsScriptWithNonce(new Base64Value("1234")));
+        assertFalse("unsafe inline script is not allowed", p.allowsUnsafeInlineScript());
+        assertFalse("unsafe inline style is not allowed", p.allowsUnsafeInlineStyle());
     }
 
     @Test public void testAllowsConnect() {