From bdf266cf55032d05641b20dce8804412dc93b6d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 15 Jan 2025 09:51:42 +0100 Subject: [PATCH] fix(core): Prevent prototype pollution in task runner (#12588) --- docker/images/n8n/n8n-task-runners.json | 1 + .../__tests__/js-task-runner.test.ts | 71 +++++++++++++++++++ .../src/js-task-runner/js-task-runner.ts | 19 ++++- .../__tests__/task-runner-process.test.ts | 3 +- .../src/task-runners/task-runner-process.ts | 10 ++- 5 files changed, 99 insertions(+), 5 deletions(-) diff --git a/docker/images/n8n/n8n-task-runners.json b/docker/images/n8n/n8n-task-runners.json index 1f38cf6d93..a26053e5e2 100644 --- a/docker/images/n8n/n8n-task-runners.json +++ b/docker/images/n8n/n8n-task-runners.json @@ -6,6 +6,7 @@ "command": "/usr/local/bin/node", "args": [ "--disallow-code-generation-from-strings", + "--disable-proto=delete", "/usr/local/lib/node_modules/n8n/node_modules/@n8n/task-runner/dist/start.js" ], "allowed-env": [ diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts index 6ef479239d..6828f23cb1 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts @@ -1342,4 +1342,75 @@ describe('JsTaskRunner', () => { task.cleanup(); }); }); + + describe('prototype pollution prevention', () => { + const checkPrototypeIntact = () => { + const obj: Record = {}; + expect(obj.maliciousKey).toBeUndefined(); + }; + + test('Object.setPrototypeOf should no-op for local object', async () => { + checkPrototypeIntact(); + + const outcome = await executeForAllItems({ + code: ` + const obj = {}; + Object.setPrototypeOf(obj, { maliciousKey: 'value' }); + return [{ json: { prototypeChanged: obj.maliciousKey !== undefined } }]; + `, + inputItems: [{ a: 1 }], + }); + + expect(outcome.result).toEqual([wrapIntoJson({ prototypeChanged: false })]); + checkPrototypeIntact(); + }); + + test('Reflect.setPrototypeOf should no-op for local object', async () => { + checkPrototypeIntact(); + + const outcome = await executeForAllItems({ + code: ` + const obj = {}; + Reflect.setPrototypeOf(obj, { maliciousKey: 'value' }); + return [{ json: { prototypeChanged: obj.maliciousKey !== undefined } }]; + `, + inputItems: [{ a: 1 }], + }); + + expect(outcome.result).toEqual([wrapIntoJson({ prototypeChanged: false })]); + checkPrototypeIntact(); + }); + + test('Object.setPrototypeOf should no-op for incoming object', async () => { + checkPrototypeIntact(); + + const outcome = await executeForAllItems({ + code: ` + const obj = $input.first(); + Object.setPrototypeOf(obj, { maliciousKey: 'value' }); + return [{ json: { prototypeChanged: obj.maliciousKey !== undefined } }]; + `, + inputItems: [{ a: 1 }], + }); + + expect(outcome.result).toEqual([wrapIntoJson({ prototypeChanged: false })]); + checkPrototypeIntact(); + }); + + test('Reflect.setPrototypeOf should no-op for incoming object', async () => { + checkPrototypeIntact(); + + const outcome = await executeForAllItems({ + code: ` + const obj = $input.first(); + Reflect.setPrototypeOf(obj, { maliciousKey: 'value' }); + return [{ json: { prototypeChanged: obj.maliciousKey !== undefined } }]; + `, + inputItems: [{ a: 1 }], + }); + + expect(outcome.result).toEqual([wrapIntoJson({ prototypeChanged: false })]); + checkPrototypeIntact(); + }); + }); }); diff --git a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts index 58750327d7..46ee1a6e7b 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts @@ -103,6 +103,20 @@ export class JsTaskRunner extends TaskRunner { allowedBuiltInModules: parseModuleAllowList(jsRunnerConfig.allowedBuiltInModules ?? ''), allowedExternalModules: parseModuleAllowList(jsRunnerConfig.allowedExternalModules ?? ''), }); + + this.preventPrototypePollution(); + } + + private preventPrototypePollution() { + if (process.env.NODE_ENV === 'test') return; // needed for Jest + + Object.getOwnPropertyNames(globalThis) + // @ts-expect-error globalThis does not have string in index signature + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-member-access + .map((name) => globalThis[name]) + .filter((value) => typeof value === 'function') + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-member-access + .forEach((fn) => Object.freeze(fn.prototype)); } async executeTask( @@ -203,8 +217,11 @@ export class JsTaskRunner extends TaskRunner { signal.addEventListener('abort', abortHandler, { once: true }); + const preventPrototypeManipulation = + 'Object.getPrototypeOf = () => ({}); Reflect.getPrototypeOf = () => ({}); Object.setPrototypeOf = () => false; Reflect.setPrototypeOf = () => false;'; + const taskResult = runInContext( - `globalThis.global = globalThis; module.exports = async function VmCodeWrapper() {${settings.code}\n}()`, + `globalThis.global = globalThis; ${preventPrototypeManipulation}; module.exports = async function VmCodeWrapper() {${settings.code}\n}()`, context, { timeout: this.taskTimeout * 1000 }, ) as Promise; diff --git a/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts b/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts index a3ca65fe21..7bf475bf7c 100644 --- a/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts +++ b/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts @@ -118,13 +118,14 @@ describe('TaskRunnerProcess', () => { expect(options.env).not.toHaveProperty('NODE_OPTIONS'); }); - it('should use --disallow-code-generation-from-strings flag', async () => { + it('should use --disallow-code-generation-from-strings and --disable-proto=delete flags', async () => { jest.spyOn(authService, 'createGrantToken').mockResolvedValue('grantToken'); await taskRunnerProcess.start(); expect(spawnMock.mock.calls[0].at(1)).toEqual([ '--disallow-code-generation-from-strings', + '--disable-proto=delete', expect.stringContaining('/packages/@n8n/task-runner/dist/start.js'), ]); }); diff --git a/packages/cli/src/task-runners/task-runner-process.ts b/packages/cli/src/task-runners/task-runner-process.ts index 7e83e56ca0..a657a35e5b 100644 --- a/packages/cli/src/task-runners/task-runner-process.ts +++ b/packages/cli/src/task-runners/task-runner-process.ts @@ -106,9 +106,13 @@ export class TaskRunnerProcess extends TypedEmitter { startNode(grantToken: string, taskBrokerUri: string) { const startScript = require.resolve('@n8n/task-runner/start'); - return spawn('node', ['--disallow-code-generation-from-strings', startScript], { - env: this.getProcessEnvVars(grantToken, taskBrokerUri), - }); + return spawn( + 'node', + ['--disallow-code-generation-from-strings', '--disable-proto=delete', startScript], + { + env: this.getProcessEnvVars(grantToken, taskBrokerUri), + }, + ); } @OnShutdown()