From 784ed594eacde6f10e3b63893526f4a6026c7781 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 14 Dec 2022 15:48:50 +0100 Subject: [PATCH] repl: improve robustness wrt to prototype pollution PR-URL: https://github.com/nodejs/node/pull/45604 Reviewed-By: James M Snell --- lib/internal/debugger/inspect_repl.js | 37 +++++++------ lib/internal/util.js | 42 ++++++++++++++ lib/repl.js | 18 +++--- test/parallel/test-primordials-regexp.js | 55 +++++++++++++++++++ .../test-startup-empty-regexp-statics.js | 17 ++++++ 5 files changed, 142 insertions(+), 27 deletions(-) diff --git a/lib/internal/debugger/inspect_repl.js b/lib/internal/debugger/inspect_repl.js index 965aa64b0fbcc6..bd6a59ae282ba3 100644 --- a/lib/internal/debugger/inspect_repl.js +++ b/lib/internal/debugger/inspect_repl.js @@ -28,14 +28,15 @@ const { ReflectGetOwnPropertyDescriptor, ReflectOwnKeys, RegExpPrototypeExec, - RegExpPrototypeSymbolReplace, SafeMap, - SafePromiseAll, + SafePromiseAllReturnArrayLike, + SafePromiseAllReturnVoid, String, StringFromCharCode, StringPrototypeEndsWith, StringPrototypeIncludes, StringPrototypeRepeat, + StringPrototypeReplaceAll, StringPrototypeSlice, StringPrototypeSplit, StringPrototypeStartsWith, @@ -53,7 +54,7 @@ const Repl = require('repl'); const vm = require('vm'); const { fileURLToPath } = require('internal/url'); -const { customInspectSymbol } = require('internal/util'); +const { customInspectSymbol, SideEffectFreeRegExpPrototypeSymbolReplace } = require('internal/util'); const { inspect: utilInspect } = require('internal/util/inspect'); const debuglog = require('internal/util/debuglog').debuglog('inspect'); @@ -121,7 +122,7 @@ const { } = internalBinding('builtins'); const NATIVES = internalBinding('natives'); function isNativeUrl(url) { - url = RegExpPrototypeSymbolReplace(/\.js$/, url, ''); + url = SideEffectFreeRegExpPrototypeSymbolReplace(/\.js$/, url, ''); return StringPrototypeStartsWith(url, 'node:internal/') || ArrayPrototypeIncludes(PUBLIC_BUILTINS, url) || @@ -159,8 +160,8 @@ function markSourceColumn(sourceText, position, useColors) { // Colourize char if stdout supports colours if (useColors) { - tail = RegExpPrototypeSymbolReplace(/(.+?)([^\w]|$)/, tail, - '\u001b[32m$1\u001b[39m$2'); + tail = SideEffectFreeRegExpPrototypeSymbolReplace(/(.+?)([^\w]|$)/, tail, + '\u001b[32m$1\u001b[39m$2'); } // Return source line with coloured char at `position` @@ -340,9 +341,9 @@ class ScopeSnapshot { StringPrototypeSlice(this.type, 1); const name = this.name ? `<${this.name}>` : ''; const prefix = `${type}${name} `; - return RegExpPrototypeSymbolReplace(/^Map /, - utilInspect(this.properties, opts), - prefix); + return SideEffectFreeRegExpPrototypeSymbolReplace(/^Map /, + utilInspect(this.properties, opts), + prefix); } } @@ -517,7 +518,7 @@ function createRepl(inspector) { } loadScopes() { - return SafePromiseAll( + return SafePromiseAllReturnArrayLike( ArrayPrototypeFilter( this.scopeChain, (scope) => scope.type !== 'global' @@ -656,14 +657,14 @@ function createRepl(inspector) { (error) => `<${error.message}>`); const lastIndex = watchedExpressions.length - 1; - const values = await SafePromiseAll(watchedExpressions, inspectValue); + const values = await SafePromiseAllReturnArrayLike(watchedExpressions, inspectValue); const lines = ArrayPrototypeMap(watchedExpressions, (expr, idx) => { const prefix = `${leftPad(idx, ' ', lastIndex)}: ${expr} =`; const value = inspect(values[idx]); if (!StringPrototypeIncludes(value, '\n')) { return `${prefix} ${value}`; } - return `${prefix}\n ${RegExpPrototypeSymbolReplace(/\n/g, value, '\n ')}`; + return `${prefix}\n ${StringPrototypeReplaceAll(value, '\n', '\n ')}`; }); const valueList = ArrayPrototypeJoin(lines, '\n'); return verbose ? `Watchers:\n${valueList}\n` : valueList; @@ -805,8 +806,8 @@ function createRepl(inspector) { registerBreakpoint); } - const escapedPath = RegExpPrototypeSymbolReplace(/([/\\.?*()^${}|[\]])/g, - script, '\\$1'); + const escapedPath = SideEffectFreeRegExpPrototypeSymbolReplace(/([/\\.?*()^${}|[\]])/g, + script, '\\$1'); const urlRegex = `^(.*[\\/\\\\])?${escapedPath}$`; return PromisePrototypeThen( @@ -860,9 +861,9 @@ function createRepl(inspector) { location.lineNumber + 1)); if (!newBreakpoints.length) return PromiseResolve(); return PromisePrototypeThen( - SafePromiseAll(newBreakpoints), - (results) => { - print(`${results.length} breakpoints restored.`); + SafePromiseAllReturnVoid(newBreakpoints), + () => { + print(`${newBreakpoints.length} breakpoints restored.`); }); } @@ -896,7 +897,7 @@ function createRepl(inspector) { inspector.suspendReplWhile(() => PromisePrototypeThen( - SafePromiseAll([formatWatchers(true), selectedFrame.list(2)]), + SafePromiseAllReturnArrayLike([formatWatchers(true), selectedFrame.list(2)]), ({ 0: watcherList, 1: context }) => { const breakContext = watcherList ? `${watcherList}\n${inspect(context)}` : diff --git a/lib/internal/util.js b/lib/internal/util.js index b6fab1187960ec..05f1066b106091 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -23,13 +23,24 @@ const { ReflectApply, ReflectConstruct, RegExpPrototypeExec, + RegExpPrototypeGetDotAll, + RegExpPrototypeGetGlobal, + RegExpPrototypeGetHasIndices, + RegExpPrototypeGetIgnoreCase, + RegExpPrototypeGetMultiline, + RegExpPrototypeGetSticky, + RegExpPrototypeGetUnicode, + RegExpPrototypeGetSource, SafeMap, SafeSet, + SafeWeakMap, StringPrototypeReplace, StringPrototypeToLowerCase, StringPrototypeToUpperCase, Symbol, SymbolFor, + SymbolReplace, + SymbolSplit, } = primordials; const { @@ -559,6 +570,35 @@ function SideEffectFreeRegExpPrototypeExec(regex, string) { return FunctionPrototypeCall(RegExpFromAnotherRealm.prototype.exec, regex, string); } +const crossRelmRegexes = new SafeWeakMap(); +function getCrossRelmRegex(regex) { + const cached = crossRelmRegexes.get(regex); + if (cached) return cached; + + let flagString = ''; + if (RegExpPrototypeGetHasIndices(regex)) flagString += 'd'; + if (RegExpPrototypeGetGlobal(regex)) flagString += 'g'; + if (RegExpPrototypeGetIgnoreCase(regex)) flagString += 'i'; + if (RegExpPrototypeGetMultiline(regex)) flagString += 'm'; + if (RegExpPrototypeGetDotAll(regex)) flagString += 's'; + if (RegExpPrototypeGetUnicode(regex)) flagString += 'u'; + if (RegExpPrototypeGetSticky(regex)) flagString += 'y'; + + const { RegExp: RegExpFromAnotherRealm } = getInternalGlobal(); + const crossRelmRegex = new RegExpFromAnotherRealm(RegExpPrototypeGetSource(regex), flagString); + crossRelmRegexes.set(regex, crossRelmRegex); + return crossRelmRegex; +} + +function SideEffectFreeRegExpPrototypeSymbolReplace(regex, string, replacement) { + return getCrossRelmRegex(regex)[SymbolReplace](string, replacement); +} + +function SideEffectFreeRegExpPrototypeSymbolSplit(regex, string, limit = undefined) { + return getCrossRelmRegex(regex)[SymbolSplit](string, limit); +} + + function isArrayBufferDetached(value) { if (ArrayBufferPrototypeGetByteLength(value) === 0) { return _isArrayBufferDetached(value); @@ -594,6 +634,8 @@ module.exports = { once, promisify, SideEffectFreeRegExpPrototypeExec, + SideEffectFreeRegExpPrototypeSymbolReplace, + SideEffectFreeRegExpPrototypeSymbolSplit, sleep, spliceOne, toUSVString, diff --git a/lib/repl.js b/lib/repl.js index 124d4781498cc4..9aaf0c07f54579 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -77,8 +77,6 @@ const { ReflectApply, RegExp, RegExpPrototypeExec, - RegExpPrototypeSymbolReplace, - RegExpPrototypeSymbolSplit, SafePromiseRace, SafeSet, SafeWeakSet, @@ -111,7 +109,9 @@ const { const { decorateErrorStack, isError, - deprecate + deprecate, + SideEffectFreeRegExpPrototypeSymbolReplace, + SideEffectFreeRegExpPrototypeSymbolSplit, } = require('internal/util'); const { inspect } = require('internal/util/inspect'); const vm = require('vm'); @@ -456,7 +456,7 @@ function REPLServer(prompt, // Remove all "await"s and attempt running the script // in order to detect if error is truly non recoverable - const fallbackCode = RegExpPrototypeSymbolReplace(/\bawait\b/g, code, ''); + const fallbackCode = SideEffectFreeRegExpPrototypeSymbolReplace(/\bawait\b/g, code, ''); try { vm.createScript(fallbackCode, { filename: file, @@ -681,22 +681,22 @@ function REPLServer(prompt, if (e.stack) { if (e.name === 'SyntaxError') { // Remove stack trace. - e.stack = RegExpPrototypeSymbolReplace( + e.stack = SideEffectFreeRegExpPrototypeSymbolReplace( /^\s+at\s.*\n?/gm, - RegExpPrototypeSymbolReplace(/^REPL\d+:\d+\r?\n/, e.stack, ''), + SideEffectFreeRegExpPrototypeSymbolReplace(/^REPL\d+:\d+\r?\n/, e.stack, ''), ''); const importErrorStr = 'Cannot use import statement outside a ' + 'module'; if (StringPrototypeIncludes(e.message, importErrorStr)) { e.message = 'Cannot use import statement inside the Node.js ' + 'REPL, alternatively use dynamic import'; - e.stack = RegExpPrototypeSymbolReplace( + e.stack = SideEffectFreeRegExpPrototypeSymbolReplace( /SyntaxError:.*\n/, e.stack, `SyntaxError: ${e.message}\n`); } } else if (self.replMode === module.exports.REPL_MODE_STRICT) { - e.stack = RegExpPrototypeSymbolReplace( + e.stack = SideEffectFreeRegExpPrototypeSymbolReplace( /(\s+at\s+REPL\d+:)(\d+)/, e.stack, (_, pre, line) => pre + (line - 1) @@ -728,7 +728,7 @@ function REPLServer(prompt, if (errStack === '') { errStack = self.writer(e); } - const lines = RegExpPrototypeSymbolSplit(/(?<=\n)/, errStack); + const lines = SideEffectFreeRegExpPrototypeSymbolSplit(/(?<=\n)/, errStack); let matched = false; errStack = ''; diff --git a/test/parallel/test-primordials-regexp.js b/test/parallel/test-primordials-regexp.js index 825ebbca549180..61e733708870d2 100644 --- a/test/parallel/test-primordials-regexp.js +++ b/test/parallel/test-primordials-regexp.js @@ -5,6 +5,7 @@ const { mustNotCall } = require('../common'); const assert = require('assert'); const { + RegExpPrototypeExec, RegExpPrototypeSymbolReplace, RegExpPrototypeSymbolSearch, RegExpPrototypeSymbolSplit, @@ -12,6 +13,12 @@ const { hardenRegExp, } = require('internal/test/binding').primordials; +const { + SideEffectFreeRegExpPrototypeExec, + SideEffectFreeRegExpPrototypeSymbolReplace, + SideEffectFreeRegExpPrototypeSymbolSplit, +} = require('internal/util'); + Object.defineProperties(RegExp.prototype, { [Symbol.match]: { @@ -89,14 +96,46 @@ hardenRegExp(hardenRegExp(/1/)); // IMO there are no valid use cases in node core to use RegExpPrototypeSymbolMatch // or RegExpPrototypeSymbolMatchAll, they are inherently unsafe. +assert.strictEqual(RegExpPrototypeExec(/foo/, 'bar'), null); +assert.strictEqual(RegExpPrototypeExec(hardenRegExp(/foo/), 'bar'), null); +assert.strictEqual(SideEffectFreeRegExpPrototypeExec(/foo/, 'bar'), null); +assert.strictEqual(SideEffectFreeRegExpPrototypeExec(hardenRegExp(/foo/), 'bar'), null); +{ + const expected = ['bar']; + Object.defineProperties(expected, { + index: { __proto__: null, configurable: true, writable: true, enumerable: true, value: 0 }, + input: { __proto__: null, configurable: true, writable: true, enumerable: true, value: 'bar' }, + groups: { __proto__: null, configurable: true, writable: true, enumerable: true }, + }); + const actual = SideEffectFreeRegExpPrototypeExec(/bar/, 'bar'); + + // assert.deepStrictEqual(actual, expected) doesn't work for cross-realm comparison. + + assert.strictEqual(Array.isArray(actual), Array.isArray(expected)); + assert.deepStrictEqual(Reflect.ownKeys(actual), Reflect.ownKeys(expected)); + for (const key of Reflect.ownKeys(expected)) { + assert.deepStrictEqual( + Reflect.getOwnPropertyDescriptor(actual, key), + Reflect.getOwnPropertyDescriptor(expected, key), + ); + } +} { const myRegex = hardenRegExp(/a/); assert.strictEqual(RegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'bear'); } +{ + const myRegex = /a/; + assert.strictEqual(SideEffectFreeRegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'bear'); +} { const myRegex = hardenRegExp(/a/g); assert.strictEqual(RegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'beer'); } +{ + const myRegex = /a/g; + assert.strictEqual(SideEffectFreeRegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'beer'); +} { const myRegex = hardenRegExp(/a/); assert.strictEqual(RegExpPrototypeSymbolSearch(myRegex, 'baar'), 1); @@ -109,6 +148,22 @@ hardenRegExp(hardenRegExp(/1/)); const myRegex = hardenRegExp(/a/); assert.deepStrictEqual(RegExpPrototypeSymbolSplit(myRegex, 'baar', 0), []); } +{ + const myRegex = /a/; + const expected = []; + const actual = SideEffectFreeRegExpPrototypeSymbolSplit(myRegex, 'baar', 0); + + // assert.deepStrictEqual(actual, expected) doesn't work for cross-realm comparison. + + assert.strictEqual(Array.isArray(actual), Array.isArray(expected)); + assert.deepStrictEqual(Reflect.ownKeys(actual), Reflect.ownKeys(expected)); + for (const key of Reflect.ownKeys(expected)) { + assert.deepStrictEqual( + Reflect.getOwnPropertyDescriptor(actual, key), + Reflect.getOwnPropertyDescriptor(expected, key), + ); + } +} { const myRegex = hardenRegExp(/a/); assert.deepStrictEqual(RegExpPrototypeSymbolSplit(myRegex, 'baar', 1), ['b']); diff --git a/test/parallel/test-startup-empty-regexp-statics.js b/test/parallel/test-startup-empty-regexp-statics.js index 80f6bef5a5a08e..5744bbc14bba21 100644 --- a/test/parallel/test-startup-empty-regexp-statics.js +++ b/test/parallel/test-startup-empty-regexp-statics.js @@ -40,6 +40,23 @@ const allRegExpStatics = assert.strictEqual(child.signal, null); } +{ + const child = spawnSync(process.execPath, + [ '--expose-internals', '-p', `const { + SideEffectFreeRegExpPrototypeExec, + SideEffectFreeRegExpPrototypeSymbolReplace, + SideEffectFreeRegExpPrototypeSymbolSplit, + } = require("internal/util"); + SideEffectFreeRegExpPrototypeExec(/foo/, "foo"); + SideEffectFreeRegExpPrototypeSymbolReplace(/o/, "foo", "a"); + SideEffectFreeRegExpPrototypeSymbolSplit(/o/, "foo"); + ${allRegExpStatics}` ], + { stdio: ['inherit', 'pipe', 'inherit'] }); + assert.match(child.stdout.toString(), /^undefined\r?\n$/); + assert.strictEqual(child.status, 0); + assert.strictEqual(child.signal, null); +} + { const child = spawnSync(process.execPath, [ '-e', `console.log(${allRegExpStatics})`, '--input-type=module' ],