From 816d449ecde0e62d409279ef86c307949d9a2de7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 8 Oct 2022 20:24:02 +0200 Subject: [PATCH 1/3] readline: refactor to avoid unsafe regex primordials Refs: https://github.com/nodejs/node/pull/43475 --- lib/readline.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/readline.js b/lib/readline.js index f80f383170814b..e54c2defe14aca 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -48,12 +48,12 @@ const { NumberIsNaN, ObjectDefineProperty, ObjectSetPrototypeOf, - RegExpPrototypeTest, + RegExpPrototypeExec, + RegExpPrototypeSymbolReplace, + RegExpPrototypeSymbolSplit, StringPrototypeCodePointAt, StringPrototypeEndsWith, - StringPrototypeMatch, StringPrototypeRepeat, - StringPrototypeReplace, StringPrototypeSlice, StringPrototypeSplit, StringPrototypeStartsWith, @@ -642,12 +642,12 @@ Interface.prototype._normalWrite = function(b) { let string = this._decoder.write(b); if (this._sawReturnAt && DateNow() - this._sawReturnAt <= this.crlfDelay) { - string = StringPrototypeReplace(string, /^\n/, ''); + string = RegExpPrototypeSymbolReplace(/^\n/, string, ''); this._sawReturnAt = 0; } // Run test() on the new string chunk, not on the entire line buffer. - const newPartContainsEnding = RegExpPrototypeTest(lineEnding, string); + const newPartContainsEnding = RegExpPrototypeExec(lineEnding, string) !== null; if (this._line_buffer) { string = this._line_buffer + string; @@ -773,7 +773,7 @@ Interface.prototype._wordLeft = function() { const leading = StringPrototypeSlice(this.line, 0, this.cursor); const reversed = ArrayPrototypeJoin( ArrayPrototypeReverse(ArrayFrom(leading)), ''); - const match = StringPrototypeMatch(reversed, /^\s*(?:[^\w\s]+|\w+)?/); + const match = RegExpPrototypeExec(/^\s*(?:[^\w\s]+|\w+)?/, reversed); this._moveCursor(-match[0].length); } }; @@ -782,7 +782,7 @@ Interface.prototype._wordLeft = function() { Interface.prototype._wordRight = function() { if (this.cursor < this.line.length) { const trailing = StringPrototypeSlice(this.line, this.cursor); - const match = StringPrototypeMatch(trailing, /^(?:\s+|[^\w\s]+|\w+)\s*/); + const match = RegExpPrototypeExec(/^(?:\s+|[^\w\s]+|\w+)\s*/, trailing); this._moveCursor(match[0].length); } }; @@ -818,7 +818,7 @@ Interface.prototype._deleteWordLeft = function() { let leading = StringPrototypeSlice(this.line, 0, this.cursor); const reversed = ArrayPrototypeJoin( ArrayPrototypeReverse(ArrayFrom(leading)), ''); - const match = StringPrototypeMatch(reversed, /^\s*(?:[^\w\s]+|\w+)?/); + const match = RegExpPrototypeExec(/^\s*(?:[^\w\s]+|\w+)?/, reversed); leading = StringPrototypeSlice(leading, 0, leading.length - match[0].length); this.line = leading + StringPrototypeSlice(this.line, this.cursor, @@ -832,7 +832,7 @@ Interface.prototype._deleteWordLeft = function() { Interface.prototype._deleteWordRight = function() { if (this.cursor < this.line.length) { const trailing = StringPrototypeSlice(this.line, this.cursor); - const match = StringPrototypeMatch(trailing, /^(?:\s+|\W+|\w+)\s*/); + const match = RegExpPrototypeExec(/^(?:\s+|\W+|\w+)\s*/, trailing); this.line = StringPrototypeSlice(this.line, 0, this.cursor) + StringPrototypeSlice(trailing, match[0].length); this._refreshLine(); @@ -1272,7 +1272,7 @@ Interface.prototype._ttyWrite = function(s, key) { // falls through default: if (typeof s === 'string' && s) { - const lines = StringPrototypeSplit(s, /\r\n|\n|\r/); + const lines = RegExpPrototypeSymbolSplit(/\r\n|\n|\r/, s); for (let i = 0, len = lines.length; i < len; i++) { if (i > 0) { this._line(); From a15e589988c02b2f432a5df43ac74f9dd8aad5d5 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 17 Jun 2022 19:12:42 +0200 Subject: [PATCH 2/3] tools: refactor `avoid-prototype-pollution` lint rule The lint rule was not catching all occurences of unsafe primordials use, and was too strict on some methods. PR-URL: https://github.com/nodejs/node/pull/43476 Reviewed-By: Moshe Atlow --- lib/internal/net.js | 6 ++ .../test-eslint-avoid-prototype-pollution.js | 23 +++++++ .../eslint-rules/avoid-prototype-pollution.js | 65 ++++++++++++------- 3 files changed, 71 insertions(+), 23 deletions(-) diff --git a/lib/internal/net.js b/lib/internal/net.js index 8ae3170228dc32..625377acd57caa 100644 --- a/lib/internal/net.js +++ b/lib/internal/net.js @@ -29,10 +29,16 @@ const IPv6Reg = new RegExp('^(' + ')(%[0-9a-zA-Z-.:]{1,})?$'); function isIPv4(s) { + // TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it + // no longer creates a perf regression in the dns benchmark. + // eslint-disable-next-line node-core/avoid-prototype-pollution return RegExpPrototypeTest(IPv4Reg, s); } function isIPv6(s) { + // TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it + // no longer creates a perf regression in the dns benchmark. + // eslint-disable-next-line node-core/avoid-prototype-pollution return RegExpPrototypeTest(IPv6Reg, s); } diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index f10d6ea973b347..c30928f56ce91a 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -45,6 +45,9 @@ new RuleTester({ 'ReflectDefineProperty({}, "key", { "__proto__": null })', 'ObjectDefineProperty({}, "key", { \'__proto__\': null })', 'ReflectDefineProperty({}, "key", { \'__proto__\': null })', + 'StringPrototypeReplace("some string", "some string", "some replacement")', + 'StringPrototypeReplaceAll("some string", "some string", "some replacement")', + 'StringPrototypeSplit("some string", "some string")', 'new Proxy({}, otherObject)', 'new Proxy({}, someFactory())', 'new Proxy({}, { __proto__: null })', @@ -167,18 +170,38 @@ new RuleTester({ code: 'StringPrototypeMatch("some string", /some regex/)', errors: [{ message: /looks up the Symbol\.match property/ }], }, + { + code: 'let v = StringPrototypeMatch("some string", /some regex/)', + errors: [{ message: /looks up the Symbol\.match property/ }], + }, + { + code: 'let v = StringPrototypeMatch("some string", new RegExp("some regex"))', + errors: [{ message: /looks up the Symbol\.match property/ }], + }, { code: 'StringPrototypeMatchAll("some string", /some regex/)', errors: [{ message: /looks up the Symbol\.matchAll property/ }], }, + { + code: 'let v = StringPrototypeMatchAll("some string", new RegExp("some regex"))', + errors: [{ message: /looks up the Symbol\.matchAll property/ }], + }, { code: 'StringPrototypeReplace("some string", /some regex/, "some replacement")', errors: [{ message: /looks up the Symbol\.replace property/ }], }, + { + code: 'StringPrototypeReplace("some string", new RegExp("some regex"), "some replacement")', + errors: [{ message: /looks up the Symbol\.replace property/ }], + }, { code: 'StringPrototypeReplaceAll("some string", /some regex/, "some replacement")', errors: [{ message: /looks up the Symbol\.replace property/ }], }, + { + code: 'StringPrototypeReplaceAll("some string", new RegExp("some regex"), "some replacement")', + errors: [{ message: /looks up the Symbol\.replace property/ }], + }, { code: 'StringPrototypeSearch("some string", /some regex/)', errors: [{ message: /looks up the Symbol\.search property/ }], diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index d59b62f95028cc..d26a410b1d9f67 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -1,5 +1,7 @@ 'use strict'; +const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`; + function checkProperties(context, node) { if ( node.type === 'CallExpression' && @@ -64,8 +66,10 @@ function checkPropertyDescriptor(context, node) { } function createUnsafeStringMethodReport(context, name, lookedUpProperty) { + const lastDotPosition = '$String.prototype.'.length; + const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`; return { - [`${CallExpression}[expression.callee.name=${JSON.stringify(name)}]`](node) { + [CallExpression(unsafePrimordialName)](node) { context.report({ node, message: `${name} looks up the ${lookedUpProperty} property on the first argument`, @@ -74,31 +78,46 @@ function createUnsafeStringMethodReport(context, name, lookedUpProperty) { }; } -const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]'; +function createUnsafeStringMethodOnRegexReport(context, name, lookedUpProperty) { + const dotPosition = 'Symbol.'.length; + const safePrimordialName = `RegExpPrototypeSymbol${lookedUpProperty.charAt(dotPosition).toUpperCase()}${lookedUpProperty.slice(dotPosition + 1)}`; + const lastDotPosition = '$String.prototype.'.length; + const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`; + return { + [[ + `${CallExpression(unsafePrimordialName)}[arguments.1.type=Literal][arguments.1.regex]`, + `${CallExpression(unsafePrimordialName)}[arguments.1.type=NewExpression][arguments.1.callee.name=RegExp]`, + ].join(',')](node) { + context.report({ + node, + message: `${name} looks up the ${lookedUpProperty} property of the passed regex, use ${safePrimordialName} directly`, + }); + } + }; +} + module.exports = { meta: { hasSuggestions: true }, create(context) { return { - [`${CallExpression}[expression.callee.name=${/^(Object|Reflect)DefinePropert(ies|y)$/}]`]( - node - ) { - switch (node.expression.callee.name) { + [CallExpression(/^(Object|Reflect)DefinePropert(ies|y)$/)](node) { + switch (node.callee.name) { case 'ObjectDefineProperties': - checkProperties(context, node.expression.arguments[1]); + checkProperties(context, node.arguments[1]); break; case 'ReflectDefineProperty': case 'ObjectDefineProperty': - checkPropertyDescriptor(context, node.expression.arguments[2]); + checkPropertyDescriptor(context, node.arguments[2]); break; default: throw new Error('Unreachable'); } }, - [`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) { - checkProperties(context, node.expression.arguments[1]); + [`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) { + checkProperties(context, node.arguments[1]); }, - [`${CallExpression}[expression.callee.name="RegExpPrototypeTest"]`](node) { + [CallExpression('RegExpPrototypeTest')](node) { context.report({ node, message: '%RegExp.prototype.test% looks up the "exec" property of `this` value', @@ -116,18 +135,18 @@ module.exports = { }], }); }, - [`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) { + [CallExpression(/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/)](node) { context.report({ node, - message: node.expression.callee.name + ' looks up the "exec" property of `this` value', + message: node.callee.name + ' looks up the "exec" property of `this` value', }); }, - ...createUnsafeStringMethodReport(context, 'StringPrototypeMatch', 'Symbol.match'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeMatchAll', 'Symbol.matchAll'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeReplace', 'Symbol.replace'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeReplaceAll', 'Symbol.replace'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeSearch', 'Symbol.search'), - ...createUnsafeStringMethodReport(context, 'StringPrototypeSplit', 'Symbol.split'), + ...createUnsafeStringMethodReport(context, '%String.prototype.match%', 'Symbol.match'), + ...createUnsafeStringMethodReport(context, '%String.prototype.matchAll%', 'Symbol.matchAll'), + ...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replace%', 'Symbol.replace'), + ...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replaceAll%', 'Symbol.replace'), + ...createUnsafeStringMethodReport(context, '%String.prototype.search%', 'Symbol.search'), + ...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.split%', 'Symbol.split'), 'NewExpression[callee.name="Proxy"][arguments.1.type="ObjectExpression"]'(node) { for (const { key, value } of node.arguments[1].properties) { @@ -146,7 +165,7 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=PromisePrototypeCatch]`](node) { + [CallExpression('PromisePrototypeCatch')](node) { context.report({ node, message: '%Promise.prototype.catch% look up the `then` property of ' + @@ -154,7 +173,7 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=PromisePrototypeFinally]`](node) { + [CallExpression('PromisePrototypeFinally')](node) { context.report({ node, message: '%Promise.prototype.finally% look up the `then` property of ' + @@ -163,10 +182,10 @@ module.exports = { }); }, - [`${CallExpression}[expression.callee.name=${/^Promise(All(Settled)?|Any|Race)/}]`](node) { + [CallExpression(/^Promise(All(Settled)?|Any|Race)/)](node) { context.report({ node, - message: `Use Safe${node.expression.callee.name} instead of ${node.expression.callee.name}`, + message: `Use Safe${node.callee.name} instead of ${node.callee.name}`, }); }, }; From f8364c5393adcac83e885abe75f3c3aa6d0cbdc7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 30 Aug 2022 14:18:29 +0200 Subject: [PATCH 3/3] tools: fix typo in `avoid-prototype-pollution` lint rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/44446 Reviewed-By: Moshe Atlow Reviewed-By: Tobias Nießen Reviewed-By: Mohammed Keyvanzadeh --- tools/eslint-rules/avoid-prototype-pollution.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index d26a410b1d9f67..f7e7fbe7da0287 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -168,7 +168,7 @@ module.exports = { [CallExpression('PromisePrototypeCatch')](node) { context.report({ node, - message: '%Promise.prototype.catch% look up the `then` property of ' + + message: '%Promise.prototype.catch% looks up the `then` property of ' + 'the `this` argument, use PromisePrototypeThen instead', }); }, @@ -176,7 +176,7 @@ module.exports = { [CallExpression('PromisePrototypeFinally')](node) { context.report({ node, - message: '%Promise.prototype.finally% look up the `then` property of ' + + message: '%Promise.prototype.finally% looks up the `then` property of ' + 'the `this` argument, use SafePromisePrototypeFinally or ' + 'try/finally instead', });