From c74132c0222b570ce6b80ab8cd49f9ab27be9a17 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 5 Jun 2019 13:33:07 -0500 Subject: [PATCH 01/19] policy: add policy-integrity to mitigate policy tampering --- doc/api/policy.md | 9 ++++ lib/internal/bootstrap/pre_execution.js | 27 ++++++++++ src/node_options.cc | 16 ++++++ src/node_options.h | 2 + test/fixtures/policy/dep-policy.json | 7 +++ test/fixtures/policy/dep.js | 2 + test/parallel/test-policy-integrity-flag.js | 57 +++++++++++++++++++++ 7 files changed, 120 insertions(+) create mode 100644 test/fixtures/policy/dep-policy.json create mode 100644 test/fixtures/policy/dep.js create mode 100644 test/parallel/test-policy-integrity-flag.js diff --git a/doc/api/policy.md b/doc/api/policy.md index be7ea3480b79fa..a1955f2b3ee835 100644 --- 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/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index e9571a89a62857..1948c4e29e6a30 100644 --- 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 index 2b177806683a63..acb8c8cf142682 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -121,6 +121,13 @@ void EnvironmentOptions::CheckOptions(std::vector* 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) { @@ -313,6 +320,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 index 63892d3c477767..08ce432e9a26b3 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -105,6 +105,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 new file mode 100644 index 00000000000000..b148674a0a65ee --- /dev/null +++ b/test/fixtures/policy/dep-policy.json @@ -0,0 +1,7 @@ +{ + "resources": { + "./dep.js": { + "integrity": "sha512-7CMcc2oytFfMnGQaXbJk84gYWF2J7p/fmWPW7dsnJyniD+vgxtK9VAZ/22UxFOA4q5d27RoGLxSqNZ/nGCJkMw==" + } + } +} diff --git a/test/fixtures/policy/dep.js b/test/fixtures/policy/dep.js new file mode 100644 index 00000000000000..1c61a090d275a8 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..16be492acfd045 --- /dev/null +++ b/test/parallel/test-policy-integrity-flag.js @@ -0,0 +1,57 @@ +'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)); +const depPolicySRI = `sha512-${policyHash.digest('base64')}`; +{ + 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 } = spawnSync( + process.execPath, + [ + '--policy-integrity', depPolicySRI, + '--experimental-policy', depPolicy, dep, + ] + ); + + assert.strictEqual(status, 0); +} From 56256e054b4989804a4e8958e0265fd97e81f7dd Mon Sep 17 00:00:00 2001 From: Bradley Meck Date: Wed, 17 Jul 2019 12:18:00 -0500 Subject: [PATCH 02/19] Update test/parallel/test-policy-integrity-flag.js Co-Authored-By: Vse Mozhet Byt --- test/parallel/test-policy-integrity-flag.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index 16be492acfd045..f64e0be5d58b4a 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -11,7 +11,7 @@ const { spawnSync } = require('child_process'); const fs = require('fs'); const crypto = require('crypto'); -const depPolicy =fixtures.path('policy', 'dep-policy.json'); +const depPolicy = fixtures.path('policy', 'dep-policy.json'); const dep = fixtures.path('policy', 'dep.js'); const emptyHash = crypto.createHash('sha512'); From 1678f4023b6c6543c42bb76935fed7c9c1c9f86d Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 17 Jul 2019 12:25:56 -0500 Subject: [PATCH 03/19] update cli.md and node.1 with policy-integrity flag --- doc/api/cli.md | 9 +++++++++ doc/node.1 | 3 +++ 2 files changed, 12 insertions(+) diff --git a/doc/api/cli.md b/doc/api/cli.md index dc695baed91ca5..5886161d67d0b0 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -426,6 +426,14 @@ 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` + + +Instructs node to error prior to running any code if the policy does not have +the specified integrity. + ### `--preserve-symlinks` Instructs node to error prior to running any code if the policy does not have From a6924e10661ddcdf30a8a77ae507dce11246fed6 Mon Sep 17 00:00:00 2001 From: Bradley Meck Date: Wed, 17 Jul 2019 12:36:53 -0500 Subject: [PATCH 05/19] Update doc/node.1 Co-Authored-By: mscdex --- doc/node.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/node.1 b/doc/node.1 index 5efd74b9b34761..51593b559c229c 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -228,7 +228,7 @@ 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 -experimental-policy +.It Fl -policy-integrity Instructs node to error prior to running any code if the policy does not have the specified integrity. . .It Fl -preserve-symlinks From 3252926a2244a6569637f7e79f35ff7201eb46ac Mon Sep 17 00:00:00 2001 From: Bradley Meck Date: Wed, 17 Jul 2019 12:37:01 -0500 Subject: [PATCH 06/19] Update doc/node.1 Co-Authored-By: Vse Mozhet Byt --- doc/node.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/node.1 b/doc/node.1 index 51593b559c229c..61db2812643121 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -229,7 +229,7 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is Emit pending deprecation warnings. . .It Fl -policy-integrity -Instructs node to error prior to running any code if the policy does not have the specified integrity. +Instructs Node.js to error prior to running any code if the policy does not have the specified integrity. . .It Fl -preserve-symlinks Instructs the module loader to preserve symbolic links when resolving and caching modules other than the main module. From ecb9153bb0e40c61d66df9ae5db496c04d072fa3 Mon Sep 17 00:00:00 2001 From: Bradley Meck Date: Wed, 17 Jul 2019 12:39:08 -0500 Subject: [PATCH 07/19] Update doc/api/cli.md Co-Authored-By: Vse Mozhet Byt --- doc/api/cli.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 3a05e1eff18de9..fa1a8501b95246 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -431,7 +431,7 @@ developers may leverage to detect deprecated API usage. added: REPLACEME --> -Instructs node to error prior to running any code if the policy does not have +Instructs Node.js to error prior to running any code if the policy does not have the specified integrity. ### `--preserve-symlinks` From 10d47aae615e2c7cd2a6e11c983a76643b360f92 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 17 Jul 2019 12:44:56 -0500 Subject: [PATCH 08/19] explain parameter for policy-integrity --- doc/api/cli.md | 5 +++-- doc/node.1 | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index fa1a8501b95246..274c3378a8af63 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -426,13 +426,13 @@ 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` +### `--policy-integrity=sri` Instructs Node.js to error prior to running any code if the policy does not have -the specified integrity. +the specified integrity. It expects a [Subresource Integrity] string as a parameter. ### `--preserve-symlinks` 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. +the specified integrity. It expects a [Subresource Integrity][] string as a +parameter. ### `--preserve-symlinks` +> 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. From f4ed6881bf1e1802ab809e644bd1fa3d6c5d3728 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 19 Jul 2019 14:27:50 -0500 Subject: [PATCH 13/19] stderr dump from CI --- test/parallel/test-policy-integrity-flag.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index f64e0be5d58b4a..c696f65d1a4b80 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -45,7 +45,7 @@ const depPolicySRI = `sha512-${policyHash.digest('base64')}`; assert.strictEqual(status, 9); } { - const { status } = spawnSync( + const { status, stderr } = spawnSync( process.execPath, [ '--policy-integrity', depPolicySRI, From 17dfaeabf5e5fe9c1097894d61a893ddefa79893 Mon Sep 17 00:00:00 2001 From: Bradley Meck Date: Fri, 19 Jul 2019 14:28:17 -0500 Subject: [PATCH 14/19] Update test/parallel/test-policy-integrity-flag.js Co-Authored-By: Rich Trott --- test/parallel/test-policy-integrity-flag.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index c696f65d1a4b80..f590958fd9f650 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -53,5 +53,5 @@ const depPolicySRI = `sha512-${policyHash.digest('base64')}`; ] ); - assert.strictEqual(status, 0); + assert.strictEqual(status, 0, `status: ${status}\nstderr: ${stderr}`); } From 32e4619080d3f4d6b2c88011c42b6ac9502c6d2a Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 19 Jul 2019 15:39:43 -0500 Subject: [PATCH 15/19] error in CI went away --- test/parallel/test-policy-integrity-flag.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index f590958fd9f650..f64e0be5d58b4a 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -45,7 +45,7 @@ const depPolicySRI = `sha512-${policyHash.digest('base64')}`; assert.strictEqual(status, 9); } { - const { status, stderr } = spawnSync( + const { status } = spawnSync( process.execPath, [ '--policy-integrity', depPolicySRI, @@ -53,5 +53,5 @@ const depPolicySRI = `sha512-${policyHash.digest('base64')}`; ] ); - assert.strictEqual(status, 0, `status: ${status}\nstderr: ${stderr}`); + assert.strictEqual(status, 0); } From 556907a3f2e19703303fbbdcd0e2367ad9350666 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 19 Jul 2019 16:16:10 -0500 Subject: [PATCH 16/19] win32 investigation continues --- test/parallel/test-policy-integrity-flag.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index f64e0be5d58b4a..f590958fd9f650 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -45,7 +45,7 @@ const depPolicySRI = `sha512-${policyHash.digest('base64')}`; assert.strictEqual(status, 9); } { - const { status } = spawnSync( + const { status, stderr } = spawnSync( process.execPath, [ '--policy-integrity', depPolicySRI, @@ -53,5 +53,5 @@ const depPolicySRI = `sha512-${policyHash.digest('base64')}`; ] ); - assert.strictEqual(status, 0); + assert.strictEqual(status, 0, `status: ${status}\nstderr: ${stderr}`); } From 8bbd89509ceb8d3c12e098c2ffd84b233b398202 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 19 Jul 2019 16:38:57 -0500 Subject: [PATCH 17/19] investigating win32 --- test/parallel/test-policy-integrity-flag.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index f590958fd9f650..b99f5fdfe71849 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -20,6 +20,10 @@ const emptySRI = `sha512-${emptyHash.digest('base64')}`; const policyHash = crypto.createHash('sha512'); policyHash.update(fs.readFileSync(depPolicy)); const depPolicySRI = `sha512-${policyHash.digest('base64')}`; +console.dir({ + depPolicySRI, + body: JSON.stringify(fs.readFileSync(depPolicy).toString('utf8')) +}) { const { status, stderr } = spawnSync( process.execPath, From de664014700ad4c9229e2fb11eae500e1620b497 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 19 Jul 2019 19:18:26 -0500 Subject: [PATCH 18/19] add multiple integrity values for diff line endings --- test/fixtures/policy/dep-policy.json | 2 +- test/parallel/test-policy-integrity-flag.js | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/fixtures/policy/dep-policy.json b/test/fixtures/policy/dep-policy.json index b148674a0a65ee..6cc483a578733c 100644 --- a/test/fixtures/policy/dep-policy.json +++ b/test/fixtures/policy/dep-policy.json @@ -1,7 +1,7 @@ { "resources": { "./dep.js": { - "integrity": "sha512-7CMcc2oytFfMnGQaXbJk84gYWF2J7p/fmWPW7dsnJyniD+vgxtK9VAZ/22UxFOA4q5d27RoGLxSqNZ/nGCJkMw==" + "integrity": "sha512-7CMcc2oytFfMnGQaXbJk84gYWF2J7p/fmWPW7dsnJyniD+vgxtK9VAZ/22UxFOA4q5d27RoGLxSqNZ/nGCJkMw== sha512-scgN9Td0bGMlGH2lUHvEeHtz92Hx6AO+sYhU3WRI6bn3jEUCXbXJs68nOOsGzRWR7a2tbqGoETnOCpHHf1Njhw==" } } } diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index b99f5fdfe71849..a3be164f001961 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -19,7 +19,11 @@ emptyHash.update(''); const emptySRI = `sha512-${emptyHash.digest('base64')}`; const policyHash = crypto.createHash('sha512'); policyHash.update(fs.readFileSync(depPolicy)); -const depPolicySRI = `sha512-${policyHash.digest('base64')}`; +// When using \n only +const nixPolicySRI = 'sha512-u/nXI6UacK5fKDC2bopcgnuQY4JXJKlK3dESO3GIKKxwogVHjJqpF9rgk7Zw+TJXIc96xBUWKHuUgOzic8/4tQ=='; +// When \n is turned into \r\n +const windowsPolicySRI = 'sha512-OeyCPRo4OZMosHyquZXDHpuU1F4KzG9UHFnn12FMaHsvqFUt3TFZ+7wmZE7ThZ5rsQWkUjc9ZH0knGZ2e8BYPQ=='; +const depPolicySRI = `${nixPolicySRI} ${windowsPolicySRI}`; console.dir({ depPolicySRI, body: JSON.stringify(fs.readFileSync(depPolicy).toString('utf8')) From 14d04ed0d48d8b7f2ab8d25e2ed177c18c0ef31a Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 20 Jul 2019 12:28:25 -0700 Subject: [PATCH 19/19] lint-fixup --- test/parallel/test-policy-integrity-flag.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index a3be164f001961..3b332758d18c0a 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -19,15 +19,19 @@ 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,