policy: makeRequireFunction on mainModule.require

Co-authored-by: Bradley Farias <[email protected]>
Backport-PR-URL: nodejs-private/node-private#372
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 <[email protected]>
Reviewed-by: Michael Dawson <[email protected]>

Author: 2 people

lib/internal/modules/cjs/helpers.js

@@ -27,6 +27,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;
 });
@@ -86,7 +90,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) {
@@ -96,11 +100,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);
     };
   }
 

lib/internal/modules/cjs/loader.js

@@ -95,6 +95,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 {
   cjsConditions,
   hasEsmSyntax,
@@ -155,6 +158,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) {
@@ -203,6 +220,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 = [];
@@ -863,6 +889,7 @@ Module._load = function(request, parent, isMain) {
 
   if (isMain) {
     process.mainModule = module;
+    setOwnProperty(module.require, 'main', process.mainModule);
     module.id = '.';
   }
 
@@ -1053,24 +1080,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;
@@ -1118,10 +1127,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);
   }
 
@@ -1152,18 +1160,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;
@@ -1339,7 +1346,7 @@ Module._preloadModules = function(requests) {
     }
   }
   for (let n = 0; n < requests.length; n++)
-    parent.require(requests[n]);
+    internalRequire(parent, requests[n]);
   isPreloading = false;
 };
 

src/env.h

@@ -174,7 +174,8 @@ class NoArrayBufferZeroFillScope {
   V(napi_type_tag, "node:napi:type_tag")                                       \
   V(napi_wrapper, "node:napi:wrapper")                                         \
   V(untransferable_object_private_symbol, "node:untransferableObject")         \
-  V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array")
+  V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array")           \
+  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/fixtures/policy-manifest/valid-module.js

@@ -8,7 +8,7 @@ Error: an error!
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
     at Module.load (node:internal/modules/cjs/loader:*)
     at Function.Module._load (node:internal/modules/cjs/loader:*)
-    at Module.require (node:internal/modules/cjs/loader:*)
+    at Module.internalRequire (node:internal/modules/cjs/loader:*)
 *enclosing-call-site.js:16
       throw new Error('an error!')
                 ^
@@ -23,4 +23,4 @@ Error: an error!
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
     at Module.load (node:internal/modules/cjs/loader:*)
     at Function.Module._load (node:internal/modules/cjs/loader:*)
-    at Module.require (node:internal/modules/cjs/loader:*)
+    at Module.internalRequire (node:internal/modules/cjs/loader:*)

test/message/source_map_disabled_by_api.out

@@ -12,7 +12,7 @@ Error: an error!
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
     at Module.load (node:internal/modules/cjs/loader:*)
     at Function.Module._load (node:internal/modules/cjs/loader:*)
-    at Module.require (node:internal/modules/cjs/loader:*)
+    at Module.internalRequire (node:internal/modules/cjs/loader:*)
 *enclosing-call-site-min.js:1
 var functionA=function(){functionB()};function functionB(){functionC()}var functionC=function(){functionD()},functionD=function(){if(0<Math.random())throw Error("an error!");},thrower=functionA;try{functionA()}catch(a){throw a;};
                                                                                                                                                                                                                            ^
@@ -27,4 +27,4 @@ Error: an error!
     at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
     at Module.load (node:internal/modules/cjs/loader:*)
     at Function.Module._load (node:internal/modules/cjs/loader:*)
-    at Module.require (node:internal/modules/cjs/loader:*)
+    at Module.internalRequire (node:internal/modules/cjs/loader:*)