fix(core): Filter out prototype and constructor lookups in expressions (#10382)

packages/workflow/package.json

@@ -40,7 +40,7 @@
   },
   "dependencies": {
     "@n8n/permissions": "workspace:*",
-    "@n8n/tournament": "1.0.3",
+    "@n8n/tournament": "1.0.5",
     "@n8n_io/riot-tmpl": "4.0.0",
     "ast-types": "0.15.2",
     "axios": "catalog:",

packages/workflow/src/Expression.ts

@@ -26,6 +26,7 @@ import { extendSyntax } from './Extensions/ExpressionExtension';
 import { evaluateExpression, setErrorHandler } from './ExpressionEvaluatorProxy';
 import { getGlobalState } from './GlobalState';
 import { ApplicationError } from './errors/application.error';
+import { sanitizer, sanitizerName } from './ExpressionSandboxing';
 
 const IS_FRONTEND_IN_DEV_MODE =
 	typeof process === 'object' &&
@@ -306,6 +307,8 @@ export class Expression {
 		data.extend = extend;
 		data.extendOptional = extendOptional;
 
+		data[sanitizerName] = sanitizer;
+
 		Object.assign(data, extendedFunctions);
 
 		const constructorValidation = new RegExp(/\.\s*constructor/gm);

packages/workflow/src/ExpressionEvaluatorProxy.ts

@@ -3,6 +3,7 @@ import type { ReturnValue, TmplDifference } from '@n8n/tournament';
 import { Tournament } from '@n8n/tournament';
 import type { ExpressionEvaluatorType } from './Interfaces';
 import * as LoggerProxy from './LoggerProxy';
+import { PrototypeSanitizer } from './ExpressionSandboxing';
 
 type Evaluator = (expr: string, data: unknown) => tmpl.ReturnValue;
 type ErrorHandler = (error: Error) => void;
@@ -18,6 +19,7 @@ const differenceChecker = (diff: TmplDifference) => {
 		if (diff.same) {
 			return;
 		}
+		// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
 		if (diff.has?.function || diff.has?.templateString) {
 			return;
 		}
@@ -30,7 +32,10 @@ const differenceChecker = (diff: TmplDifference) => {
 		LoggerProxy.error('Expression evaluator difference checker failed');
 	}
 };
-const tournamentEvaluator = new Tournament(errorHandler, undefined);
+const tournamentEvaluator = new Tournament(errorHandler, undefined, undefined, {
+	before: [],
+	after: [PrototypeSanitizer],
+});
 let evaluator: Evaluator = tmpl.tmpl;
 let currentEvaluatorType: ExpressionEvaluatorType = 'tmpl';
 let diffExpressions = false;

packages/workflow/src/ExpressionSandboxing.ts

@@ -0,0 +1,58 @@
+import { type ASTAfterHook, astBuilders as b, astVisit } from '@n8n/tournament';
+import { ExpressionError } from './errors';
+
+const forbiddenMembers = ['__proto__', 'prototype', 'constructor', 'getPrototypeOf'];
+export const sanitizerName = '__sanitize';
+const sanitizerIdentifier = b.identifier(sanitizerName);
+
+export const PrototypeSanitizer: ASTAfterHook = (ast, dataNode) => {
+	astVisit(ast, {
+		visitMemberExpression(path) {
+			this.traverse(path);
+			const node = path.node;
+			if (!node.computed) {
+				// This is static, so we're safe to error here
+				if (node.property.type !== 'Identifier') {
+					// eslint-disable-next-line n8n-local-rules/no-plain-errors
+					throw new ExpressionError(
+						`Unknown property type ${node.property.type} while sanitising expression`,
+					);
+				}
+
+				if (forbiddenMembers.includes(node.property.name)) {
+					throw new ExpressionError(
+						`Cannot access "${node.property.name}" due to security concerns`,
+					);
+				}
+			} else if (node.property.type === 'StringLiteral' || node.property.type === 'Literal') {
+				// Check any static strings against our forbidden list
+				if (forbiddenMembers.includes(node.property.value as string)) {
+					throw new ExpressionError(
+						`Cannot access "${node.property.value as string}" due to security concerns`,
+					);
+				}
+			} else if (!node.property.type.endsWith('Literal')) {
+				// This isn't a literal value, so we need to wrap it
+				path.replace(
+					b.memberExpression(
+						// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-explicit-any
+						node.object as any,
+						// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+						b.callExpression(b.memberExpression(dataNode, sanitizerIdentifier), [
+							// eslint-disable-next-line @typescript-eslint/no-explicit-any
+							node.property as any,
+						]),
+						true,
+					),
+				);
+			}
+		},
+	});
+};
+
+export const sanitizer = (value: unknown): unknown => {
+	if (forbiddenMembers.includes(value as string)) {
+		throw new ExpressionError(`Cannot access "${value as string}" due to security concerns`);
+	}
+	return value;
+};

packages/workflow/test/Expression.test.ts

@@ -74,7 +74,9 @@ for (const evaluator of ['tmpl', 'tournament'] as const) {
 				expect(evaluate('={{Reflect}}')).toEqual({});
 				expect(evaluate('={{Proxy}}')).toEqual({});
 
-				expect(evaluate('={{constructor}}')).toEqual({});
+				expect(() => evaluate('={{constructor}}')).toThrowError(
+					new ExpressionError('Cannot access "constructor" due to security concerns'),
+				);
 
 				expect(evaluate('={{escape}}')).toEqual({});
 				expect(evaluate('={{unescape}}')).toEqual({});
@@ -166,7 +168,7 @@ for (const evaluator of ['tmpl', 'tournament'] as const) {
 				const testFn = jest.fn();
 				Object.assign(global, { testFn });
 				expect(() => evaluate("={{ Date['constructor']('testFn()')()}}")).toThrowError(
-					new ExpressionError('Arbitrary code execution detected'),
+					new ExpressionError('Cannot access "constructor" due to security concerns'),
 				);
 				expect(testFn).not.toHaveBeenCalled();
 			});

packages/workflow/test/ExpressionSandboxing.test.ts

@@ -0,0 +1,84 @@
+import { PrototypeSanitizer, sanitizer } from '@/ExpressionSandboxing';
+import { Tournament } from '@n8n/tournament';
+
+const tournament = new Tournament(
+	(e) => {
+		throw e;
+	},
+	undefined,
+	undefined,
+	{
+		before: [],
+		after: [PrototypeSanitizer],
+	},
+);
+
+const errorRegex = /^Cannot access ".*" due to security concerns$/;
+
+describe('PrototypeSanitizer', () => {
+	describe('Static analysis', () => {
+		it('should not allow access to __proto__', () => {
+			expect(() => {
+				tournament.execute('{{ ({}).__proto__.__proto__ }}', {});
+			}).toThrowError(errorRegex);
+
+			expect(() => {
+				tournament.execute('{{ ({})["__proto__"]["__proto__"] }}', {});
+			}).toThrowError(errorRegex);
+		});
+
+		it('should not allow access to prototype', () => {
+			expect(() => {
+				tournament.execute('{{ Number.prototype }}', { Number });
+			}).toThrowError(errorRegex);
+
+			expect(() => {
+				tournament.execute('{{ Number["prototype"] }}', { Number });
+			}).toThrowError(errorRegex);
+		});
+
+		it('should not allow access to constructor', () => {
+			expect(() => {
+				tournament.execute('{{ Number.constructor }}', {
+					__sanitize: sanitizer,
+					Number,
+				});
+			}).toThrowError(errorRegex);
+
+			expect(() => {
+				tournament.execute('{{ Number["constructor"] }}', {
+					__sanitize: sanitizer,
+					Number,
+				});
+			}).toThrowError(errorRegex);
+		});
+	});
+
+	describe('Runtime', () => {
+		it('should not allow access to __proto__', () => {
+			expect(() => {
+				tournament.execute('{{ ({})["__" + (() => "proto")() + "__"] }}', {
+					__sanitize: sanitizer,
+				});
+			}).toThrowError(errorRegex);
+		});
+
+		it('should not allow access to prototype', () => {
+			expect(() => {
+				tournament.execute('{{ Number["pro" + (() => "toty")() + "pe"] }}', {
+					__sanitize: sanitizer,
+					Number,
+				});
+			}).toThrowError(errorRegex);
+		});
+
+		it('should not allow access to constructor', () => {
+			expect(() => {
+				tournament.execute('{{ Number["cons" + (() => "truc")() + "tor"] }}', {
+					__sanitize: sanitizer,
+					Number,
+				});
+			}).toThrowError(errorRegex);
+		});
+	});
+});