From ed1717215c3827ff770faf5d4dd9dae29d83240b Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 22 Mar 2024 17:41:02 +0800 Subject: [PATCH] vm: harden module type checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check if the value returned from user linker function is a null-ish value. `validateInternalField` should be preferred when checking `this` argument to guard against null-ish `this`. Co-authored-by: Mike Ralphson PR-URL: https://github.com/nodejs/node/pull/52162 Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Yagiz Nizipli # Conflicts: # lib/internal/vm/module.js --- lib/internal/vm/module.js | 67 +++++++++-------------- test/parallel/test-vm-module-basic.js | 16 +++--- test/parallel/test-vm-module-errors.js | 8 +-- test/parallel/test-vm-module-link.js | 17 ++++++ test/parallel/test-vm-module-synthetic.js | 8 +-- 5 files changed, 59 insertions(+), 57 deletions(-) diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index 5db3d9535f3974..cd4f3a65f8466c 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -7,7 +7,9 @@ const { ArrayPrototypeIndexOf, ArrayPrototypeSome, ObjectDefineProperty, + ObjectFreeze, ObjectGetPrototypeOf, + ObjectPrototypeHasOwnProperty, ObjectSetPrototypeOf, ReflectApply, SafePromiseAllReturnVoid, @@ -43,6 +45,7 @@ const { validateObject, validateUint32, validateString, + validateInternalField, } = require('internal/validators'); const binding = internalBinding('module_wrap'); @@ -75,6 +78,13 @@ const kLink = Symbol('kLink'); const { isContext } = require('internal/vm'); +function isModule(object) { + if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, kWrap)) { + return false; + } + return true; +} + class Module { constructor(options) { emitExperimentalWarning('VM Modules'); @@ -148,23 +158,17 @@ class Module { } get identifier() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); return this[kWrap].url; } get context() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); return this[kContext]; } get namespace() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); if (this[kWrap].getStatus() < kInstantiated) { throw new ERR_VM_MODULE_STATUS('must not be unlinked or linking'); } @@ -172,16 +176,12 @@ class Module { } get status() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); return STATUS_MAP[this[kWrap].getStatus()]; } get error() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); if (this[kWrap].getStatus() !== kErrored) { throw new ERR_VM_MODULE_STATUS('must be errored'); } @@ -189,9 +189,7 @@ class Module { } async link(linker) { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); validateFunction(linker, 'linker'); if (this.status === 'linked') { throw new ERR_VM_MODULE_ALREADY_LINKED(); @@ -204,10 +202,7 @@ class Module { } async evaluate(options = kEmptyObject) { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } - + validateInternalField(this, kWrap, 'Module'); validateObject(options, 'options'); let timeout = options.timeout; @@ -230,9 +225,7 @@ class Module { } [customInspectSymbol](depth, options) { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'Module'); if (typeof depth === 'number' && depth < 0) return this; @@ -307,7 +300,7 @@ class SourceTextModule extends Module { const promises = this[kWrap].link(async (identifier, attributes) => { const module = await linker(identifier, this, { attributes, assert: attributes }); - if (module[kWrap] === undefined) { + if (!isModule(module)) { throw new ERR_VM_MODULE_NOT_MODULE(); } if (module.context !== this.context) { @@ -338,19 +331,13 @@ class SourceTextModule extends Module { } get dependencySpecifiers() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } - if (this[kDependencySpecifiers] === undefined) { - this[kDependencySpecifiers] = this[kWrap].getStaticDependencySpecifiers(); - } + validateInternalField(this, kDependencySpecifiers, 'SourceTextModule'); + this[kDependencySpecifiers] ??= ObjectFreeze(this[kWrap].getStaticDependencySpecifiers()); return this[kDependencySpecifiers]; } get status() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kDependencySpecifiers, 'SourceTextModule'); if (this.#error !== kNoError) { return 'errored'; } @@ -361,9 +348,7 @@ class SourceTextModule extends Module { } get error() { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kDependencySpecifiers, 'SourceTextModule'); if (this.#error !== kNoError) { return this.#error; } @@ -416,9 +401,7 @@ class SyntheticModule extends Module { } setExport(name, value) { - if (this[kWrap] === undefined) { - throw new ERR_VM_MODULE_NOT_MODULE(); - } + validateInternalField(this, kWrap, 'SyntheticModule'); validateString(name, 'name'); if (this[kWrap].getStatus() < kInstantiated) { throw new ERR_VM_MODULE_STATUS('must be linked'); @@ -433,7 +416,7 @@ function importModuleDynamicallyWrap(importModuleDynamically) { if (isModuleNamespaceObject(m)) { return m; } - if (!m || m[kWrap] === undefined) { + if (!isModule(m)) { throw new ERR_VM_MODULE_NOT_MODULE(); } if (m.status === 'errored') { diff --git a/test/parallel/test-vm-module-basic.js b/test/parallel/test-vm-module-basic.js index b563fd6de2bbd8..cba1e037ac455a 100644 --- a/test/parallel/test-vm-module-basic.js +++ b/test/parallel/test-vm-module-basic.js @@ -84,13 +84,15 @@ const util = require('util'); assert.strictEqual(util.inspect(m, { depth: -1 }), '[SourceTextModule]'); - assert.throws( - () => m[util.inspect.custom].call({ __proto__: null }), - { - code: 'ERR_VM_MODULE_NOT_MODULE', - message: 'Provided module is not an instance of Module' - }, - ); + for (const value of [null, { __proto__: null }, SourceTextModule.prototype]) { + assert.throws( + () => m[util.inspect.custom].call(value), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "this" argument must be an instance of Module/, + }, + ); + } } { diff --git a/test/parallel/test-vm-module-errors.js b/test/parallel/test-vm-module-errors.js index bec8258a4145c7..ad247f83edcbfe 100644 --- a/test/parallel/test-vm-module-errors.js +++ b/test/parallel/test-vm-module-errors.js @@ -216,8 +216,8 @@ async function checkInvalidOptionForEvaluate() { await assert.rejects(async () => { await Module.prototype[method](); }, { - code: 'ERR_VM_MODULE_NOT_MODULE', - message: /Provided module is not an instance of Module/ + code: 'ERR_INVALID_ARG_TYPE', + message: /The "this" argument must be an instance of Module/ }); }); } @@ -241,8 +241,8 @@ function checkInvalidCachedData() { function checkGettersErrors() { const expectedError = { - code: 'ERR_VM_MODULE_NOT_MODULE', - message: /Provided module is not an instance of Module/ + code: 'ERR_INVALID_ARG_TYPE', + message: /The "this" argument must be an instance of (?:Module|SourceTextModule)/, }; const getters = ['identifier', 'context', 'namespace', 'status', 'error']; getters.forEach((getter) => { diff --git a/test/parallel/test-vm-module-link.js b/test/parallel/test-vm-module-link.js index 6b19a4d4916868..26dcb69885ba3c 100644 --- a/test/parallel/test-vm-module-link.js +++ b/test/parallel/test-vm-module-link.js @@ -28,6 +28,22 @@ async function simple() { delete globalThis.fiveResult; } +async function invalidLinkValue() { + const invalidValues = [ + undefined, + null, + {}, + SourceTextModule.prototype, + ]; + + for (const value of invalidValues) { + const module = new SourceTextModule('import "foo"'); + await assert.rejects(module.link(() => value), { + code: 'ERR_VM_MODULE_NOT_MODULE', + }); + } +} + async function depth() { const foo = new SourceTextModule('export default 5'); await foo.link(common.mustNotCall()); @@ -143,6 +159,7 @@ const finished = common.mustCall(); (async function main() { await simple(); + await invalidLinkValue(); await depth(); await circular(); await circular2(); diff --git a/test/parallel/test-vm-module-synthetic.js b/test/parallel/test-vm-module-synthetic.js index 9d1c07ead5c4cd..48e4ed6b13240a 100644 --- a/test/parallel/test-vm-module-synthetic.js +++ b/test/parallel/test-vm-module-synthetic.js @@ -66,12 +66,12 @@ const assert = require('assert'); }); } - { + for (const value of [null, {}, SyntheticModule.prototype]) { assert.throws(() => { - SyntheticModule.prototype.setExport.call({}, 'foo'); + SyntheticModule.prototype.setExport.call(value, 'foo'); }, { - code: 'ERR_VM_MODULE_NOT_MODULE', - message: /Provided module is not an instance of Module/ + code: 'ERR_INVALID_ARG_TYPE', + message: /The "this" argument must be an instance of SyntheticModule/ }); }