From 4e147a31caf701a57efc686b3793292400f0206f Mon Sep 17 00:00:00 2001
From: Andreas Wagner <mail@andreaswagner.org>
Date: Wed, 22 Nov 2017 15:34:22 +0100
Subject: [PATCH] More precise CSP evaluation

---
 src/parsers/manifestjson.js        | 31 +++++++++++++++++++++++++++---
 tests/parsers/test.manifestjson.js |  7 +++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/parsers/manifestjson.js b/src/parsers/manifestjson.js
index 7c71774c..306a4825 100644
--- a/src/parsers/manifestjson.js
+++ b/src/parsers/manifestjson.js
@@ -285,14 +285,24 @@ export default class ManifestJSONParser extends JSONParser {
     // Not sure about FTP here but CSP spec treats ws/wss as
     // equivalent to http/https.
     const validProtocols = ['ftp:', 'http:', 'https:', 'ws:', 'wss:'];
-    const candidates = ['script-src', 'default-src', 'worker-src'];
+    // The order is important here, 'default-src' needs to be
+    // before 'script-src'
+    const candidates = ['default-src', 'script-src', 'worker-src'];
 
+    let insecureDefaultSrc = false;
+    let secureScriptSrc = false;
     for (let i = 0; i < candidates.length; i++) {
       /* eslint-disable no-continue */
       const candidate = candidates[i];
       if (Object.prototype.hasOwnProperty.call(directives, candidate)) {
         const values = directives[candidate];
 
+        // If the 'default-src' is insecure, check whether the 'script-src'
+        // makes it secure, ie 'script-src: self;'
+        if (insecureDefaultSrc && values.length === 1 && values[0] === '\'self\'') {
+          secureScriptSrc = true;
+        }
+
         for (let j = 0; j < values.length; j++) {
           let value = values[j].trim();
 
@@ -306,7 +316,13 @@ export default class ManifestJSONParser extends JSONParser {
             (validProtocols.some((x) => value.startsWith(x))));
 
           if (hasProtocol) {
-            this.collector.addWarning(messages.MANIFEST_CSP);
+            if (candidate === 'default-src') {
+              // Remember insecure 'default-src' to check whether a later
+              // 'script-src' makes it secure
+              insecureDefaultSrc = true;
+            } else {
+              this.collector.addWarning(messages.MANIFEST_CSP);
+            }
             continue;
           }
 
@@ -323,12 +339,21 @@ export default class ManifestJSONParser extends JSONParser {
           if (value === '*' || value.search(CSP_KEYWORD_RE) === -1) {
             // everything else looks like something we don't understand
             // / support otherwise is invalid so let's warn about that.
-            this.collector.addWarning(messages.MANIFEST_CSP);
+            if (candidate === 'default-src') {
+              // Remember insecure 'default-src' to check whether a later
+              // 'script-src' makes it secure
+              insecureDefaultSrc = true;
+            } else {
+              this.collector.addWarning(messages.MANIFEST_CSP);
+            }
             continue;
           }
         }
       }
     }
+    if (insecureDefaultSrc && !secureScriptSrc) {
+      this.collector.addWarning(messages.MANIFEST_CSP);
+    }
   }
 
   getAddonId() {
diff --git a/tests/parsers/test.manifestjson.js b/tests/parsers/test.manifestjson.js
index e3d09ce9..d4b5e675 100644
--- a/tests/parsers/test.manifestjson.js
+++ b/tests/parsers/test.manifestjson.js
@@ -431,6 +431,13 @@ describe('ManifestJSONParser', () => {
         // unsafe-inline is not supported by Firefox and won't be for the
         // forseeable future. See http://bit.ly/2wG6LP0 for more details-
         "script-src 'self' 'unsafe-inline';",
+
+        // 'default-src' is insecure, but the limiting 'script-src' prevents
+        // remote script injection
+        "default-src *; script-src 'self'",
+        "default-src https:; script-src 'self'",
+        "default-src example.com; script-src 'self'",
+        "default-src http://remote.com/; script-src 'self'",
       ];
 
       validValues.forEach((validValue) => {