From af9140088621abd09016848f4526d66b7a81b9ba Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Tue, 25 Oct 2022 00:23:27 -0300 Subject: [PATCH] policy: makeRequireFunction on mainModule.require Signed-off-by: RafaelGSS Co-authored-by: Bradley Farias 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 Reviewed-by: Matteo Collina --- lib/internal/modules/cjs/loader.js | 58 +++++++++------- lib/internal/modules/helpers.js | 12 +++- src/env_properties.h | 3 +- .../policy-manifest/main-module-bypass.js | 1 + .../object-define-property-bypass.js | 19 ++++++ .../policy-manifest/onerror-exit.json | 9 +++ .../onerror-resource-exit.json | 17 +++++ test/fixtures/policy-manifest/valid-module.js | 0 test/parallel/test-policy-manifest.js | 67 +++++++++++++++---- 9 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 test/fixtures/policy-manifest/main-module-bypass.js create mode 100644 test/fixtures/policy-manifest/object-define-property-bypass.js create mode 100644 test/fixtures/policy-manifest/onerror-exit.json create mode 100644 test/fixtures/policy-manifest/onerror-resource-exit.json create mode 100644 test/fixtures/policy-manifest/valid-module.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 2852dc2eb9cdbe..de7f37ad133042 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/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; }; diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index e3fbd3a3eb84a4..b5cc52401f9ddc 100644 --- a/lib/internal/modules/helpers.js +++ b/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); }; } diff --git a/src/env_properties.h b/src/env_properties.h index 3960b0add85d79..c4c30eb5e416ad 100644 --- a/src/env_properties.h +++ b/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. diff --git a/test/fixtures/policy-manifest/main-module-bypass.js b/test/fixtures/policy-manifest/main-module-bypass.js new file mode 100644 index 00000000000000..a1cec2ddd3ffbb --- /dev/null +++ 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 new file mode 100644 index 00000000000000..5543fd35b28b26 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..24bd92817d24b1 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..f08bc8d32c07e7 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/test/parallel/test-policy-manifest.js b/test/parallel/test-policy-manifest.js index a8494175f3e68f..f8bebdf4cf69f1 100644 --- a/test/parallel/test-policy-manifest.js +++ b/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 result = spawnSync(process.execPath, [ - '--experimental-policy', - policyFilepath, - './fhqwhgads.js', -]); - -assert.notStrictEqual(result.status, 0); -const stderr = result.stderr.toString(); -assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/); -assert.match(stderr, /pattern needs to have a single trailing "\*"/); +{ + const policyFilepath = fixtures.path('policy-manifest', 'invalid.json'); + const result = spawnSync(process.execPath, [ + '--experimental-policy', + policyFilepath, + './fhqwhgads.js', + ]); + + assert.notStrictEqual(result.status, 0); + const stderr = result.stderr.toString(); + assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/); + assert.match(stderr, /pattern needs to have a single trailing "\*"/); +} + +{ + 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 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); +}