Skip to content

Commit

Permalink
repl: improve robustness wrt to prototype pollution
Browse files Browse the repository at this point in the history 
PR-URL: #45604
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@aduh95 @juanarbol
aduh95 authored and juanarbol committed Jan 24, 2023
1 parent e0cda56 commit 784ed59
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 27 deletions.
37 changes: 19 additions & 18 deletions lib/internal/debugger/inspect_repl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ const {
ReflectGetOwnPropertyDescriptor,
ReflectOwnKeys,
RegExpPrototypeExec,
RegExpPrototypeSymbolReplace,
SafeMap,
SafePromiseAll,
SafePromiseAllReturnArrayLike,
SafePromiseAllReturnVoid,
String,
StringFromCharCode,
StringPrototypeEndsWith,
StringPrototypeIncludes,
StringPrototypeRepeat,
StringPrototypeReplaceAll,
StringPrototypeSlice,
StringPrototypeSplit,
StringPrototypeStartsWith,
Expand All @@ -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');

Expand Down Expand Up @@ -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) ||
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -517,7 +518,7 @@ function createRepl(inspector) {
}

loadScopes() {
return SafePromiseAll(
return SafePromiseAllReturnArrayLike(
ArrayPrototypeFilter(
this.scopeChain,
(scope) => scope.type !== 'global'
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.`);
});
}

Expand Down Expand Up @@ -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)}` :
Expand Down
42 changes: 42 additions & 0 deletions lib/internal/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -594,6 +634,8 @@ module.exports = {
once,
promisify,
SideEffectFreeRegExpPrototypeExec,
SideEffectFreeRegExpPrototypeSymbolReplace,
SideEffectFreeRegExpPrototypeSymbolSplit,
sleep,
spliceOne,
toUSVString,
Expand Down
18 changes: 9 additions & 9 deletions lib/repl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ const {
ReflectApply,
RegExp,
RegExpPrototypeExec,
RegExpPrototypeSymbolReplace,
RegExpPrototypeSymbolSplit,
SafePromiseRace,
SafeSet,
SafeWeakSet,
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 = '';
Expand Down
55 changes: 55 additions & 0 deletions test/parallel/test-primordials-regexp.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,20 @@ const { mustNotCall } = require('../common');
const assert = require('assert');

const {
RegExpPrototypeExec,
RegExpPrototypeSymbolReplace,
RegExpPrototypeSymbolSearch,
RegExpPrototypeSymbolSplit,
SafeStringPrototypeSearch,
hardenRegExp,
} = require('internal/test/binding').primordials;

const {
SideEffectFreeRegExpPrototypeExec,
SideEffectFreeRegExpPrototypeSymbolReplace,
SideEffectFreeRegExpPrototypeSymbolSplit,
} = require('internal/util');


Object.defineProperties(RegExp.prototype, {
[Symbol.match]: {
Expand Down Expand Up @@ -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);
Expand All @@ -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']);
Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-startup-empty-regexp-statics.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' ],
Expand Down

0 comments on commit 784ed59

Please sign in to comment.