From c7b7f2bec20ac6260d88132baf12cc35d9a6c25a Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 20 Oct 2022 22:12:03 -0500 Subject: [PATCH] lib: add lint rule to protect against `Object.prototype.then` pollution PR-URL: https://github.com/nodejs/node/pull/45061 Reviewed-By: Jacob Smith Reviewed-By: Moshe Atlow Reviewed-By: Minwoo Jung Reviewed-By: James M Snell Reviewed-By: Yagiz Nizipli --- lib/internal/crypto/cfrg.js | 2 +- lib/internal/crypto/ec.js | 2 +- lib/internal/crypto/rsa.js | 2 +- lib/internal/fs/cp/cp.js | 2 +- lib/internal/fs/promises.js | 16 +++--- lib/internal/modules/esm/loader.js | 1 + lib/internal/modules/esm/resolve.js | 5 +- lib/internal/webstreams/readablestream.js | 6 +- .../test-eslint-avoid-prototype-pollution.js | 39 +++++++++++++ .../eslint-rules/avoid-prototype-pollution.js | 56 +++++++++++++------ 10 files changed, 98 insertions(+), 33 deletions(-) diff --git a/lib/internal/crypto/cfrg.js b/lib/internal/crypto/cfrg.js index 2b40f152b1e433..3227b692afb99e 100644 --- a/lib/internal/crypto/cfrg.js +++ b/lib/internal/crypto/cfrg.js @@ -188,7 +188,7 @@ async function cfrgGenerateKey(algorithm, extractable, keyUsages) { privateUsages, extractable); - return { privateKey, publicKey }; + return { __proto__: null, privateKey, publicKey }; } function cfrgExportKey(key, format) { diff --git a/lib/internal/crypto/ec.js b/lib/internal/crypto/ec.js index c235de5202957e..d1aa2a7b0af9ac 100644 --- a/lib/internal/crypto/ec.js +++ b/lib/internal/crypto/ec.js @@ -146,7 +146,7 @@ async function ecGenerateKey(algorithm, extractable, keyUsages) { privateUsages, extractable); - return { publicKey, privateKey }; + return { __proto__: null, publicKey, privateKey }; } function ecExportKey(key, format) { diff --git a/lib/internal/crypto/rsa.js b/lib/internal/crypto/rsa.js index eb407c7b4dcf52..83e174fb6b4a52 100644 --- a/lib/internal/crypto/rsa.js +++ b/lib/internal/crypto/rsa.js @@ -219,7 +219,7 @@ async function rsaKeyGenerate( privateUsages, extractable); - return { publicKey, privateKey }; + return { __proto__: null, publicKey, privateKey }; } function rsaExportKey(key, format) { diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index 0f49e8de4b2aee..565c80e05f3381 100644 --- a/lib/internal/fs/cp/cp.js +++ b/lib/internal/fs/cp/cp.js @@ -115,7 +115,7 @@ async function checkPaths(src, dest, opts) { code: 'EINVAL', }); } - return { srcStat, destStat, skipped: false }; + return { __proto__: null, srcStat, destStat, skipped: false }; } function areIdentical(srcStat, destStat) { diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 847995f1807d6e..f38391ac753e4d 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -580,7 +580,7 @@ async function read(handle, bufferOrParams, offset, length, position) { length |= 0; if (length === 0) - return { bytesRead: length, buffer }; + return { __proto__: null, bytesRead: length, buffer }; if (buffer.byteLength === 0) { throw new ERR_INVALID_ARG_VALUE('buffer', buffer, @@ -595,7 +595,7 @@ async function read(handle, bufferOrParams, offset, length, position) { const bytesRead = (await binding.read(handle.fd, buffer, offset, length, position, kUsePromises)) || 0; - return { bytesRead, buffer }; + return { __proto__: null, bytesRead, buffer }; } async function readv(handle, buffers, position) { @@ -606,12 +606,12 @@ async function readv(handle, buffers, position) { const bytesRead = (await binding.readBuffers(handle.fd, buffers, position, kUsePromises)) || 0; - return { bytesRead, buffers }; + return { __proto__: null, bytesRead, buffers }; } async function write(handle, buffer, offsetOrOptions, length, position) { if (buffer?.byteLength === 0) - return { bytesWritten: 0, buffer }; + return { __proto__: null, bytesWritten: 0, buffer }; let offset = offsetOrOptions; if (isArrayBufferView(buffer)) { @@ -636,14 +636,14 @@ async function write(handle, buffer, offsetOrOptions, length, position) { const bytesWritten = (await binding.writeBuffer(handle.fd, buffer, offset, length, position, kUsePromises)) || 0; - return { bytesWritten, buffer }; + return { __proto__: null, bytesWritten, buffer }; } validateStringAfterArrayBufferView(buffer, 'buffer'); validateEncoding(buffer, length); const bytesWritten = (await binding.writeString(handle.fd, buffer, offset, length, kUsePromises)) || 0; - return { bytesWritten, buffer }; + return { __proto__: null, bytesWritten, buffer }; } async function writev(handle, buffers, position) { @@ -653,12 +653,12 @@ async function writev(handle, buffers, position) { position = null; if (buffers.length === 0) { - return { bytesWritten: 0, buffers }; + return { __proto__: null, bytesWritten: 0, buffers }; } const bytesWritten = (await binding.writeBuffers(handle.fd, buffers, position, kUsePromises)) || 0; - return { bytesWritten, buffers }; + return { __proto__: null, bytesWritten, buffers }; } async function rename(oldPath, newPath) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 85c009454a70e4..c4b69442ccff6b 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -378,6 +378,7 @@ class ESMLoader { const { module } = await job.run(); return { + __proto__: null, namespace: module.getNamespace(), }; } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 2e0605fa8e7792..0b3414535ba3ee 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -977,7 +977,7 @@ async function defaultResolve(specifier, context = {}) { missing = false; } else if (destination) { const href = destination.href; - return { url: href }; + return { __proto__: null, url: href }; } if (missing) { // Prevent network requests from firing if resolution would be banned. @@ -1035,7 +1035,7 @@ async function defaultResolve(specifier, context = {}) { if (maybeReturn) return maybeReturn; // This must come after checkIfDisallowedImport - if (parsed && parsed.protocol === 'node:') return { url: specifier }; + if (parsed && parsed.protocol === 'node:') return { __proto__: null, url: specifier }; throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports); @@ -1087,6 +1087,7 @@ async function defaultResolve(specifier, context = {}) { throwIfUnsupportedURLProtocol(url); return { + __proto__: null, // Do NOT cast `url` to a string: that will work even when there are real // problems, silencing them url: url.href, diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index ade543c957dd5a..5cb4cc1e12d5f6 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -476,7 +476,7 @@ class ReadableStream { async function returnSteps(value) { if (done) - return { done: true, value }; + return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution done = true; if (reader[kState].stream === undefined) { @@ -488,11 +488,11 @@ class ReadableStream { const result = readableStreamReaderGenericCancel(reader, value); readableStreamReaderGenericRelease(reader); await result; - return { done: true, value }; + return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution } readableStreamReaderGenericRelease(reader); - return { done: true, value }; + return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution } // TODO(@jasnell): Explore whether an async generator diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index 00429c9cab14cd..0a39dd489499e6 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -45,6 +45,17 @@ new RuleTester({ 'ReflectDefineProperty({}, "key", { "__proto__": null })', 'ObjectDefineProperty({}, "key", { \'__proto__\': null })', 'ReflectDefineProperty({}, "key", { \'__proto__\': null })', + 'async function myFn() { return { __proto__: null } }', + 'async function myFn() { function myFn() { return {} } return { __proto__: null } }', + 'const myFn = async function myFn() { return { __proto__: null } }', + 'const myFn = async function () { return { __proto__: null } }', + 'const myFn = async () => { return { __proto__: null } }', + 'const myFn = async () => ({ __proto__: null })', + 'function myFn() { return {} }', + 'const myFn = function myFn() { return {} }', + 'const myFn = function () { return {} }', + 'const myFn = () => { return {} }', + 'const myFn = () => ({})', 'StringPrototypeReplace("some string", "some string", "some replacement")', 'StringPrototypeReplaceAll("some string", "some string", "some replacement")', 'StringPrototypeSplit("some string", "some string")', @@ -150,6 +161,34 @@ new RuleTester({ code: 'ReflectDefineProperty({}, "key", { enumerable: true })', errors: [{ message: /null-prototype/ }], }, + { + code: 'async function myFn(){ return {} }', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'async function myFn(){ async function someOtherFn() { return { __proto__: null } } return {} }', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'async function myFn(){ if (true) { return {} } return { __proto__: null } }', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'const myFn = async function myFn(){ return {} }', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'const myFn = async function (){ return {} }', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'const myFn = async () => { return {} }', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'const myFn = async () => ({})', + errors: [{ message: /null-prototype/ }], + }, { code: 'RegExpPrototypeTest(/some regex/, "some string")', errors: [{ message: /looks up the "exec" property/ }], diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index 7276b94ed7df12..343dbaf39d15e0 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -1,6 +1,7 @@ 'use strict'; const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`; +const AnyFunction = 'FunctionDeclaration, FunctionExpression, ArrowFunctionExpression'; function checkProperties(context, node) { if ( @@ -25,6 +26,22 @@ function checkProperties(context, node) { } } +function isNullPrototypeObjectExpression(node) { + if (node.type !== 'ObjectExpression') return; + + for (const { key, value } of node.properties) { + if ( + key != null && value != null && + ((key.type === 'Identifier' && key.name === '__proto__') || + (key.type === 'Literal' && key.value === '__proto__')) && + value.type === 'Literal' && value.value === null + ) { + return true; + } + } + return false; +} + function checkPropertyDescriptor(context, node) { if ( node.type === 'CallExpression' && @@ -46,23 +63,12 @@ function checkPropertyDescriptor(context, node) { }], }); } - if (node.type !== 'ObjectExpression') return; - - for (const { key, value } of node.properties) { - if ( - key != null && value != null && - ((key.type === 'Identifier' && key.name === '__proto__') || - (key.type === 'Literal' && key.value === '__proto__')) && - value.type === 'Literal' && value.value === null - ) { - return true; - } + if (isNullPrototypeObjectExpression(node) === false) { + context.report({ + node, + message: 'Must use null-prototype object for property descriptors', + }); } - - context.report({ - node, - message: 'Must use null-prototype object for property descriptors', - }); } function createUnsafeStringMethodReport(context, name, lookedUpProperty) { @@ -117,6 +123,24 @@ module.exports = { [`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) { checkProperties(context, node.arguments[1]); }, + + [`:matches(${AnyFunction})[async=true]>BlockStatement ReturnStatement>ObjectExpression, :matches(${AnyFunction})[async=true]>ObjectExpression`](node) { + if (node.parent.type === 'ReturnStatement') { + let { parent } = node; + do { + ({ parent } = parent); + } while (!parent.type.includes('Function')); + + if (!parent.async) return; + } + if (isNullPrototypeObjectExpression(node) === false) { + context.report({ + node, + message: 'Use null-prototype when returning from async function', + }); + } + }, + [CallExpression('RegExpPrototypeTest')](node) { context.report({ node,