policy: makeRequireFunction on mainModule.require

Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>

Co-authored-by: Bradley Farias <bradley.meck@gmail.com>
Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1747642
CVE-ID: CVE-2023-23918
PR-URL: https://github.com/nodejs-private/node-private/pull/358
Reviewed-by: Bradley Farias <bradley.meck@gmail.com>
Reviewed-by: Matteo Collina <matteo.collina@gmail.com>

lib/internal/modules/cjs/loader.js

@@ -96,6 +96,11 @@ const path = require('path');
 const { sep } = path;
 const { internalModuleStat } = internalBinding('fs');
 const { safeGetenv } = internalBinding('credentials');
+const {
+  privateSymbols: {
+    require_private_symbol,
+  },
+} = internalBinding('util');
 const {
   getCjsConditions,
   initializeCjsConditions,
@@ -151,6 +156,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) {
@@ -205,6 +224,16 @@ function Module(id = '', parent) {
   this.filename = null;
   this.loaded = false;
   this.children = [];
+  let redirects;
+  const manifest = policy()?.manifest;
+  if (manifest) {
+    const moduleURL = pathToFileURL(id);
+    redirects = 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;
 }
 
 Module._cache = { __proto__: null };
@@ -927,6 +956,7 @@ Module._load = function(request, parent, isMain) {
 
   if (isMain) {
     process.mainModule = module;
+    setOwnProperty(module.require, 'main', process.mainModule);
     module.id = '.';
   }
 
@@ -1113,24 +1143,6 @@ Module.prototype.load = function(filename) {
     cascadedLoader.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;
@@ -1199,11 +1211,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
 // Returns exception, if any.
 Module.prototype._compile = function(content, filename) {
   let moduleURL;
-  let redirects;
   const manifest = policy()?.manifest;
   if (manifest) {
     moduleURL = pathToFileURL(filename);
-    redirects = manifest.getDependencyMapper(moduleURL);
+    manifest.getDependencyMapper(moduleURL);
     manifest.assertIntegrity(moduleURL, content);
   }
 
@@ -1233,7 +1244,6 @@ 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;
@@ -1241,10 +1251,10 @@ Module.prototype._compile = function(content, filename) {
   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;
@@ -1422,7 +1432,7 @@ Module._preloadModules = function(requests) {
     }
   }
   for (let n = 0; n < requests.length; n++)
-    parent.require(requests[n]);
+    internalRequire(parent, requests[n]);
   isPreloading = false;
 };
 

lib/internal/modules/helpers.js

@@ -26,6 +26,12 @@ const { pathToFileURL, fileURLToPath, URL } = require('internal/url');
 const { getOptionValue } = require('internal/options');
 const { setOwnProperty } = require('internal/util');
 
+const {
+  privateSymbols: {
+    require_private_symbol,
+  },
+} = internalBinding('util');
+
 let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
   debug = fn;
 });
@@ -95,7 +101,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) {
@@ -105,11 +111,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);
     };
   }
 

src/env_properties.h

@@ -24,7 +24,8 @@
   V(napi_type_tag, "node:napi:type_tag")                                       \
   V(napi_wrapper, "node:napi:wrapper")                                         \
   V(untransferable_object_private_symbol, "node:untransferableObject")         \
-  V(exit_info_private_symbol, "node:exit_info_private_symbol")
+  V(exit_info_private_symbol, "node:exit_info_private_symbol")                 \
+  V(require_private_symbol, "node:require_private_symbol")
 
 // Symbols are per-isolate primitives but Environment proxies them
 // for the sake of convenience.

test/fixtures/policy-manifest/main-module-bypass.js

@@ -0,0 +1 @@
+process.mainModule.require('os').cpus();

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')

test/fixtures/policy-manifest/onerror-exit.json

@@ -0,0 +1,9 @@
+{
+  "onerror": "exit",
+  "scopes": {
+    "file:": {
+      "integrity": true,
+      "dependencies": {}
+    }
+  }
+}

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
+      }
+    }
+  }
+}

test/parallel/test-policy-manifest.js

@@ -11,15 +11,58 @@ const assert = require('assert');
 const { spawnSync } = require('child_process');
 const fixtures = require('../common/fixtures.js');
 
-const policyFilepath = fixtures.path('policy-manifest', 'invalid.json');
+{
+  const policyFilepath = fixtures.path('policy-manifest', 'invalid.json');
+  const result = spawnSync(process.execPath, [
+    '--experimental-policy',
+    policyFilepath,
+    './fhqwhgads.js',
+  ]);
 
-const result = spawnSync(process.execPath, [
+  assert.notStrictEqual(result.status, 0);
-  '--experimental-policy',
+  const stderr = result.stderr.toString();
-  policyFilepath,
+  assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/);
-  './fhqwhgads.js',
+  assert.match(stderr, /pattern needs to have a single trailing "\*"/);
-]);
+}
 
-assert.notStrictEqual(result.status, 0);
+{
-const stderr = result.stderr.toString();
+  const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
-assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/);
+  const result = spawnSync(process.execPath, [
-assert.match(stderr, /pattern needs to have a single trailing "\*"/);
+    '--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 module 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);
+}