Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

repl: improve robustness wrt to prototype pollution #45604

Merged
merged 1 commit into from
Dec 14, 2022
Merged

repl: improve robustness wrt to prototype pollution #45604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions lib/internal/debugger/inspect_repl.js
View file
Open in desktop
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
Open in desktop
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 @@ -646,6 +657,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 @@ -684,6 +724,8 @@ module.exports = {
once,
promisify,
SideEffectFreeRegExpPrototypeExec,
SideEffectFreeRegExpPrototypeSymbolReplace,
SideEffectFreeRegExpPrototypeSymbolSplit,
sleep,
spliceOne,
toUSVString,
Expand Down
18 changes: 9 additions & 9 deletions lib/repl.js
View file
Open in desktop
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 @@ -455,7 +455,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 @@ -680,22 +680,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 @@ -727,7 +727,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
Open in desktop
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
Open in desktop
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