Skip to content

Commit 784ed59

Browse files
aduh95juanarbol
aduh95
authored and
juanarbol
committed
repl: improve robustness wrt to prototype pollution
PR-URL: #45604 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent e0cda56 commit 784ed59

File tree

5 files changed

+142
-27
lines changed

5 files changed

+142
-27
lines changed

‎lib/internal/debugger/inspect_repl.js

+19-18
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,15 @@ const {
2828
ReflectGetOwnPropertyDescriptor,
2929
ReflectOwnKeys,
3030
RegExpPrototypeExec,
31-
RegExpPrototypeSymbolReplace,
3231
SafeMap,
33-
SafePromiseAll,
32+
SafePromiseAllReturnArrayLike,
33+
SafePromiseAllReturnVoid,
3434
String,
3535
StringFromCharCode,
3636
StringPrototypeEndsWith,
3737
StringPrototypeIncludes,
3838
StringPrototypeRepeat,
39+
StringPrototypeReplaceAll,
3940
StringPrototypeSlice,
4041
StringPrototypeSplit,
4142
StringPrototypeStartsWith,
@@ -53,7 +54,7 @@ const Repl = require('repl');
5354
const vm = require('vm');
5455
const { fileURLToPath } = require('internal/url');
5556

56-
const { customInspectSymbol } = require('internal/util');
57+
const { customInspectSymbol, SideEffectFreeRegExpPrototypeSymbolReplace } = require('internal/util');
5758
const { inspect: utilInspect } = require('internal/util/inspect');
5859
const debuglog = require('internal/util/debuglog').debuglog('inspect');
5960

@@ -121,7 +122,7 @@ const {
121122
} = internalBinding('builtins');
122123
const NATIVES = internalBinding('natives');
123124
function isNativeUrl(url) {
124-
url = RegExpPrototypeSymbolReplace(/\.js$/, url, '');
125+
url = SideEffectFreeRegExpPrototypeSymbolReplace(/\.js$/, url, '');
125126

126127
return StringPrototypeStartsWith(url, 'node:internal/') ||
127128
ArrayPrototypeIncludes(PUBLIC_BUILTINS, url) ||
@@ -159,8 +160,8 @@ function markSourceColumn(sourceText, position, useColors) {
159160

160161
// Colourize char if stdout supports colours
161162
if (useColors) {
162-
tail = RegExpPrototypeSymbolReplace(/(.+?)([^\w]|$)/, tail,
163-
'\u001b[32m$1\u001b[39m$2');
163+
tail = SideEffectFreeRegExpPrototypeSymbolReplace(/(.+?)([^\w]|$)/, tail,
164+
'\u001b[32m$1\u001b[39m$2');
164165
}
165166

166167
// Return source line with coloured char at `position`
@@ -340,9 +341,9 @@ class ScopeSnapshot {
340341
StringPrototypeSlice(this.type, 1);
341342
const name = this.name ? `<${this.name}>` : '';
342343
const prefix = `${type}${name} `;
343-
return RegExpPrototypeSymbolReplace(/^Map /,
344-
utilInspect(this.properties, opts),
345-
prefix);
344+
return SideEffectFreeRegExpPrototypeSymbolReplace(/^Map /,
345+
utilInspect(this.properties, opts),
346+
prefix);
346347
}
347348
}
348349

@@ -517,7 +518,7 @@ function createRepl(inspector) {
517518
}
518519

519520
loadScopes() {
520-
return SafePromiseAll(
521+
return SafePromiseAllReturnArrayLike(
521522
ArrayPrototypeFilter(
522523
this.scopeChain,
523524
(scope) => scope.type !== 'global'
@@ -656,14 +657,14 @@ function createRepl(inspector) {
656657
(error) => `<${error.message}>`);
657658
const lastIndex = watchedExpressions.length - 1;
658659

659-
const values = await SafePromiseAll(watchedExpressions, inspectValue);
660+
const values = await SafePromiseAllReturnArrayLike(watchedExpressions, inspectValue);
660661
const lines = ArrayPrototypeMap(watchedExpressions, (expr, idx) => {
661662
const prefix = `${leftPad(idx, ' ', lastIndex)}: ${expr} =`;
662663
const value = inspect(values[idx]);
663664
if (!StringPrototypeIncludes(value, '\n')) {
664665
return `${prefix} ${value}`;
665666
}
666-
return `${prefix}\n ${RegExpPrototypeSymbolReplace(/\n/g, value, '\n ')}`;
667+
return `${prefix}\n ${StringPrototypeReplaceAll(value, '\n', '\n ')}`;
667668
});
668669
const valueList = ArrayPrototypeJoin(lines, '\n');
669670
return verbose ? `Watchers:\n${valueList}\n` : valueList;
@@ -805,8 +806,8 @@ function createRepl(inspector) {
805806
registerBreakpoint);
806807
}
807808

808-
const escapedPath = RegExpPrototypeSymbolReplace(/([/\\.?*()^${}|[\]])/g,
809-
script, '\\$1');
809+
const escapedPath = SideEffectFreeRegExpPrototypeSymbolReplace(/([/\\.?*()^${}|[\]])/g,
810+
script, '\\$1');
810811
const urlRegex = `^(.*[\\/\\\\])?${escapedPath}$`;
811812

812813
return PromisePrototypeThen(
@@ -860,9 +861,9 @@ function createRepl(inspector) {
860861
location.lineNumber + 1));
861862
if (!newBreakpoints.length) return PromiseResolve();
862863
return PromisePrototypeThen(
863-
SafePromiseAll(newBreakpoints),
864-
(results) => {
865-
print(`${results.length} breakpoints restored.`);
864+
SafePromiseAllReturnVoid(newBreakpoints),
865+
() => {
866+
print(`${newBreakpoints.length} breakpoints restored.`);
866867
});
867868
}
868869

@@ -896,7 +897,7 @@ function createRepl(inspector) {
896897

897898
inspector.suspendReplWhile(() =>
898899
PromisePrototypeThen(
899-
SafePromiseAll([formatWatchers(true), selectedFrame.list(2)]),
900+
SafePromiseAllReturnArrayLike([formatWatchers(true), selectedFrame.list(2)]),
900901
({ 0: watcherList, 1: context }) => {
901902
const breakContext = watcherList ?
902903
`${watcherList}\n${inspect(context)}` :

‎lib/internal/util.js

+42
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,24 @@ const {
2323
ReflectApply,
2424
ReflectConstruct,
2525
RegExpPrototypeExec,
26+
RegExpPrototypeGetDotAll,
27+
RegExpPrototypeGetGlobal,
28+
RegExpPrototypeGetHasIndices,
29+
RegExpPrototypeGetIgnoreCase,
30+
RegExpPrototypeGetMultiline,
31+
RegExpPrototypeGetSticky,
32+
RegExpPrototypeGetUnicode,
33+
RegExpPrototypeGetSource,
2634
SafeMap,
2735
SafeSet,
36+
SafeWeakMap,
2837
StringPrototypeReplace,
2938
StringPrototypeToLowerCase,
3039
StringPrototypeToUpperCase,
3140
Symbol,
3241
SymbolFor,
42+
SymbolReplace,
43+
SymbolSplit,
3344
} = primordials;
3445

3546
const {
@@ -559,6 +570,35 @@ function SideEffectFreeRegExpPrototypeExec(regex, string) {
559570
return FunctionPrototypeCall(RegExpFromAnotherRealm.prototype.exec, regex, string);
560571
}
561572

573+
const crossRelmRegexes = new SafeWeakMap();
574+
function getCrossRelmRegex(regex) {
575+
const cached = crossRelmRegexes.get(regex);
576+
if (cached) return cached;
577+
578+
let flagString = '';
579+
if (RegExpPrototypeGetHasIndices(regex)) flagString += 'd';
580+
if (RegExpPrototypeGetGlobal(regex)) flagString += 'g';
581+
if (RegExpPrototypeGetIgnoreCase(regex)) flagString += 'i';
582+
if (RegExpPrototypeGetMultiline(regex)) flagString += 'm';
583+
if (RegExpPrototypeGetDotAll(regex)) flagString += 's';
584+
if (RegExpPrototypeGetUnicode(regex)) flagString += 'u';
585+
if (RegExpPrototypeGetSticky(regex)) flagString += 'y';
586+
587+
const { RegExp: RegExpFromAnotherRealm } = getInternalGlobal();
588+
const crossRelmRegex = new RegExpFromAnotherRealm(RegExpPrototypeGetSource(regex), flagString);
589+
crossRelmRegexes.set(regex, crossRelmRegex);
590+
return crossRelmRegex;
591+
}
592+
593+
function SideEffectFreeRegExpPrototypeSymbolReplace(regex, string, replacement) {
594+
return getCrossRelmRegex(regex)[SymbolReplace](string, replacement);
595+
}
596+
597+
function SideEffectFreeRegExpPrototypeSymbolSplit(regex, string, limit = undefined) {
598+
return getCrossRelmRegex(regex)[SymbolSplit](string, limit);
599+
}
600+
601+
562602
function isArrayBufferDetached(value) {
563603
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
564604
return _isArrayBufferDetached(value);
@@ -594,6 +634,8 @@ module.exports = {
594634
once,
595635
promisify,
596636
SideEffectFreeRegExpPrototypeExec,
637+
SideEffectFreeRegExpPrototypeSymbolReplace,
638+
SideEffectFreeRegExpPrototypeSymbolSplit,
597639
sleep,
598640
spliceOne,
599641
toUSVString,

‎lib/repl.js

+9-9
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,6 @@ const {
7777
ReflectApply,
7878
RegExp,
7979
RegExpPrototypeExec,
80-
RegExpPrototypeSymbolReplace,
81-
RegExpPrototypeSymbolSplit,
8280
SafePromiseRace,
8381
SafeSet,
8482
SafeWeakSet,
@@ -111,7 +109,9 @@ const {
111109
const {
112110
decorateErrorStack,
113111
isError,
114-
deprecate
112+
deprecate,
113+
SideEffectFreeRegExpPrototypeSymbolReplace,
114+
SideEffectFreeRegExpPrototypeSymbolSplit,
115115
} = require('internal/util');
116116
const { inspect } = require('internal/util/inspect');
117117
const vm = require('vm');
@@ -456,7 +456,7 @@ function REPLServer(prompt,
456456

457457
// Remove all "await"s and attempt running the script
458458
// in order to detect if error is truly non recoverable
459-
const fallbackCode = RegExpPrototypeSymbolReplace(/\bawait\b/g, code, '');
459+
const fallbackCode = SideEffectFreeRegExpPrototypeSymbolReplace(/\bawait\b/g, code, '');
460460
try {
461461
vm.createScript(fallbackCode, {
462462
filename: file,
@@ -681,22 +681,22 @@ function REPLServer(prompt,
681681
if (e.stack) {
682682
if (e.name === 'SyntaxError') {
683683
// Remove stack trace.
684-
e.stack = RegExpPrototypeSymbolReplace(
684+
e.stack = SideEffectFreeRegExpPrototypeSymbolReplace(
685685
/^\s+at\s.*\n?/gm,
686-
RegExpPrototypeSymbolReplace(/^REPL\d+:\d+\r?\n/, e.stack, ''),
686+
SideEffectFreeRegExpPrototypeSymbolReplace(/^REPL\d+:\d+\r?\n/, e.stack, ''),
687687
'');
688688
const importErrorStr = 'Cannot use import statement outside a ' +
689689
'module';
690690
if (StringPrototypeIncludes(e.message, importErrorStr)) {
691691
e.message = 'Cannot use import statement inside the Node.js ' +
692692
'REPL, alternatively use dynamic import';
693-
e.stack = RegExpPrototypeSymbolReplace(
693+
e.stack = SideEffectFreeRegExpPrototypeSymbolReplace(
694694
/SyntaxError:.*\n/,
695695
e.stack,
696696
`SyntaxError: ${e.message}\n`);
697697
}
698698
} else if (self.replMode === module.exports.REPL_MODE_STRICT) {
699-
e.stack = RegExpPrototypeSymbolReplace(
699+
e.stack = SideEffectFreeRegExpPrototypeSymbolReplace(
700700
/(\s+at\s+REPL\d+:)(\d+)/,
701701
e.stack,
702702
(_, pre, line) => pre + (line - 1)
@@ -728,7 +728,7 @@ function REPLServer(prompt,
728728
if (errStack === '') {
729729
errStack = self.writer(e);
730730
}
731-
const lines = RegExpPrototypeSymbolSplit(/(?<=\n)/, errStack);
731+
const lines = SideEffectFreeRegExpPrototypeSymbolSplit(/(?<=\n)/, errStack);
732732
let matched = false;
733733

734734
errStack = '';

‎test/parallel/test-primordials-regexp.js

+55
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,20 @@ const { mustNotCall } = require('../common');
55
const assert = require('assert');
66

77
const {
8+
RegExpPrototypeExec,
89
RegExpPrototypeSymbolReplace,
910
RegExpPrototypeSymbolSearch,
1011
RegExpPrototypeSymbolSplit,
1112
SafeStringPrototypeSearch,
1213
hardenRegExp,
1314
} = require('internal/test/binding').primordials;
1415

16+
const {
17+
SideEffectFreeRegExpPrototypeExec,
18+
SideEffectFreeRegExpPrototypeSymbolReplace,
19+
SideEffectFreeRegExpPrototypeSymbolSplit,
20+
} = require('internal/util');
21+
1522

1623
Object.defineProperties(RegExp.prototype, {
1724
[Symbol.match]: {
@@ -89,14 +96,46 @@ hardenRegExp(hardenRegExp(/1/));
8996
// IMO there are no valid use cases in node core to use RegExpPrototypeSymbolMatch
9097
// or RegExpPrototypeSymbolMatchAll, they are inherently unsafe.
9198

99+
assert.strictEqual(RegExpPrototypeExec(/foo/, 'bar'), null);
100+
assert.strictEqual(RegExpPrototypeExec(hardenRegExp(/foo/), 'bar'), null);
101+
assert.strictEqual(SideEffectFreeRegExpPrototypeExec(/foo/, 'bar'), null);
102+
assert.strictEqual(SideEffectFreeRegExpPrototypeExec(hardenRegExp(/foo/), 'bar'), null);
103+
{
104+
const expected = ['bar'];
105+
Object.defineProperties(expected, {
106+
index: { __proto__: null, configurable: true, writable: true, enumerable: true, value: 0 },
107+
input: { __proto__: null, configurable: true, writable: true, enumerable: true, value: 'bar' },
108+
groups: { __proto__: null, configurable: true, writable: true, enumerable: true },
109+
});
110+
const actual = SideEffectFreeRegExpPrototypeExec(/bar/, 'bar');
111+
112+
// assert.deepStrictEqual(actual, expected) doesn't work for cross-realm comparison.
113+
114+
assert.strictEqual(Array.isArray(actual), Array.isArray(expected));
115+
assert.deepStrictEqual(Reflect.ownKeys(actual), Reflect.ownKeys(expected));
116+
for (const key of Reflect.ownKeys(expected)) {
117+
assert.deepStrictEqual(
118+
Reflect.getOwnPropertyDescriptor(actual, key),
119+
Reflect.getOwnPropertyDescriptor(expected, key),
120+
);
121+
}
122+
}
92123
{
93124
const myRegex = hardenRegExp(/a/);
94125
assert.strictEqual(RegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'bear');
95126
}
127+
{
128+
const myRegex = /a/;
129+
assert.strictEqual(SideEffectFreeRegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'bear');
130+
}
96131
{
97132
const myRegex = hardenRegExp(/a/g);
98133
assert.strictEqual(RegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'beer');
99134
}
135+
{
136+
const myRegex = /a/g;
137+
assert.strictEqual(SideEffectFreeRegExpPrototypeSymbolReplace(myRegex, 'baar', 'e'), 'beer');
138+
}
100139
{
101140
const myRegex = hardenRegExp(/a/);
102141
assert.strictEqual(RegExpPrototypeSymbolSearch(myRegex, 'baar'), 1);
@@ -109,6 +148,22 @@ hardenRegExp(hardenRegExp(/1/));
109148
const myRegex = hardenRegExp(/a/);
110149
assert.deepStrictEqual(RegExpPrototypeSymbolSplit(myRegex, 'baar', 0), []);
111150
}
151+
{
152+
const myRegex = /a/;
153+
const expected = [];
154+
const actual = SideEffectFreeRegExpPrototypeSymbolSplit(myRegex, 'baar', 0);
155+
156+
// assert.deepStrictEqual(actual, expected) doesn't work for cross-realm comparison.
157+
158+
assert.strictEqual(Array.isArray(actual), Array.isArray(expected));
159+
assert.deepStrictEqual(Reflect.ownKeys(actual), Reflect.ownKeys(expected));
160+
for (const key of Reflect.ownKeys(expected)) {
161+
assert.deepStrictEqual(
162+
Reflect.getOwnPropertyDescriptor(actual, key),
163+
Reflect.getOwnPropertyDescriptor(expected, key),
164+
);
165+
}
166+
}
112167
{
113168
const myRegex = hardenRegExp(/a/);
114169
assert.deepStrictEqual(RegExpPrototypeSymbolSplit(myRegex, 'baar', 1), ['b']);

‎test/parallel/test-startup-empty-regexp-statics.js

+17
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,23 @@ const allRegExpStatics =
4040
assert.strictEqual(child.signal, null);
4141
}
4242

43+
{
44+
const child = spawnSync(process.execPath,
45+
[ '--expose-internals', '-p', `const {
46+
SideEffectFreeRegExpPrototypeExec,
47+
SideEffectFreeRegExpPrototypeSymbolReplace,
48+
SideEffectFreeRegExpPrototypeSymbolSplit,
49+
} = require("internal/util");
50+
SideEffectFreeRegExpPrototypeExec(/foo/, "foo");
51+
SideEffectFreeRegExpPrototypeSymbolReplace(/o/, "foo", "a");
52+
SideEffectFreeRegExpPrototypeSymbolSplit(/o/, "foo");
53+
${allRegExpStatics}` ],
54+
{ stdio: ['inherit', 'pipe', 'inherit'] });
55+
assert.match(child.stdout.toString(), /^undefined\r?\n$/);
56+
assert.strictEqual(child.status, 0);
57+
assert.strictEqual(child.signal, null);
58+
}
59+
4360
{
4461
const child = spawnSync(process.execPath,
4562
[ '-e', `console.log(${allRegExpStatics})`, '--input-type=module' ],

0 commit comments

Comments
 (0)