From b68e5e798138be0041ba9ace72d8d45e63c068a1 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Mon, 29 May 2023 19:45:33 -0300 Subject: [PATCH] policy: handle Module.constructor and main.extensions bypass Signed-off-by: RafaelGSS PR-URL: https://github.com/nodejs-private/node-private/pull/417 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1960870 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=2043807 Reviewed-By: Geoffrey Booth CVE-ID: CVE-2023-32002,CVE-2023-32006 --- lib/internal/modules/cjs/loader.js | 11 +++- lib/internal/modules/helpers.js | 13 ++++- .../policy-manifest/createRequire-bypass.js | 2 + .../policy-manifest/invalid-module.js | 0 .../main-constructor-bypass.js | 2 + .../main-constructor-extensions-bypass.js | 2 + .../policy-manifest/manifest-impersonate.json | 13 +++++ .../module-constructor-bypass.js | 1 + test/parallel/test-policy-manifest.js | 55 +++++++++++++++++++ 9 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 test/fixtures/policy-manifest/createRequire-bypass.js create mode 100644 test/fixtures/policy-manifest/invalid-module.js create mode 100644 test/fixtures/policy-manifest/main-constructor-bypass.js create mode 100644 test/fixtures/policy-manifest/main-constructor-extensions-bypass.js create mode 100644 test/fixtures/policy-manifest/manifest-impersonate.json create mode 100644 test/fixtures/policy-manifest/module-constructor-bypass.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 011051f87d8bfb..19a7d7e671f5ab 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -152,8 +152,8 @@ const isWindows = process.platform === 'win32'; const relativeResolveCache = { __proto__: null }; let requireDepth = 0; -let statCache = null; let isPreloading = false; +let statCache = null; function internalRequire(module, id) { validateString(id, 'id'); @@ -1429,5 +1429,14 @@ Module.syncBuiltinESMExports = function syncBuiltinESMExports() { } }; +ObjectDefineProperty(Module.prototype, 'constructor', { + __proto__: null, + get: function() { + return policy() ? undefined : Module; + }, + configurable: false, + enumerable: false, +}); + // Backwards compatibility Module.Module = Module; diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index 3cdffe9e49d74c..307a34cb09b512 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -14,6 +14,7 @@ const { StringPrototypeStartsWith, } = primordials; const { + ERR_INVALID_ARG_TYPE, ERR_MANIFEST_DEPENDENCY_MISSING, ERR_UNKNOWN_BUILTIN_MODULE, } = require('internal/errors').codes; @@ -68,12 +69,22 @@ function loadBuiltinModule(filename, request) { return mod; } +let $Module = null; +function lazyModule() { + $Module = $Module || require('internal/modules/cjs/loader').Module; + return $Module; +} + // Invoke with makeRequireFunction(module) where |module| is the Module object // to use as the context for the require() function. // Use redirects to set up a mapping from a policy and restrict dependencies const urlToFileCache = new SafeMap(); function makeRequireFunction(mod, redirects) { - const Module = mod.constructor; + // lazy due to cycle + const Module = lazyModule(); + if (mod instanceof Module !== true) { + throw new ERR_INVALID_ARG_TYPE('mod', 'Module', mod); + } let require; if (redirects) { diff --git a/test/fixtures/policy-manifest/createRequire-bypass.js b/test/fixtures/policy-manifest/createRequire-bypass.js new file mode 100644 index 00000000000000..06827ea0251d13 --- /dev/null +++ b/test/fixtures/policy-manifest/createRequire-bypass.js @@ -0,0 +1,2 @@ +const os = module.constructor.createRequire('file:///os-access-module.js')('os') +os.cpus() \ No newline at end of file diff --git a/test/fixtures/policy-manifest/invalid-module.js b/test/fixtures/policy-manifest/invalid-module.js new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/test/fixtures/policy-manifest/main-constructor-bypass.js b/test/fixtures/policy-manifest/main-constructor-bypass.js new file mode 100644 index 00000000000000..066af168b1432e --- /dev/null +++ b/test/fixtures/policy-manifest/main-constructor-bypass.js @@ -0,0 +1,2 @@ +const m = new require.main.constructor(); +m.require('./invalid-module') diff --git a/test/fixtures/policy-manifest/main-constructor-extensions-bypass.js b/test/fixtures/policy-manifest/main-constructor-extensions-bypass.js new file mode 100644 index 00000000000000..7e002dc17a7741 --- /dev/null +++ b/test/fixtures/policy-manifest/main-constructor-extensions-bypass.js @@ -0,0 +1,2 @@ +const m = new require.main.constructor(); +require.extensions['.js'](m, './invalid-module') diff --git a/test/fixtures/policy-manifest/manifest-impersonate.json b/test/fixtures/policy-manifest/manifest-impersonate.json new file mode 100644 index 00000000000000..5e15b6e9cca906 --- /dev/null +++ b/test/fixtures/policy-manifest/manifest-impersonate.json @@ -0,0 +1,13 @@ +{ + "resources": { + "./createRequire-bypass.js": { + "integrity": true + }, + "/os-access-module.js": { + "integrity": true, + "dependencies": { + "os": true + } + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy-manifest/module-constructor-bypass.js b/test/fixtures/policy-manifest/module-constructor-bypass.js new file mode 100644 index 00000000000000..8ff9e532a81f71 --- /dev/null +++ b/test/fixtures/policy-manifest/module-constructor-bypass.js @@ -0,0 +1 @@ +module.constructor._load('node:child_process'); diff --git a/test/parallel/test-policy-manifest.js b/test/parallel/test-policy-manifest.js index 5dfadb3631de66..3f5057ff4a2cd4 100644 --- a/test/parallel/test-policy-manifest.js +++ b/test/parallel/test-policy-manifest.js @@ -81,3 +81,58 @@ const fixtures = require('../common/fixtures.js'); 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', 'module-constructor-bypass.js'); + const result = spawnSync(process.execPath, [ + '--experimental-policy', + policyFilepath, + mainModuleBypass, + ]); + assert.notStrictEqual(result.status, 0); + const stderr = result.stderr.toString(); + assert.match(stderr, /TypeError/); +} + +{ + const policyFilepath = fixtures.path('policy-manifest', 'manifest-impersonate.json'); + const createRequireBypass = fixtures.path('policy-manifest', 'createRequire-bypass.js'); + const result = spawnSync(process.execPath, [ + '--experimental-policy', + policyFilepath, + createRequireBypass, + ]); + + assert.notStrictEqual(result.status, 0); + const stderr = result.stderr.toString(); + assert.match(stderr, /TypeError/); +} + +{ + const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json'); + const mainModuleBypass = fixtures.path('policy-manifest', 'main-constructor-bypass.js'); + const result = spawnSync(process.execPath, [ + '--experimental-policy', + policyFilepath, + mainModuleBypass, + ]); + + assert.notStrictEqual(result.status, 0); + const stderr = result.stderr.toString(); + assert.match(stderr, /TypeError/); +} + +{ + const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json'); + const mainModuleBypass = fixtures.path('policy-manifest', 'main-constructor-extensions-bypass.js'); + const result = spawnSync(process.execPath, [ + '--experimental-policy', + policyFilepath, + mainModuleBypass, + ]); + + assert.notStrictEqual(result.status, 0); + const stderr = result.stderr.toString(); + assert.match(stderr, /TypeError/); +}