Bug 1594234 manifest v3 content security validation improvements r=ro…

…bwu,geckoview-reviewers,agi This patch adds CSP validation for manifest v3 changes when parsing the addon manifest. Differential Revision: https://phabricator.services.mozilla.com/D100720
dothq · Jan 19, 2021 · 98c9307 · 98c9307
1 parent 4a14410
commit 98c9307
Show file tree

Hide file tree

Showing 11 changed files with 423 additions and 55 deletions.
diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
@@ -68,11 +68,6 @@ pref("extensions.startupScanScopes", 0);
 pref("extensions.geckoProfiler.acceptedExtensionIds", "[email protected],[email protected],[email protected]");
 
 
-// Add-on content security policies.
-pref("extensions.webextensions.base-content-security-policy", "script-src 'self' https://* moz-extension: blob: filesystem: 'unsafe-eval' 'unsafe-inline'; object-src 'self' https://* moz-extension: blob: filesystem:;");
-pref("extensions.webextensions.base-content-security-policy.v3", "script-src 'self'; object-src 'self'; style-src 'self'; worker-src 'self';");
-pref("extensions.webextensions.default-content-security-policy", "script-src 'self'; object-src 'self';");
-
 pref("extensions.webextensions.remote", true);
 pref("extensions.webextensions.background-delayed-startup", true);
 

diff --git a/caps/nsIAddonPolicyService.idl b/caps/nsIAddonPolicyService.idl
@@ -75,10 +75,21 @@ interface nsIAddonPolicyService : nsISupports
 [scriptable, uuid(7a4fe60b-9131-45f5-83f3-dc63b5d71a5d)]
 interface nsIAddonContentPolicy : nsISupports
 {
+  /* options to pass to validateAddonCSP
+   *
+   * Manifest V2 uses CSP_ALLOW_ANY.
+   * In Manifest V3, extension_pages would use CSP_ALLOW_LOCALHOST and
+   * sandbox would use CSP_ALLOW_EVAL.
+   */
+  const unsigned long CSP_ALLOW_ANY = 0xFFFF;
+  const unsigned long CSP_ALLOW_LOCALHOST = (1<<0);
+  const unsigned long CSP_ALLOW_EVAL = (1<<1);
+  const unsigned long CSP_ALLOW_REMOTE = (1<<2);
+
   /**
    * Checks a custom content security policy string, to ensure that it meets
    * minimum security requirements. Returns null for valid policies, or a
    * string describing the error for invalid policies.
    */
-  AString validateAddonCSP(in AString aPolicyString);
+  AString validateAddonCSP(in AString aPolicyString, in unsigned long aPermittedPolicy);
 };
diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js
@@ -196,11 +196,6 @@ pref("extensions.installDistroAddons", false);
 pref("extensions.webextPermissionPrompts", true);
 pref("extensions.webextOptionalPermissionPrompts", true);
 
-// Add-on content security policies.
-pref("extensions.webextensions.base-content-security-policy", "script-src 'self' https://* moz-extension: blob: filesystem: 'unsafe-eval' 'unsafe-inline'; object-src 'self' https://* moz-extension: blob: filesystem:;");
-pref("extensions.webextensions.base-content-security-policy.v3", "script-src 'self'; object-src 'self'; style-src 'self'; worker-src 'self';");
-pref("extensions.webextensions.default-content-security-policy", "script-src 'self'; object-src 'self';");
-
 pref("extensions.webextensions.background-delayed-startup", true);
 
 pref("extensions.experiments.enabled", false);

diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
@@ -3907,6 +3907,12 @@ pref("extensions.webcompat-reporter.newIssueEndpoint", "https://webcompat.com/is
   pref("extensions.webcompat-reporter.enabled", false);
 #endif
 
+// Add-on content security policies.
+pref("extensions.webextensions.base-content-security-policy", "script-src 'self' https://* http://localhost:* http://127.0.0.1:* moz-extension: blob: filesystem: 'unsafe-eval' 'unsafe-inline'; object-src 'self' moz-extension: blob: filesystem:;");
+pref("extensions.webextensions.base-content-security-policy.v3", "script-src 'self' http://localhost:* http://127.0.0.1:*; object-src 'self';");
+pref("extensions.webextensions.default-content-security-policy", "script-src 'self'; object-src 'self';");
+
+
 pref("network.buffer.cache.count", 24);
 pref("network.buffer.cache.size",  32768);
 

diff --git a/toolkit/components/extensions/Extension.jsm b/toolkit/components/extensions/Extension.jsm
@@ -883,6 +883,7 @@ class ExtensionData {
         this.manifestWarning(error);
       },
       preprocessors: {},
+      manifestVersion: this.manifest.manifest_version,
     };
 
     if (this.fluentL10n || this.localeData) {

diff --git a/toolkit/components/extensions/Schemas.jsm b/toolkit/components/extensions/Schemas.jsm
@@ -388,7 +388,7 @@ class Context {
       }
     }
 
-    let props = ["preprocessors", "isChromeCompat"];
+    let props = ["preprocessors", "isChromeCompat", "manifestVersion"];
     for (let prop of props) {
       if (prop in params) {
         if (prop in this && typeof this[prop] == "object") {
@@ -1099,7 +1099,14 @@ const FORMATS = {
   },
 
   contentSecurityPolicy(string, context) {
-    let error = contentPolicyService.validateAddonCSP(string);
+    // Manifest V3 extension_pages allows localhost.  When sandbox is
+    // implemented, or any other V3 or later directive, the flags
+    // logic will need to be updated.
+    let flags =
+      context.manifestVersion < 3
+        ? Ci.nsIAddonContentPolicy.CSP_ALLOW_ANY
+        : Ci.nsIAddonContentPolicy.CSP_ALLOW_LOCALHOST;
+    let error = contentPolicyService.validateAddonCSP(string, flags);
     if (error != null) {
       // The CSP validation error is not reported as part of the "choices" error message,
       // we log the CSP validation error explicitly here to make it easier for the addon developers

diff --git a/toolkit/components/extensions/test/xpcshell/test_csp_custom_policies.js b/toolkit/components/extensions/test/xpcshell/test_csp_custom_policies.js
@@ -140,6 +140,26 @@ add_task(async function test_extension_csp() {
       },
       expectedPolicy: aps.defaultCSP,
     },
+    {
+      name: "manifest_v2 allows https protocol",
+      manifest: {
+        manifest_version: 3,
+        content_security_policy: {
+          extension_pages: `script-src 'self' https://*; object-src 'self'`,
+        },
+      },
+      expectedPolicy: aps.defaultCSP,
+    },
+    {
+      name: "manifest_v2 allows unsafe-eval",
+      manifest: {
+        manifest_version: 3,
+        content_security_policy: {
+          extension_pages: `script-src 'self' 'unsafe-eval'; object-src 'self'`,
+        },
+      },
+      expectedPolicy: aps.defaultCSP,
+    },
     {
       name: "manifest_v3 invalid csp results in default csp used",
       manifest: {
@@ -150,6 +170,46 @@ add_task(async function test_extension_csp() {
       },
       expectedPolicy: aps.defaultCSP,
     },
+    {
+      name: "manifest_v3 forbidden protocol results in default csp used",
+      manifest: {
+        manifest_version: 3,
+        content_security_policy: {
+          extension_pages: `script-src 'self' https://*; object-src 'self'`,
+        },
+      },
+      expectedPolicy: aps.defaultCSP,
+    },
+    {
+      name: "manifest_v3 forbidden eval results in default csp used",
+      manifest: {
+        manifest_version: 3,
+        content_security_policy: {
+          extension_pages: `script-src 'self' 'unsafe-eval'; object-src 'self'`,
+        },
+      },
+      expectedPolicy: aps.defaultCSP,
+    },
+    {
+      name: "manifest_v3 allows localhost",
+      manifest: {
+        manifest_version: 3,
+        content_security_policy: {
+          extension_pages: `script-src 'self' https://localhost; object-src 'self'`,
+        },
+      },
+      expectedPolicy: `script-src 'self' https://localhost; object-src 'self'`,
+    },
+    {
+      name: "manifest_v3 allows 127.0.0.1",
+      manifest: {
+        manifest_version: 3,
+        content_security_policy: {
+          extension_pages: `script-src 'self' https://127.0.0.1; object-src 'self'`,
+        },
+      },
+      expectedPolicy: `script-src 'self' https://127.0.0.1; object-src 'self'`,
+    },
     {
       name: "manifest_v2 csp",
       manifest: {

diff --git a/toolkit/components/extensions/test/xpcshell/test_csp_validator.js b/toolkit/components/extensions/test/xpcshell/test_csp_validator.js
@@ -6,11 +6,64 @@ const cps = Cc["@mozilla.org/addons/content-policy;1"].getService(
   Ci.nsIAddonContentPolicy
 );
 
+add_task(async function test_csp_validator_flags() {
+  let checkPolicy = (policy, flags, expectedResult, message = null) => {
+    info(`Checking policy: ${policy}`);
+
+    let result = cps.validateAddonCSP(policy, flags);
+    equal(result, expectedResult);
+  };
+
+  let flags = Ci.nsIAddonContentPolicy;
+
+  checkPolicy(
+    "default-src 'self'; script-src 'self' http://localhost",
+    0,
+    "\u2018script-src\u2019 directive contains a forbidden http: protocol source",
+    "localhost disallowed"
+  );
+  checkPolicy(
+    "default-src 'self'; script-src 'self' http://localhost",
+    flags.CSP_ALLOW_LOCALHOST,
+    null,
+    "localhost allowed"
+  );
+
+  checkPolicy(
+    "default-src 'self'; script-src 'self' 'unsafe-eval'",
+    0,
+    "\u2018script-src\u2019 directive contains a forbidden 'unsafe-eval' keyword",
+    "eval disallowed"
+  );
+  checkPolicy(
+    "default-src 'self'; script-src 'self' 'unsafe-eval'",
+    flags.CSP_ALLOW_EVAL,
+    null,
+    "eval allowed"
+  );
+
+  checkPolicy(
+    "default-src 'self'; script-src 'self' https://example.com",
+    0,
+    "\u2018script-src\u2019 directive contains a forbidden https: protocol source",
+    "remote disallowed"
+  );
+  checkPolicy(
+    "default-src 'self'; script-src 'self' https://example.com",
+    flags.CSP_ALLOW_REMOTE,
+    null,
+    "remote allowed"
+  );
+});
+
 add_task(async function test_csp_validator() {
   let checkPolicy = (policy, expectedResult, message = null) => {
     info(`Checking policy: ${policy}`);
 
-    let result = cps.validateAddonCSP(policy);
+    let result = cps.validateAddonCSP(
+      policy,
+      Ci.nsIAddonContentPolicy.CSP_ALLOW_ANY
+    );
     equal(result, expectedResult);
   };
 
@@ -82,6 +135,16 @@ add_task(async function test_csp_validator() {
     "\u2018script-src\u2019 directive contains a forbidden 'unsafe-inline' keyword"
   );
 
+  // Localhost is always valid
+  for (let src of [
+    "http://localhost",
+    "https://localhost",
+    "http://127.0.0.1",
+    "https://127.0.0.1",
+  ]) {
+    checkPolicy(`script-src 'self' ${src}; object-src 'none';`, null);
+  }
+
   let directives = ["script-src", "object-src"];
 
   for (let [directive, other] of [directives, directives.slice().reverse()]) {
@@ -92,17 +155,135 @@ add_task(async function test_csp_validator() {
       );
     }
 
+    for (let protocol of ["http", "https"]) {
+      checkPolicy(
+        `${directive} 'self' ${protocol}:; ${other} 'self';`,
+        `${protocol}: protocol requires a host in \u2018${directive}\u2019 directives`
+      );
+    }
+
+    checkPolicy(
+      `${directive} 'self' http://example.com; ${other} 'self';`,
+      `\u2018${directive}\u2019 directive contains a forbidden http: protocol source`
+    );
+
+    for (let protocol of ["ftp", "meh"]) {
+      checkPolicy(
+        `${directive} 'self' ${protocol}:; ${other} 'self';`,
+        `\u2018${directive}\u2019 directive contains a forbidden ${protocol}: protocol source`
+      );
+    }
+
+    checkPolicy(
+      `${directive} 'self' 'nonce-01234'; ${other} 'self';`,
+      `\u2018${directive}\u2019 directive contains a forbidden 'nonce-*' keyword`
+    );
+  }
+});
+
+add_task(async function test_csp_validator_extension_pages() {
+  let checkPolicy = (policy, expectedResult, message = null) => {
+    info(`Checking policy: ${policy}`);
+
+    let result = cps.validateAddonCSP(
+      policy,
+      Ci.nsIAddonContentPolicy.CSP_ALLOW_LOCALHOST
+    );
+    equal(result, expectedResult);
+  };
+
+  checkPolicy("script-src 'self'; object-src 'self';", null);
+  checkPolicy("script-src 'self'; object-src 'self'; worker-src 'none'", null);
+  checkPolicy("script-src 'self'; object-src 'none'; worker-src 'self'", null);
+
+  let hash =
+    "'sha256-NjZhMDQ1YjQ1MjEwMmM1OWQ4NDBlYzA5N2Q1OWQ5NDY3ZTEzYTNmMzRmNjQ5NGU1MzlmZmQzMmMxYmIzNWYxOCAgLQo='";
+
+  checkPolicy(
+    `script-src 'self' moz-extension://09abcdef blob: filesystem: ${hash}; ` +
+      `object-src 'self' moz-extension://09abcdef blob: filesystem: ${hash}`,
+    null
+  );
+
+  for (let policy of ["", "object-src 'none';", "worker-src 'none';"]) {
+    checkPolicy(
+      policy,
+      "Policy is missing a required \u2018script-src\u2019 directive"
+    );
+  }
+
+  checkPolicy(
+    "default-src 'self'",
+    null,
+    "A valid default-src should count as a valid script-src or object-src"
+  );
+
+  for (let directive of ["script-src", "object-src", "worker-src"]) {
+    checkPolicy(
+      `default-src 'self'; ${directive} 'self'`,
+      null,
+      `A valid default-src should count as a valid ${directive}`
+    );
+    checkPolicy(
+      `default-src 'self'; ${directive} http://example.com`,
+      `\u2018${directive}\u2019 directive contains a forbidden http: protocol source`,
+      `A valid default-src should not allow an invalid ${directive} directive`
+    );
+  }
+
+  checkPolicy(
+    "script-src 'self';",
+    "Policy is missing a required \u2018object-src\u2019 directive"
+  );
+
+  checkPolicy(
+    "script-src 'none'; object-src 'none'",
+    "\u2018script-src\u2019 must include the source 'self'"
+  );
+
+  checkPolicy("script-src 'self'; object-src 'none';", null);
+
+  checkPolicy(
+    "script-src 'self' 'unsafe-inline'; object-src 'self';",
+    "\u2018script-src\u2019 directive contains a forbidden 'unsafe-inline' keyword"
+  );
+
+  checkPolicy(
+    "script-src 'self' 'unsafe-eval'; object-src 'self';",
+    "\u2018script-src\u2019 directive contains a forbidden 'unsafe-eval' keyword"
+  );
+
+  // Localhost is always valid
+  for (let src of [
+    "http://localhost",
+    "https://localhost",
+    "http://127.0.0.1",
+    "https://127.0.0.1",
+  ]) {
+    checkPolicy(`script-src 'self' ${src}; object-src 'none';`, null);
+  }
+
+  let directives = ["script-src", "object-src"];
+
+  for (let [directive, other] of [directives, directives.slice().reverse()]) {
+    for (let protocol of ["http", "https"]) {
+      checkPolicy(
+        `${directive} 'self' ${protocol}:; ${other} 'self';`,
+        `${protocol}: protocol requires a host in \u2018${directive}\u2019 directives`
+      );
+    }
+
     checkPolicy(
-      `${directive} 'self' https:; ${other} 'self';`,
-      `https: protocol requires a host in \u2018${directive}\u2019 directives`
+      `${directive} 'self' https://example.com; ${other} 'self';`,
+      `\u2018${directive}\u2019 directive contains a forbidden https: protocol source`
     );
 
     checkPolicy(
       `${directive} 'self' http://example.com; ${other} 'self';`,
       `\u2018${directive}\u2019 directive contains a forbidden http: protocol source`
     );
 
-    for (let protocol of ["http", "ftp", "meh"]) {
+    for (let protocol of ["ftp", "meh"]) {
       checkPolicy(
         `${directive} 'self' ${protocol}:; ${other} 'self';`,
         `\u2018${directive}\u2019 directive contains a forbidden ${protocol}: protocol source`