Skip to content

Commit 760701c

Browse files
eilonc-pillarcsuermann
eilonc-pillar
authored and
csuermann
committed
Merge commit from fork
1 parent c465a49 commit 760701c

File tree

5 files changed

+485
-10
lines changed

5 files changed

+485
-10
lines changed

‎packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1500,6 +1500,137 @@ describe('JsTaskRunner', () => {
15001500
});
15011501
});
15021502

1503+
describe('Error.prepareStackTrace RCE prevention', () => {
1504+
test.each([['runOnceForAllItems'], ['runOnceForEachItem']] as const)(
1505+
'should block Error.prepareStackTrace access in %s mode',
1506+
async (mode) => {
1507+
const execute = mode === 'runOnceForAllItems' ? executeForAllItems : executeForEachItem;
1508+
const outcome = await execute({
1509+
code:
1510+
mode === 'runOnceForAllItems'
1511+
? 'return [{ json: { result: Error.prepareStackTrace } }]'
1512+
: 'return { json: { result: Error.prepareStackTrace } }',
1513+
inputItems: [{ a: 1 }],
1514+
});
1515+
expect(outcome.result[0].json.result).toBeUndefined();
1516+
},
1517+
);
1518+
1519+
test.each([['runOnceForAllItems'], ['runOnceForEachItem']] as const)(
1520+
'should block Error.captureStackTrace access in %s mode',
1521+
async (mode) => {
1522+
const execute = mode === 'runOnceForAllItems' ? executeForAllItems : executeForEachItem;
1523+
const outcome = await execute({
1524+
code:
1525+
mode === 'runOnceForAllItems'
1526+
? 'return [{ json: { result: Error.captureStackTrace } }]'
1527+
: 'return { json: { result: Error.captureStackTrace } }',
1528+
inputItems: [{ a: 1 }],
1529+
});
1530+
expect(outcome.result[0].json.result).toBeUndefined();
1531+
},
1532+
);
1533+
1534+
test.each([['runOnceForAllItems'], ['runOnceForEachItem']] as const)(
1535+
'should prevent Object.defineProperty from setting Error.prepareStackTrace in %s mode',
1536+
async (mode) => {
1537+
const execute = mode === 'runOnceForAllItems' ? executeForAllItems : executeForEachItem;
1538+
// First try to define prepareStackTrace, then check if it's still undefined
1539+
const outcome = await execute({
1540+
code:
1541+
mode === 'runOnceForAllItems'
1542+
? `
1543+
Object.defineProperty(Error, 'prepareStackTrace', { value: () => 'pwned' });
1544+
return [{ json: { result: Error.prepareStackTrace } }];
1545+
`
1546+
: `
1547+
Object.defineProperty(Error, 'prepareStackTrace', { value: () => 'pwned' });
1548+
return { json: { result: Error.prepareStackTrace } };
1549+
`,
1550+
inputItems: [{ a: 1 }],
1551+
});
1552+
// prepareStackTrace should still be undefined because defineProperty is neutered
1553+
expect(outcome.result[0].json.result).toBeUndefined();
1554+
},
1555+
);
1556+
1557+
test('should prevent full RCE exploit chain via prepareStackTrace', async () => {
1558+
// This is the actual exploit payload - should NOT execute system commands
1559+
// Even though the code runs without throwing, the exploit should not work
1560+
// because Object.defineProperty is neutered and prepareStackTrace remains undefined
1561+
const outcome = await executeForAllItems({
1562+
code: `
1563+
Object.defineProperty(Error, 'prepareStackTrace', {
1564+
value: (e, stack) => {
1565+
const g = stack[0].getThis();
1566+
return g.global.process.getBuiltinModule('child_process').execSync('echo pwned').toString();
1567+
}
1568+
});
1569+
const result = new Error().stack;
1570+
// Check if the result contains 'pwned' - if so, exploit succeeded
1571+
const exploited = typeof result === 'string' && result.includes('pwned');
1572+
return [{ json: { exploited, result: typeof result } }];
1573+
`,
1574+
inputItems: [{ a: 1 }],
1575+
});
1576+
// The exploit should fail - prepareStackTrace wasn't actually set
1577+
// so .stack returns a normal stack trace string, not 'pwned'
1578+
expect(outcome.result[0].json.exploited).toBe(false);
1579+
expect(outcome.result[0].json.result).toBe('string'); // Normal stack trace
1580+
});
1581+
});
1582+
1583+
describe('Object.defineProperty security', () => {
1584+
test.each([['runOnceForAllItems'], ['runOnceForEachItem']] as const)(
1585+
'should neuter Object.defineProperty in %s mode',
1586+
async (mode) => {
1587+
const execute = mode === 'runOnceForAllItems' ? executeForAllItems : executeForEachItem;
1588+
const outcome = await execute({
1589+
code:
1590+
mode === 'runOnceForAllItems'
1591+
? `
1592+
const obj = {};
1593+
Object.defineProperty(obj, 'test', { value: 123 });
1594+
return [{ json: { hasProperty: 'test' in obj, value: obj.test } }];
1595+
`
1596+
: `
1597+
const obj = {};
1598+
Object.defineProperty(obj, 'test', { value: 123 });
1599+
return { json: { hasProperty: 'test' in obj, value: obj.test } };
1600+
`,
1601+
inputItems: [{ a: 1 }],
1602+
});
1603+
// defineProperty is neutered, property won't be defined
1604+
expect(outcome.result[0].json.hasProperty).toBe(false);
1605+
expect(outcome.result[0].json.value).toBeUndefined();
1606+
},
1607+
);
1608+
1609+
test.each([['runOnceForAllItems'], ['runOnceForEachItem']] as const)(
1610+
'should neuter Object.defineProperties in %s mode',
1611+
async (mode) => {
1612+
const execute = mode === 'runOnceForAllItems' ? executeForAllItems : executeForEachItem;
1613+
const outcome = await execute({
1614+
code:
1615+
mode === 'runOnceForAllItems'
1616+
? `
1617+
const obj = {};
1618+
Object.defineProperties(obj, { a: { value: 1 }, b: { value: 2 } });
1619+
return [{ json: { hasA: 'a' in obj, hasB: 'b' in obj } }];
1620+
`
1621+
: `
1622+
const obj = {};
1623+
Object.defineProperties(obj, { a: { value: 1 }, b: { value: 2 } });
1624+
return { json: { hasA: 'a' in obj, hasB: 'b' in obj } };
1625+
`,
1626+
inputItems: [{ a: 1 }],
1627+
});
1628+
expect(outcome.result[0].json.hasA).toBe(false);
1629+
expect(outcome.result[0].json.hasB).toBe(false);
1630+
},
1631+
);
1632+
});
1633+
15031634
describe('stack trace', () => {
15041635
it('should extract correct line number from user-defined function stack trace', async () => {
15051636
const runner = createRunnerWithOpts({});

‎packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,18 @@ export class JsTaskRunner extends TaskRunner {
601601
'Object.setPrototypeOf = () => false',
602602
'Reflect.setPrototypeOf = () => false',
603603

604+
// prevent Error.prepareStackTrace RCE attack BEFORE disabling defineProperty
605+
// This V8 API allows accessing the real global object via stack frame's getThis()
606+
'delete Error.prepareStackTrace',
607+
'delete Error.captureStackTrace',
608+
'Object.defineProperty(Error, "prepareStackTrace", { configurable: false, writable: false, value: undefined })',
609+
'Object.defineProperty(Error, "captureStackTrace", { configurable: false, writable: false, value: undefined })',
610+
611+
// prevent defineProperty attacks (used to bypass sandbox via Error.prepareStackTrace)
612+
// Must come AFTER we've locked down Error properties above
613+
'Object.defineProperty = () => ({})',
614+
'Object.defineProperties = () => ({})',
615+
604616
// wrap user code
605617
`module.exports = async function VmCodeWrapper() {${code}\n}()`,
606618
].join('; ');

‎packages/workflow/src/expression.ts‎

Lines changed: 135 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,130 @@ setErrorHandler((error: Error) => {
5050
if (isExpressionError(error)) throw error;
5151
});
5252

53+
/**
54+
* Creates a safe Object wrapper that removes dangerous static methods
55+
* that could be used to bypass property access sanitization.
56+
*
57+
* Blocked methods:
58+
* - defineProperty/defineProperties: Can set properties bypassing access checks
59+
* - setPrototypeOf/getPrototypeOf: Can manipulate prototype chains
60+
* - getOwnPropertyDescriptor(s): Can introspect sensitive properties
61+
* - __defineGetter__/__defineSetter__: Legacy methods that can bypass set traps
62+
* - __lookupGetter__/__lookupSetter__: Can introspect getters/setters
63+
*
64+
* Object.create is wrapped to prevent passing property descriptors (2nd argument)
65+
*/
66+
const createSafeObject = (): typeof Object => {
67+
const safeCreate = (proto: object | null): object => {
68+
// Only allow single-argument create (no property descriptors)
69+
return Object.create(proto);
70+
};
71+
72+
// Block dangerous static and prototype methods
73+
const blockedMethods = new Set([
74+
// Static methods that can bypass property access checks
75+
'defineProperty',
76+
'defineProperties',
77+
'setPrototypeOf',
78+
'getPrototypeOf',
79+
'getOwnPropertyDescriptor',
80+
'getOwnPropertyDescriptors',
81+
// Legacy methods that can bypass Proxy set traps
82+
'__defineGetter__',
83+
'__defineSetter__',
84+
'__lookupGetter__',
85+
'__lookupSetter__',
86+
]);
87+
88+
// Create a proxy that blocks dangerous methods
89+
return new Proxy(Object, {
90+
get(target, prop, receiver) {
91+
if (blockedMethods.has(prop as string)) {
92+
return undefined;
93+
}
94+
95+
// Wrap Object.create to prevent property descriptor argument
96+
if (prop === 'create') {
97+
return safeCreate;
98+
}
99+
100+
return Reflect.get(target, prop, receiver);
101+
},
102+
// Block defineProperty trap to prevent __defineGetter__ from working
103+
defineProperty() {
104+
return false;
105+
},
106+
});
107+
};
108+
109+
/**
110+
* List of properties that are blocked on Error and all Error subclasses.
111+
* These properties can be exploited for sandbox escape via V8's stack trace API.
112+
*/
113+
const blockedErrorProperties = new Set([
114+
// V8 stack trace manipulation
115+
'captureStackTrace',
116+
'prepareStackTrace',
117+
'stackTraceLimit',
118+
// Legacy methods that can bypass Proxy set traps
119+
'__defineGetter__',
120+
'__defineSetter__',
121+
'__lookupGetter__',
122+
'__lookupSetter__',
123+
]);
124+
125+
/**
126+
* Creates a safe Error constructor that removes dangerous static methods
127+
* like captureStackTrace and prepareStackTrace which can be exploited for RCE.
128+
*
129+
* The V8 prepareStackTrace attack works by:
130+
* 1. Setting Error.prepareStackTrace to a malicious function
131+
* 2. Creating a new Error and accessing its .stack property
132+
* 3. V8 calls the prepareStackTrace function with CallSite objects
133+
* 4. CallSite.getThis() returns the real global object, escaping the sandbox
134+
*/
135+
const createSafeError = (): typeof Error => {
136+
return new Proxy(Error, {
137+
get(target, prop, receiver) {
138+
if (blockedErrorProperties.has(prop as string)) {
139+
return undefined;
140+
}
141+
142+
return Reflect.get(target, prop, receiver);
143+
},
144+
set() {
145+
// Prevent setting any properties on Error (like prepareStackTrace)
146+
return false;
147+
},
148+
defineProperty() {
149+
// Prevent defineProperty (blocks __defineGetter__ internally)
150+
return false;
151+
},
152+
});
153+
};
154+
155+
/**
156+
* Creates a safe wrapper for Error subclasses (TypeError, SyntaxError, etc.)
157+
* While prepareStackTrace is only on Error in V8, we wrap subclasses for defense in depth.
158+
*/
159+
const createSafeErrorSubclass = <T extends ErrorConstructor>(ErrorClass: T): T => {
160+
return new Proxy(ErrorClass, {
161+
get(target, prop, receiver) {
162+
if (blockedErrorProperties.has(prop as string)) {
163+
return undefined;
164+
}
165+
166+
return Reflect.get(target, prop, receiver);
167+
},
168+
set() {
169+
return false;
170+
},
171+
defineProperty() {
172+
return false;
173+
},
174+
});
175+
};
176+
53177
export class Expression {
54178
constructor(private readonly workflow: Workflow) {}
55179

@@ -111,8 +235,8 @@ export class Expression {
111235
data.Interval = Interval;
112236
data.Duration = Duration;
113237

114-
// Objects
115-
data.Object = Object;
238+
// Objects - use safe wrapper to block dangerous methods like defineProperty
239+
data.Object = createSafeObject();
116240

117241
// Arrays
118242
data.Array = Array;
@@ -134,14 +258,15 @@ export class Expression {
134258
data.Set = typeof Set !== 'undefined' ? Set : {};
135259
data.WeakSet = typeof WeakSet !== 'undefined' ? WeakSet : {};
136260

137-
// Errors
138-
data.Error = Error;
139-
data.TypeError = TypeError;
140-
data.SyntaxError = SyntaxError;
141-
data.EvalError = EvalError;
142-
data.RangeError = RangeError;
143-
data.ReferenceError = ReferenceError;
144-
data.URIError = URIError;
261+
// Errors - use safe wrappers to block prepareStackTrace, captureStackTrace,
262+
// and other dangerous properties that could enable sandbox escape
263+
data.Error = createSafeError();
264+
data.TypeError = createSafeErrorSubclass(TypeError);
265+
data.SyntaxError = createSafeErrorSubclass(SyntaxError);
266+
data.EvalError = createSafeErrorSubclass(EvalError);
267+
data.RangeError = createSafeErrorSubclass(RangeError);
268+
data.ReferenceError = createSafeErrorSubclass(ReferenceError);
269+
data.URIError = createSafeErrorSubclass(URIError);
145270

146271
// Internationalization
147272
data.Intl = typeof Intl !== 'undefined' ? Intl : {};

‎packages/workflow/test/expression-sandboxing.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,16 @@ describe('PrototypeSanitizer', () => {
8484
}).toThrowError(errorRegex);
8585
});
8686

87+
it.each([
88+
['getPrototypeOf', '{{ Object.getPrototypeOf }}'],
89+
['binding', '{{ process.binding }}'],
90+
['_load', '{{ module._load }}'],
91+
])('should not allow access to %s', (_, expression) => {
92+
expect(() => {
93+
tournament.execute(expression, { __sanitize: sanitizer, Object, process: {}, module: {} });
94+
}).toThrowError(errorRegex);
95+
});
96+
8797
describe('Dollar sign identifier handling', () => {
8898
it('should not allow bare $ identifier', () => {
8999
expect(() => {

0 commit comments

Comments
 (0)