diff options
author | Bradley Farias <bradley.meck@gmail.com> | 2019-01-23 16:21:46 -0600 |
---|---|---|
committer | Daniel Bevenius <daniel.bevenius@gmail.com> | 2019-01-29 08:08:50 +0100 |
commit | 78982389ce9265f5f65cd9e8a192e05389506927 (patch) | |
tree | eb20a408615edbc768ed025470c30e6ef5927f4e | |
parent | d4d76b6be07841e4416fc5ab1445b73638fcea6a (diff) | |
download | android-node-v8-78982389ce9265f5f65cd9e8a192e05389506927.tar.gz android-node-v8-78982389ce9265f5f65cd9e8a192e05389506927.tar.bz2 android-node-v8-78982389ce9265f5f65cd9e8a192e05389506927.zip |
policy: ensure workers do not read fs for policy
This prevents a main thread from rewriting the policy file and loading
a worker that has a different policy from the main thread.
PR-URL: https://github.com/nodejs/node/pull/25710
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
-rw-r--r-- | lib/internal/bootstrap/node.js | 10 | ||||
-rw-r--r-- | lib/internal/process/policy.js | 18 | ||||
-rw-r--r-- | lib/internal/process/worker_thread_only.js | 16 | ||||
-rw-r--r-- | lib/internal/worker.js | 4 | ||||
-rw-r--r-- | test/parallel/test-policy-integrity.js | 106 |
5 files changed, 138 insertions, 16 deletions
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 86c0cf7aaf..789d8c1754 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -181,7 +181,7 @@ function startup() { // TODO(joyeecheung): move this down further to get better snapshotting const experimentalPolicy = getOptionValue('--experimental-policy'); - if (experimentalPolicy) { + if (isMainThread && experimentalPolicy) { process.emitWarning('Policies are experimental.', 'ExperimentalWarning'); const { pathToFileURL, URL } = NativeModule.require('url'); @@ -348,8 +348,6 @@ function startup() { } function startWorkerThreadExecution() { - prepareUserCodeExecution(); - // If we are in a worker thread, execute the script sent through the // message port. const { @@ -366,7 +364,9 @@ function startWorkerThreadExecution() { debug(`[${threadId}] is setting up worker child environment`); const port = getEnvMessagePort(); - port.on('message', createMessageHandler(port)); + port.on('message', createMessageHandler( + port, + prepareUserCodeExecution)); port.start(); // Overwrite fatalException @@ -460,7 +460,7 @@ function prepareUserCodeExecution() { // For user code, we preload modules if `-r` is passed const preloadModules = getOptionValue('--require'); - if (preloadModules) { + if (preloadModules.length) { const { _preloadModules } = NativeModule.require('internal/modules/cjs/loader'); diff --git a/lib/internal/process/policy.js b/lib/internal/process/policy.js index 0b037d4ef5..823130e3c1 100644 --- a/lib/internal/process/policy.js +++ b/lib/internal/process/policy.js @@ -5,10 +5,14 @@ const { } = require('internal/errors').codes; const { Manifest } = require('internal/policy/manifest'); let manifest; +let manifestSrc; +let manifestURL; module.exports = Object.freeze({ __proto__: null, setup(src, url) { + manifestSrc = src; + manifestURL = url; if (src === null) { manifest = null; return; @@ -31,6 +35,20 @@ module.exports = Object.freeze({ return manifest; }, + get src() { + if (typeof manifestSrc === 'undefined') { + throw new ERR_MANIFEST_TDZ(); + } + return manifestSrc; + }, + + get url() { + if (typeof manifestURL === 'undefined') { + throw new ERR_MANIFEST_TDZ(); + } + return manifestURL; + }, + assertIntegrity(moduleURL, content) { this.manifest.matchesIntegrity(moduleURL, content); } diff --git a/lib/internal/process/worker_thread_only.js b/lib/internal/process/worker_thread_only.js index b29bc7a390..2171d2b586 100644 --- a/lib/internal/process/worker_thread_only.js +++ b/lib/internal/process/worker_thread_only.js @@ -43,12 +43,24 @@ function initializeWorkerStdio() { }; } -function createMessageHandler(port) { +function createMessageHandler(port, prepareUserCodeExecution) { const publicWorker = require('worker_threads'); return function(message) { if (message.type === messageTypes.LOAD_SCRIPT) { - const { filename, doEval, workerData, publicPort, hasStdin } = message; + const { + filename, + doEval, + workerData, + publicPort, + manifestSrc, + manifestURL, + hasStdin + } = message; + if (manifestSrc) { + require('internal/process/policy').setup(manifestSrc, manifestURL); + } + prepareUserCodeExecution(); publicWorker.parentPort = publicPort; publicWorker.workerData = workerData; diff --git a/lib/internal/worker.js b/lib/internal/worker.js index e20a37af8a..1640a9dd88 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -12,6 +12,7 @@ const { ERR_INVALID_ARG_TYPE, } = require('internal/errors').codes; const { validateString } = require('internal/validators'); +const { getOptionValue } = require('internal/options'); const { drainMessagePort, @@ -112,6 +113,9 @@ class Worker extends EventEmitter { doEval: !!options.eval, workerData: options.workerData, publicPort: port2, + manifestSrc: getOptionValue('--experimental-policy') ? + require('internal/process/policy').src : + null, hasStdin: !!options.stdin }, [port2]); // Actually start the new thread now that everything is in place. diff --git a/test/parallel/test-policy-integrity.js b/test/parallel/test-policy-integrity.js index 5c1ea4fc4e..86d1078d9f 100644 --- a/test/parallel/test-policy-integrity.js +++ b/test/parallel/test-policy-integrity.js @@ -33,6 +33,17 @@ const parentFilepath = path.join(tmpdir.path, 'parent.js'); const parentURL = pathToFileURL(parentFilepath); const parentBody = 'require(\'./dep.js\')'; +const workerSpawningFilepath = path.join(tmpdir.path, 'worker_spawner.js'); +const workerSpawningURL = pathToFileURL(workerSpawningFilepath); +const workerSpawningBody = ` +const { Worker } = require('worker_threads'); +// make sure this is gone to ensure we don't do another fs read of it +// will error out if we do +require('fs').unlinkSync(${JSON.stringify(policyFilepath)}); +const w = new Worker(${JSON.stringify(parentFilepath)}); +w.on('exit', process.exit); +`; + const depFilepath = path.join(tmpdir.path, 'dep.js'); const depURL = pathToFileURL(depFilepath); const depBody = ''; @@ -49,8 +60,9 @@ if (!tmpdirURL.pathname.endsWith('/')) { } function test({ shouldFail = false, + preload = [], entry, - onerror, + onerror = undefined, resources = {} }) { const manifest = { @@ -65,7 +77,9 @@ function test({ } fs.writeFileSync(policyFilepath, JSON.stringify(manifest, null, 2)); const { status } = spawnSync(process.execPath, [ - '--experimental-policy', policyFilepath, entry + '--experimental-policy', policyFilepath, + ...preload.map((m) => ['-r', m]).flat(), + entry ]); if (shouldFail) { assert.notStrictEqual(status, 0); @@ -74,13 +88,25 @@ function test({ } } -const { status } = spawnSync(process.execPath, [ - '--experimental-policy', policyFilepath, - '--experimental-policy', policyFilepath -], { - stdio: 'pipe' -}); -assert.notStrictEqual(status, 0, 'Should not allow multiple policies'); +{ + const { status } = spawnSync(process.execPath, [ + '--experimental-policy', policyFilepath, + '--experimental-policy', policyFilepath + ], { + stdio: 'pipe' + }); + assert.notStrictEqual(status, 0, 'Should not allow multiple policies'); +} +{ + const enoentFilepath = path.join(tmpdir.path, 'enoent'); + try { fs.unlinkSync(enoentFilepath); } catch {} + const { status } = spawnSync(process.execPath, [ + '--experimental-policy', enoentFilepath, '-e', '' + ], { + stdio: 'pipe' + }); + assert.notStrictEqual(status, 0, 'Should not allow missing policies'); +} test({ shouldFail: true, @@ -196,6 +222,21 @@ test({ } }); test({ + shouldFail: false, + preload: [depFilepath], + entry: parentFilepath, + resources: { + [parentURL]: { + body: parentBody, + match: true, + }, + [depURL]: { + body: depBody, + match: true, + } + } +}); +test({ shouldFail: true, entry: parentFilepath, resources: { @@ -295,3 +336,50 @@ test({ } } }); +test({ + shouldFail: true, + entry: workerSpawningFilepath, + resources: { + [workerSpawningURL]: { + body: workerSpawningBody, + match: true, + }, + } +}); +test({ + shouldFail: false, + entry: workerSpawningFilepath, + resources: { + [workerSpawningURL]: { + body: workerSpawningBody, + match: true, + }, + [parentURL]: { + body: parentBody, + match: true, + }, + [depURL]: { + body: depBody, + match: true, + } + } +}); +test({ + shouldFail: false, + entry: workerSpawningFilepath, + preload: [parentFilepath], + resources: { + [workerSpawningURL]: { + body: workerSpawningBody, + match: true, + }, + [parentURL]: { + body: parentBody, + match: true, + }, + [depURL]: { + body: depBody, + match: true, + } + } +}); |