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: #25710 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Jan 29, 2019 · f3179f7 · f3179f7
1 parent ebcdbeb
commit f3179f7
Show file tree

Hide file tree

Showing 5 changed files with 138 additions and 16 deletions.
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -173,7 +173,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');
@@ -364,8 +364,6 @@ function startup() {
 }
 
 function startWorkerThreadExecution() {
-  prepareUserCodeExecution();
-
   // If we are in a worker thread, execute the script sent through the
   // message port.
   const {
@@ -382,7 +380,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
@@ -476,7 +476,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
@@ -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
@@ -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
@@ -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
@@ -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,
@@ -195,6 +221,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,
@@ -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,
+    }
+  }
+});