From e404223eb74b5beaf6413db6dd8d9fd919f0ed45 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 11 Jun 2022 14:45:31 +0100 Subject: [PATCH] tools: add `avoid-prototype-pollution` lint rule PR-URL: https://github.com/nodejs/node/pull/43308 Backport-PR-URL: https://github.com/nodejs/node/pull/44081 Reviewed-By: Rich Trott --- lib/.eslintrc.yaml | 1 + lib/crypto.js | 3 + lib/internal/bootstrap/node.js | 4 + lib/internal/bootstrap/pre_execution.js | 3 + lib/internal/per_context/domexception.js | 2 + lib/internal/per_context/primordials.js | 10 +- lib/readline.js | 1 + .../test-eslint-avoid-prototype-pollution.js | 147 ++++++++++++++++++ .../eslint-rules/avoid-prototype-pollution.js | 92 +++++++++++ 9 files changed, 258 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-eslint-avoid-prototype-pollution.js create mode 100644 tools/eslint-rules/avoid-prototype-pollution.js diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 9714c9c4f64..e7b27e5cff8 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -116,6 +116,7 @@ rules: - name: SubtleCrypto message: Use `const { SubtleCrypto } = require('internal/crypto/webcrypto');` instead of the global. # Custom rules in tools/eslint-rules + node-core/avoid-prototype-pollution: error node-core/lowercase-name-for-primitive: error node-core/non-ascii-character: error node-core/no-array-destructuring: error diff --git a/lib/crypto.js b/lib/crypto.js index be1cf7a99cf..3ec03cbe731 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -297,6 +297,7 @@ ObjectDefineProperties(module.exports, { // Aliases for randomBytes are deprecated. // The ecosystem needs those to exist for backwards compatibility. prng: { + __proto__: null, enumerable: false, configurable: true, writable: true, @@ -305,6 +306,7 @@ ObjectDefineProperties(module.exports, { randomBytes }, pseudoRandomBytes: { + __proto__: null, enumerable: false, configurable: true, writable: true, @@ -314,6 +316,7 @@ ObjectDefineProperties(module.exports, { randomBytes }, rng: { + __proto__: null, enumerable: false, configurable: true, writable: true, diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 75c81c04575..67cbdb9db09 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -496,6 +496,7 @@ function createGlobalConsole(consoleFromVM) { // https://heycam.github.io/webidl/#es-namespaces function exposeNamespace(target, name, namespaceObject) { ObjectDefineProperty(target, name, { + __proto__: null, writable: true, enumerable: false, configurable: true, @@ -506,6 +507,7 @@ function exposeNamespace(target, name, namespaceObject) { // https://heycam.github.io/webidl/#es-interfaces function exposeInterface(target, name, interfaceObject) { ObjectDefineProperty(target, name, { + __proto__: null, writable: true, enumerable: false, configurable: true, @@ -516,6 +518,7 @@ function exposeInterface(target, name, interfaceObject) { // https://heycam.github.io/webidl/#define-the-operations function defineOperation(target, name, method) { ObjectDefineProperty(target, name, { + __proto__: null, writable: true, enumerable: true, configurable: true, @@ -526,6 +529,7 @@ function defineOperation(target, name, method) { // https://heycam.github.io/webidl/#Replaceable function defineReplacableAttribute(target, name, value) { ObjectDefineProperty(target, name, { + __proto__: null, writable: true, enumerable: true, configurable: true, diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 19d81549b4f..2f6af18887f 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -209,18 +209,21 @@ function setupWebCrypto() { if (internalBinding('config').hasOpenSSL) { webcrypto ??= require('internal/crypto/webcrypto'); ObjectDefineProperty(globalThis, 'Crypto', { + __proto__: null, writable: true, enumerable: false, configurable: true, value: webcrypto.Crypto }); ObjectDefineProperty(globalThis, 'CryptoKey', { + __proto__: null, writable: true, enumerable: false, configurable: true, value: webcrypto.CryptoKey }); ObjectDefineProperty(globalThis, 'SubtleCrypto', { + __proto__: null, writable: true, enumerable: false, configurable: true, diff --git a/lib/internal/per_context/domexception.js b/lib/internal/per_context/domexception.js index 6c3ed1437e5..444e055dc86 100644 --- a/lib/internal/per_context/domexception.js +++ b/lib/internal/per_context/domexception.js @@ -18,12 +18,14 @@ function throwInvalidThisError(Base, type) { const key = 'ERR_INVALID_THIS'; ObjectDefineProperties(err, { message: { + __proto__: null, value: `Value of "this" must be of ${type}`, enumerable: false, writable: true, configurable: true, }, toString: { + __proto__: null, value() { return `${this.name} [${key}]: ${this.message}`; }, diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index f26193ab050..7c6f513d9cc 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -78,7 +78,7 @@ function copyPropsRenamed(src, dest, prefix) { copyAccessor(dest, prefix, newKey, desc); } else { const name = `${prefix}${newKey}`; - ReflectDefineProperty(dest, name, desc); + ReflectDefineProperty(dest, name, { __proto__: null, ...desc }); if (varargsMethods.includes(name)) { ReflectDefineProperty(dest, `${name}Apply`, { __proto__: null, @@ -105,7 +105,7 @@ function copyPropsRenamedBound(src, dest, prefix) { } const name = `${prefix}${newKey}`; - ReflectDefineProperty(dest, name, desc); + ReflectDefineProperty(dest, name, { __proto__: null, ...desc }); if (varargsMethods.includes(name)) { ReflectDefineProperty(dest, `${name}Apply`, { __proto__: null, @@ -129,7 +129,7 @@ function copyPrototype(src, dest, prefix) { } const name = `${prefix}${newKey}`; - ReflectDefineProperty(dest, name, desc); + ReflectDefineProperty(dest, name, { __proto__: null, ...desc }); if (varargsMethods.includes(name)) { ReflectDefineProperty(dest, `${name}Apply`, { __proto__: null, @@ -313,7 +313,7 @@ const copyProps = (src, dest) => { ReflectDefineProperty( dest, key, - ReflectGetOwnPropertyDescriptor(src, key)); + { __proto__: null, ...ReflectGetOwnPropertyDescriptor(src, key) }); } }); }; @@ -341,7 +341,7 @@ const makeSafe = (unsafe, safe) => { return new SafeIterator(this); }; } - ReflectDefineProperty(safe.prototype, key, desc); + ReflectDefineProperty(safe.prototype, key, { __proto__: null, ...desc }); } }); } else { diff --git a/lib/readline.js b/lib/readline.js index 7e55c99e65b..f80f3831708 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -368,6 +368,7 @@ ObjectSetPrototypeOf(Interface.prototype, EventEmitter.prototype); ObjectSetPrototypeOf(Interface, EventEmitter); ObjectDefineProperty(Interface.prototype, 'columns', { + __proto__: null, configurable: true, enumerable: true, get: function() { diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js new file mode 100644 index 00000000000..047def545e9 --- /dev/null +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -0,0 +1,147 @@ +'use strict'; + +const common = require('../common'); +if ((!common.hasCrypto) || (!common.hasIntl)) { + common.skip('ESLint tests require crypto and Intl'); +} + +common.skipIfEslintMissing(); + +const RuleTester = require('../../tools/node_modules/eslint').RuleTester; +const rule = require('../../tools/eslint-rules/avoid-prototype-pollution'); + +new RuleTester({ + parserOptions: { ecmaVersion: 2022 }, +}) + .run('property-descriptor-no-prototype-pollution', rule, { + valid: [ + 'ObjectDefineProperties({}, {})', + 'ObjectCreate(null, {})', + 'ObjectDefineProperties({}, { key })', + 'ObjectCreate(null, { key })', + 'ObjectDefineProperties({}, { ...spread })', + 'ObjectCreate(null, { ...spread })', + 'ObjectDefineProperties({}, { key: valueDescriptor })', + 'ObjectCreate(null, { key: valueDescriptor })', + 'ObjectDefineProperties({}, { key: { ...{}, __proto__: null } })', + 'ObjectCreate(null, { key: { ...{}, __proto__: null } })', + 'ObjectDefineProperties({}, { key: { __proto__: null } })', + 'ObjectCreate(null, { key: { __proto__: null } })', + 'ObjectDefineProperties({}, { key: { __proto__: null, enumerable: true } })', + 'ObjectCreate(null, { key: { __proto__: null, enumerable: true } })', + 'ObjectDefineProperties({}, { key: { "__proto__": null } })', + 'ObjectCreate(null, { key: { "__proto__": null } })', + 'ObjectDefineProperties({}, { key: { \'__proto__\': null } })', + 'ObjectCreate(null, { key: { \'__proto__\': null } })', + 'ObjectDefineProperty({}, "key", ObjectCreate(null))', + 'ReflectDefineProperty({}, "key", ObjectCreate(null))', + 'ObjectDefineProperty({}, "key", valueDescriptor)', + 'ReflectDefineProperty({}, "key", valueDescriptor)', + 'ObjectDefineProperty({}, "key", { __proto__: null })', + 'ReflectDefineProperty({}, "key", { __proto__: null })', + 'ObjectDefineProperty({}, "key", { __proto__: null, enumerable: true })', + 'ReflectDefineProperty({}, "key", { __proto__: null, enumerable: true })', + 'ObjectDefineProperty({}, "key", { "__proto__": null })', + 'ReflectDefineProperty({}, "key", { "__proto__": null })', + 'ObjectDefineProperty({}, "key", { \'__proto__\': null })', + 'ReflectDefineProperty({}, "key", { \'__proto__\': null })', + ], + invalid: [ + { + code: 'ObjectDefineProperties({}, ObjectGetOwnPropertyDescriptors({}))', + errors: [{ message: /prototype pollution/ }], + }, + { + code: 'ObjectCreate(null, ObjectGetOwnPropertyDescriptors({}))', + errors: [{ message: /prototype pollution/ }], + }, + { + code: 'ObjectDefineProperties({}, { key: {} })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectCreate(null, { key: {} })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperties({}, { key: { [void 0]: { ...{ __proto__: null } } } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectCreate(null, { key: { [void 0]: { ...{ __proto__: null } } } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperties({}, { key: { __proto__: Object.prototype } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectCreate(null, { key: { __proto__: Object.prototype } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperties({}, { key: { [`__proto__`]: null } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectCreate(null, { key: { [`__proto__`]: null } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperties({}, { key: { enumerable: true } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectCreate(null, { key: { enumerable: true } })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperty({}, "key", {})', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ReflectDefineProperty({}, "key", {})', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperty({}, "key", ObjectGetOwnPropertyDescriptor({}, "key"))', + errors: [{ message: /prototype pollution/ }], + }, + { + code: 'ReflectDefineProperty({}, "key", ObjectGetOwnPropertyDescriptor({}, "key"))', + errors: [{ message: /prototype pollution/ }], + }, + { + code: 'ObjectDefineProperty({}, "key", ReflectGetOwnPropertyDescriptor({}, "key"))', + errors: [{ message: /prototype pollution/ }], + }, + { + code: 'ReflectDefineProperty({}, "key", ReflectGetOwnPropertyDescriptor({}, "key"))', + errors: [{ message: /prototype pollution/ }], + }, + { + code: 'ObjectDefineProperty({}, "key", { __proto__: Object.prototype })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ReflectDefineProperty({}, "key", { __proto__: Object.prototype })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperty({}, "key", { [`__proto__`]: null })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ReflectDefineProperty({}, "key", { [`__proto__`]: null })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ObjectDefineProperty({}, "key", { enumerable: true })', + errors: [{ message: /null-prototype/ }], + }, + { + code: 'ReflectDefineProperty({}, "key", { enumerable: true })', + errors: [{ message: /null-prototype/ }], + }, + ] + }); diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js new file mode 100644 index 00000000000..bf6bd1e0a81 --- /dev/null +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -0,0 +1,92 @@ +'use strict'; + +function checkProperties(context, node) { + if ( + node.type === 'CallExpression' && + node.callee.name === 'ObjectGetOwnPropertyDescriptors' + ) { + context.report({ + node, + message: + 'Property descriptors inherits from the Object prototype, therefore are subject to prototype pollution', + }); + } + 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__') + ) { + checkPropertyDescriptor(context, value); + } + } +} + +function checkPropertyDescriptor(context, node) { + if ( + node.type === 'CallExpression' && + (node.callee.name === 'ObjectGetOwnPropertyDescriptor' || + node.callee.name === 'ReflectGetOwnPropertyDescriptor') + ) { + context.report({ + node, + message: + 'Property descriptors inherits from the Object prototype, therefore are subject to prototype pollution', + suggest: [{ + desc: 'Wrap the property descriptor in a null-prototype object', + fix(fixer) { + return [ + fixer.insertTextBefore(node, '{ __proto__: null,...'), + fixer.insertTextAfter(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; + } + } + + context.report({ + node, + message: 'Must use null-prototype object for property descriptors', + }); +} + +const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]'; +module.exports = { + meta: { hasSuggestions: true }, + create(context) { + return { + [`${CallExpression}[expression.callee.name=${/^(Object|Reflect)DefinePropert(ies|y)$/}]`]( + node + ) { + switch (node.expression.callee.name) { + case 'ObjectDefineProperties': + checkProperties(context, node.expression.arguments[1]); + break; + case 'ReflectDefineProperty': + case 'ObjectDefineProperty': + checkPropertyDescriptor(context, node.expression.arguments[2]); + break; + default: + throw new Error('Unreachable'); + } + }, + + [`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) { + checkProperties(context, node.expression.arguments[1]); + }, + }; + }, +};