policy: add policy-integrity to mitigate policy tampering

bmeck · targos · commit 2eeb44f3facb · 2019-07-22T21:20:42.000+02:00
PR-URL: #28734 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -457,6 +457,17 @@ unless either the `--pending-deprecation` command line flag, or the
 are used to provide a kind of selective "early warning" mechanism that
 developers may leverage to detect deprecated API usage.
 
+### `--policy-integrity=sri`
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+Instructs Node.js to error prior to running any code if the policy does not have
+the specified integrity. It expects a [Subresource Integrity][] string as a
+parameter.
+
 ### `--preserve-symlinks`
 <!-- YAML
 added: v6.3.0
@@ -992,6 +1003,7 @@ Node.js options that are allowed are:
 - `--no-warnings`
 - `--openssl-config`
 - `--pending-deprecation`
+- `--policy-integrity`
 - `--preserve-symlinks-main`
 - `--preserve-symlinks`
 - `--prof-process`
@@ -1196,6 +1208,7 @@ greater than `4` (its current default value). For more information, see the
 [Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
 [REPL]: repl.html
 [ScriptCoverage]: https://chromedevtools.github.io/devtools-protocol/tot/Profiler#type-ScriptCoverage
+[Subresource Integrity]: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity
 [V8 JavaScript code coverage]: https://v8project.blogspot.com/2017/12/javascript-code-coverage.html
 [customizing esm specifier resolution]: esm.html#esm_customizing_esm_specifier_resolution_algorithm
 [debugger]: debugger.html
diff --git a/doc/api/policy.md b/doc/api/policy.md
@@ -38,6 +38,15 @@ node --experimental-policy=policy.json app.js
 The policy manifest will be used to enforce constraints on code loaded by
 Node.js.
 
+In order to mitigate tampering with policy files on disk, an integrity for
+the policy file itself may be provided via `--policy-integrity`.
+This allows running `node` and asserting the policy file contents
+even if the file is changed on disk.
+
+```sh
+node --experimental-policy=policy.json --policy-integrity="sha384-SggXRQHwCG8g+DktYYzxkXRIkTiEYWBHqev0xnpCxYlqMBufKZHAHQM3/boDaI/0" app.js
+```
+
 ## Features
 
 ### Error Behavior
diff --git a/doc/node.1 b/doc/node.1
@@ -231,6 +231,9 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is
 .It Fl -pending-deprecation
 Emit pending deprecation warnings.
 .
+.It Fl -policy-integrity Ns = Ns Ar sri
+Instructs Node.js to error prior to running any code if the policy does not have the specified integrity. It expects a Subresource Integrity string as a parameter.
+.
 .It Fl -preserve-symlinks
 Instructs the module loader to preserve symbolic links when resolving and caching modules other than the main module.
 .
diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
@@ -4,6 +4,7 @@ const { Object, SafeWeakMap } = primordials;
 
 const { getOptionValue } = require('internal/options');
 const { Buffer } = require('buffer');
+const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes;
 
 function prepareMainThreadExecution(expandArgv1 = false) {
   // Patch the process object with legacy properties and normalizations
@@ -332,6 +333,32 @@ function initializePolicy() {
     }
     const fs = require('fs');
     const src = fs.readFileSync(manifestURL, 'utf8');
+    const experimentalPolicyIntegrity = getOptionValue('--policy-integrity');
+    if (experimentalPolicyIntegrity) {
+      const SRI = require('internal/policy/sri');
+      const { createHash, timingSafeEqual } = require('crypto');
+      const realIntegrities = new Map();
+      const integrityEntries = SRI.parse(experimentalPolicyIntegrity);
+      let foundMatch = false;
+      for (var i = 0; i < integrityEntries.length; i++) {
+        const {
+          algorithm,
+          value: expected
+        } = integrityEntries[i];
+        const hash = createHash(algorithm);
+        hash.update(src);
+        const digest = hash.digest();
+        if (digest.length === expected.length &&
+          timingSafeEqual(digest, expected)) {
+          foundMatch = true;
+          break;
+        }
+        realIntegrities.set(algorithm, digest.toString('base64'));
+      }
+      if (!foundMatch) {
+        throw new ERR_MANIFEST_ASSERT_INTEGRITY(manifestURL, realIntegrities);
+      }
+    }
     require('internal/process/policy')
       .setup(src, manifestURL.href);
   }
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -116,6 +116,13 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
   if (!userland_loader.empty() && !experimental_modules) {
     errors->push_back("--loader requires --experimental-modules be enabled");
   }
+  if (has_policy_integrity_string && experimental_policy.empty()) {
+    errors->push_back("--policy-integrity requires "
+                      "--experimental-policy be enabled");
+  }
+  if (has_policy_integrity_string && experimental_policy_integrity.empty()) {
+    errors->push_back("--policy-integrity cannot be empty");
+  }
 
   if (!module_type.empty()) {
     if (!experimental_modules) {
@@ -321,6 +328,15 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "security policy",
             &EnvironmentOptions::experimental_policy,
             kAllowedInEnvironment);
+  AddOption("[has_policy_integrity_string]",
+            "",
+            &EnvironmentOptions::has_policy_integrity_string);
+  AddOption("--policy-integrity",
+            "ensure the security policy contents match "
+            "the specified integrity",
+            &EnvironmentOptions::experimental_policy_integrity,
+            kAllowedInEnvironment);
+  Implies("--policy-integrity", "[has_policy_integrity_string]");
   AddOption("--experimental-repl-await",
             "experimental await keyword support in REPL",
             &EnvironmentOptions::experimental_repl_await,
diff --git a/src/node_options.h b/src/node_options.h
@@ -106,6 +106,8 @@ class EnvironmentOptions : public Options {
   bool experimental_wasm_modules = false;
   std::string module_type;
   std::string experimental_policy;
+  std::string experimental_policy_integrity;
+  bool has_policy_integrity_string;
   bool experimental_repl_await = false;
   bool experimental_vm_modules = false;
   bool expose_internals = false;
diff --git a/test/fixtures/policy/dep-policy.json b/test/fixtures/policy/dep-policy.json
@@ -0,0 +1,7 @@
+{
+  "resources": {
+    "./dep.js": {
+      "integrity": "sha512-7CMcc2oytFfMnGQaXbJk84gYWF2J7p/fmWPW7dsnJyniD+vgxtK9VAZ/22UxFOA4q5d27RoGLxSqNZ/nGCJkMw== sha512-scgN9Td0bGMlGH2lUHvEeHtz92Hx6AO+sYhU3WRI6bn3jEUCXbXJs68nOOsGzRWR7a2tbqGoETnOCpHHf1Njhw=="
+    }
+  }
+}
diff --git a/test/fixtures/policy/dep.js b/test/fixtures/policy/dep.js
@@ -0,0 +1,2 @@
+'use strict';
+module.exports = 'The Secret Ingredient';
diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js
@@ -0,0 +1,69 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const fixtures = require('../common/fixtures');
+
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const fs = require('fs');
+const crypto = require('crypto');
+
+const depPolicy = fixtures.path('policy', 'dep-policy.json');
+const dep = fixtures.path('policy', 'dep.js');
+
+const emptyHash = crypto.createHash('sha512');
+emptyHash.update('');
+const emptySRI = `sha512-${emptyHash.digest('base64')}`;
+const policyHash = crypto.createHash('sha512');
+policyHash.update(fs.readFileSync(depPolicy));
+
+/* eslint-disable max-len */
+// When using \n only
+const nixPolicySRI = 'sha512-u/nXI6UacK5fKDC2bopcgnuQY4JXJKlK3dESO3GIKKxwogVHjJqpF9rgk7Zw+TJXIc96xBUWKHuUgOzic8/4tQ==';
+// When \n is turned into \r\n
+const windowsPolicySRI = 'sha512-OeyCPRo4OZMosHyquZXDHpuU1F4KzG9UHFnn12FMaHsvqFUt3TFZ+7wmZE7ThZ5rsQWkUjc9ZH0knGZ2e8BYPQ==';
+/* eslint-enable max-len */
+
+const depPolicySRI = `${nixPolicySRI} ${windowsPolicySRI}`;
+console.dir({
+  depPolicySRI,
+  body: JSON.stringify(fs.readFileSync(depPolicy).toString('utf8'))
+});
+{
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--policy-integrity', emptySRI,
+      '--experimental-policy', depPolicy, dep,
+    ]
+  );
+
+  assert.ok(stderr.includes('ERR_MANIFEST_ASSERT_INTEGRITY'));
+  assert.strictEqual(status, 1);
+}
+{
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--policy-integrity', '',
+      '--experimental-policy', depPolicy, dep,
+    ]
+  );
+
+  assert.ok(stderr.includes('--policy-integrity'));
+  assert.strictEqual(status, 9);
+}
+{
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--policy-integrity', depPolicySRI,
+      '--experimental-policy', depPolicy, dep,
+    ]
+  );
+
+  assert.strictEqual(status, 0, `status: ${status}\nstderr: ${stderr}`);
+}

Original file line number	Diff line number	Diff line change
`@@ -231,6 +231,9 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is`
`231`	`231`	`.It Fl -pending-deprecation`
`232`	`232`	`Emit pending deprecation warnings.`
`233`	`233`	`.`
	`234`	`+.It Fl -policy-integrity Ns = Ns Ar sri`
	`235`	`+Instructs Node.js to error prior to running any code if the policy does not have the specified integrity. It expects a Subresource Integrity string as a parameter.`
	`236`	`+.`
`234`	`237`	`.It Fl -preserve-symlinks`
`235`	`238`	`Instructs the module loader to preserve symbolic links when resolving and caching modules other than the main module.`
`236`	`239`	`.`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+'use strict';`
	`2`	`+module.exports = 'The Secret Ingredient';`