policy: makeRequireFunction on mainModule.require

Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> Co-authored-by: Bradley Farias <bradley.meck@gmail.com> Backport-PR-URL: nodejs-private/node-private#373 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1747642 CVE-ID: CVE-2023-23918 PR-URL: nodejs-private/node-private#358 Reviewed-by: Bradley Farias <bradley.meck@gmail.com> Reviewed-by: Michael Dawson <midawson@redhat.com>
nodejs · Feb 15, 2023 · 83975b7 · 83975b7
1 parent fa115ee
commit 83975b7
Show file tree

Hide file tree

Showing 10 changed files with 158 additions and 30 deletions.
diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js
@@ -26,6 +26,10 @@ const { getOptionValue } = require('internal/options');
 const { setOwnProperty } = require('internal/util');
 const userConditions = getOptionValue('--conditions');
 
+const {
+  require_private_symbol,
+} = internalBinding('util');
+
 let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
   debug = fn;
 });
@@ -85,7 +89,7 @@ function makeRequireFunction(mod, redirects) {
             filepath = fileURLToPath(destination);
             urlToFileCache.set(href, filepath);
           }
-          return mod.require(filepath);
+          return mod[require_private_symbol](mod, filepath);
         }
       }
       if (missing) {
@@ -95,11 +99,11 @@ function makeRequireFunction(mod, redirects) {
           ArrayPrototypeJoin([...conditions], ', ')
         ));
       }
-      return mod.require(specifier);
+      return mod[require_private_symbol](mod, specifier);
     };
   } else {
     require = function require(path) {
-      return mod.require(path);
+      return mod[require_private_symbol](mod, path);
     };
   }
 

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -76,7 +76,11 @@ const {
   maybeCacheSourceMap,
 } = require('internal/source_map/source_map_cache');
 const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url');
-const { deprecate, filterOwnProperties, setOwnProperty } = require('internal/util');
+const {
+  deprecate,
+  filterOwnProperties,
+  setOwnProperty,
+} = require('internal/util');
 const vm = require('vm');
 const assert = require('internal/assert');
 const fs = require('fs');
@@ -86,6 +90,9 @@ const { sep } = path;
 const { internalModuleStat } = internalBinding('fs');
 const packageJsonReader = require('internal/modules/package_json_reader');
 const { safeGetenv } = internalBinding('credentials');
+const {
+  require_private_symbol,
+} = internalBinding('util');
 const {
   makeRequireFunction,
   normalizeReferrerURL,
@@ -143,6 +150,20 @@ let requireDepth = 0;
 let statCache = null;
 let isPreloading = false;
 
+function internalRequire(module, id) {
+  validateString(id, 'id');
+  if (id === '') {
+    throw new ERR_INVALID_ARG_VALUE('id', id,
+                                    'must be a non-empty string');
+  }
+  requireDepth++;
+  try {
+    return Module._load(id, module, /* isMain */ false);
+  } finally {
+    requireDepth--;
+  }
+}
+
 function stat(filename) {
   filename = path.toNamespacedPath(filename);
   if (statCache !== null) {
@@ -169,6 +190,15 @@ function Module(id = '', parent) {
   this.filename = null;
   this.loaded = false;
   this.children = [];
+  let redirects;
+  if (policy?.manifest) {
+    const moduleURL = pathToFileURL(id);
+    redirects = policy.manifest.getDependencyMapper(moduleURL);
+  }
+  setOwnProperty(this, 'require', makeRequireFunction(this, redirects));
+  // Loads a module at the given file path. Returns that module's
+  // `exports` property.
+  this[require_private_symbol] = internalRequire;
 }
 
 const builtinModules = [];
@@ -776,6 +806,7 @@ Module._load = function(request, parent, isMain) {
 
   if (isMain) {
     process.mainModule = module;
+    setOwnProperty(module.require, 'main', process.mainModule);
     module.id = '.';
   }
 
@@ -959,24 +990,6 @@ Module.prototype.load = function(filename) {
     ESMLoader.cjsCache.set(this, exports);
 };
 
-
-// Loads a module at the given file path. Returns that module's
-// `exports` property.
-Module.prototype.require = function(id) {
-  validateString(id, 'id');
-  if (id === '') {
-    throw new ERR_INVALID_ARG_VALUE('id', id,
-                                    'must be a non-empty string');
-  }
-  requireDepth++;
-  try {
-    return Module._load(id, this, /* isMain */ false);
-  } finally {
-    requireDepth--;
-  }
-};
-
-
 // Resolved path to process.argv[1] will be lazily placed here
 // (needed for setting breakpoint when called with --inspect-brk)
 let resolvedArgv;
@@ -1037,10 +1050,9 @@ function wrapSafe(filename, content, cjsModuleInstance) {
 // Returns exception, if any.
 Module.prototype._compile = function(content, filename) {
   let moduleURL;
-  let redirects;
   if (policy?.manifest) {
     moduleURL = pathToFileURL(filename);
-    redirects = policy.manifest.getDependencyMapper(moduleURL);
+    policy.manifest.getDependencyMapper(moduleURL);
     policy.manifest.assertIntegrity(moduleURL, content);
   }
 
@@ -1071,18 +1083,17 @@ Module.prototype._compile = function(content, filename) {
     }
   }
   const dirname = path.dirname(filename);
-  const require = makeRequireFunction(this, redirects);
   let result;
   const exports = this.exports;
   const thisValue = exports;
   const module = this;
   if (requireDepth === 0) statCache = new SafeMap();
   if (inspectorWrapper) {
     result = inspectorWrapper(compiledWrapper, thisValue, exports,
-                              require, module, filename, dirname);
+                              module.require, module, filename, dirname);
   } else {
     result = ReflectApply(compiledWrapper, thisValue,
-                          [exports, require, module, filename, dirname]);
+                          [exports, module.require, module, filename, dirname]);
   }
   hasLoadedAnyUserCJSModule = true;
   if (requireDepth === 0) statCache = null;
@@ -1240,7 +1251,7 @@ Module._preloadModules = function(requests) {
     }
   }
   for (let n = 0; n < requests.length; n++)
-    parent.require(requests[n]);
+    internalRequire(parent, requests[n]);
   isPreloading = false;
 };
 

diff --git a/src/env.h b/src/env.h
@@ -153,6 +153,7 @@ constexpr size_t kFsStatsBufferLength =
   V(napi_type_tag, "node:napi:type_tag")                                      \
   V(napi_wrapper, "node:napi:wrapper")                                        \
   V(untransferable_object_private_symbol, "node:untransferableObject")        \
+  V(require_private_symbol, "node:require_private_symbol")
 
 // Symbols are per-isolate primitives but Environment proxies them
 // for the sake of convenience.

diff --git a/test/fixtures/policy-manifest/main-module-bypass.js b/test/fixtures/policy-manifest/main-module-bypass.js
@@ -0,0 +1 @@
+process.mainModule.require('os').cpus();
diff --git a/test/fixtures/policy-manifest/object-define-property-bypass.js b/test/fixtures/policy-manifest/object-define-property-bypass.js
@@ -0,0 +1,19 @@
+let requires = new WeakMap()
+Object.defineProperty(Object.getPrototypeOf(module), 'require', {
+  get() {
+    return requires.get(this);
+  },
+  set(v) {
+    requires.set(this, v);
+    process.nextTick(() => {
+      let fs = Reflect.apply(v, this, ['fs'])
+      if (typeof fs.readFileSync === 'function') {
+        process.exit(1);
+      }
+    })
+    return requires.get(this);
+  },
+  configurable: true
+})
+
+require('./valid-module')
diff --git a/test/fixtures/policy-manifest/onerror-exit.json b/test/fixtures/policy-manifest/onerror-exit.json
@@ -0,0 +1,9 @@
+{
+  "onerror": "exit",
+  "scopes": {
+    "file:": {
+      "integrity": true,
+      "dependencies": {}
+    }
+  }
+}
diff --git a/test/fixtures/policy-manifest/onerror-resource-exit.json b/test/fixtures/policy-manifest/onerror-resource-exit.json
@@ -0,0 +1,17 @@
+{
+  "onerror": "exit",
+  "resources": {
+    "./object-define-property-bypass.js": {
+      "integrity": true,
+      "dependencies": {
+        "./valid-module": true
+      }
+    },
+    "./valid-module.js": {
+      "integrity": true,
+      "dependencies": {
+        "fs": true
+      }
+    }
+  }
+}
diff --git a/test/fixtures/policy-manifest/valid-module.js b/test/fixtures/policy-manifest/valid-module.js
diff --git a/test/parallel/test-module-prototype-mutation.js b/test/parallel/test-module-prototype-mutation.js
@@ -4,9 +4,12 @@ const fixtures = require('../common/fixtures');
 const assert = require('assert');
 
 assert.strictEqual(
-  require(fixtures.path('es-module-specifiers', 'node_modules', 'no-main-field')),
+  require(
+    fixtures.path('es-module-specifiers', 'node_modules', 'no-main-field')
+  ),
   'no main field'
 );
 
 import(fixtures.fileURL('es-module-specifiers', 'index.mjs'))
-  .then(common.mustCall((module) => assert.strictEqual(module.noMain, 'no main field')));
+  .then(common.mustCall((module) =>
+    assert.strictEqual(module.noMain, 'no main field')));
diff --git a/test/parallel/test-policy-manifest.js b/test/parallel/test-policy-manifest.js
@@ -0,0 +1,63 @@
+'use strict';
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+common.requireNoPackageJSONAbove();
+
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const fixtures = require('../common/fixtures.js');
+
+{
+  const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
+  const result = spawnSync(process.execPath, [
+    '--experimental-policy',
+    policyFilepath,
+    '-e',
+    'require("os").cpus()',
+  ]);
+
+  assert.notStrictEqual(result.status, 0);
+  const stderr = result.stderr.toString();
+  assert.match(stderr, /ERR_MANIFEST_DEPENDENCY_MISSING/);
+  assert.match(stderr, /does not list os as a dependency specifier for conditions: require, node, node-addons/);
+}
+
+{
+  const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
+  const mainModuleBypass = fixtures.path(
+    'policy-manifest',
+    'main-module-bypass.js',
+  );
+  const result = spawnSync(process.execPath, [
+    '--experimental-policy',
+    policyFilepath,
+    mainModuleBypass,
+  ]);
+
+  assert.notStrictEqual(result.status, 0);
+  const stderr = result.stderr.toString();
+  assert.match(stderr, /ERR_MANIFEST_DEPENDENCY_MISSING/);
+  assert.match(stderr, /does not list os as a dependency specifier for conditions: require, node, node-addons/);
+}
+
+{
+  const policyFilepath = fixtures.path(
+    'policy-manifest',
+    'onerror-resource-exit.json',
+  );
+  const objectDefinePropertyBypass = fixtures.path(
+    'policy-manifest',
+    'object-define-property-bypass.js',
+  );
+  const result = spawnSync(process.execPath, [
+    '--experimental-policy',
+    policyFilepath,
+    objectDefinePropertyBypass,
+  ]);
+
+  assert.strictEqual(result.status, 0);
+}