From 83975b7fb463e29c92c8e83693b725e269cee149 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 Backport-PR-URL: https://github.com/nodejs-private/node-private/pull/373 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: Michael Dawson --- lib/internal/modules/cjs/helpers.js | 10 ++- lib/internal/modules/cjs/loader.js | 61 ++++++++++-------- src/env.h | 1 + .../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-module-prototype-mutation.js | 7 ++- test/parallel/test-policy-manifest.js | 63 +++++++++++++++++++ 10 files changed, 158 insertions(+), 30 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 create mode 100644 test/parallel/test-policy-manifest.js diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 7fed1a18370d59..b5baf5b59be225 100644 --- 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 index 5e61ae80ce74f6..730e4ebddccabd 100644 --- 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,7 +1083,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; @@ -1079,10 +1090,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; @@ -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 index 824202ef5258e4..a904dce632453d 100644 --- 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 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-module-prototype-mutation.js b/test/parallel/test-module-prototype-mutation.js index 9e50f91a7fdb2b..36b22f45a288de 100644 --- 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 new file mode 100644 index 00000000000000..6cce935a04583c --- /dev/null +++ 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); +}