fix(core): Do not allow arbitrary path traversal in BinaryDataManager (#5523)

Author: netroy

packages/cli/src/Server.ts

@@ -33,6 +33,7 @@ import {
 	LoadNodeParameterOptions,
 	LoadNodeListSearch,
 	UserSettings,
+	FileNotFoundError,
 } from 'n8n-core';
 
 import type {
@@ -1119,21 +1120,26 @@ class Server extends AbstractServer {
 				// TODO UM: check if this needs permission check for UM
 				const identifier = req.params.path;
 				const binaryDataManager = BinaryDataManager.getInstance();
-				const binaryPath = binaryDataManager.getBinaryPath(identifier);
-				let { mode, fileName, mimeType } = req.query;
-				if (!fileName || !mimeType) {
-					try {
-						const metadata = await binaryDataManager.getBinaryMetadata(identifier);
-						fileName = metadata.fileName;
-						mimeType = metadata.mimeType;
-						res.setHeader('Content-Length', metadata.fileSize);
-					} catch {}
-				}
-				if (mimeType) res.setHeader('Content-Type', mimeType);
-				if (mode === 'download') {
-					res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
+				try {
+					const binaryPath = binaryDataManager.getBinaryPath(identifier);
+					let { mode, fileName, mimeType } = req.query;
+					if (!fileName || !mimeType) {
+						try {
+							const metadata = await binaryDataManager.getBinaryMetadata(identifier);
+							fileName = metadata.fileName;
+							mimeType = metadata.mimeType;
+							res.setHeader('Content-Length', metadata.fileSize);
+						} catch {}
+					}
+					if (mimeType) res.setHeader('Content-Type', mimeType);
+					if (mode === 'download') {
+						res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
+					}
+					res.sendFile(binaryPath);
+				} catch (error) {
+					if (error instanceof FileNotFoundError) res.writeHead(404).end();
+					else throw error;
 				}
-				res.sendFile(binaryPath);
 			},
 		);
 

packages/core/src/BinaryDataManager/FileSystem.ts

@@ -7,6 +7,7 @@ import type { BinaryMetadata } from 'n8n-workflow';
 import { jsonParse } from 'n8n-workflow';
 
 import type { IBinaryDataConfig, IBinaryDataManager } from '../Interfaces';
+import { FileNotFoundError } from '../errors';
 
 const PREFIX_METAFILE = 'binarymeta';
 const PREFIX_PERSISTED_METAFILE = 'persistedmeta';
@@ -85,17 +86,17 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
 	}
 
 	getBinaryPath(identifier: string): string {
-		return path.join(this.storagePath, identifier);
+		return this.resolveStoragePath(identifier);
 	}
 
 	getMetadataPath(identifier: string): string {
-		return path.join(this.storagePath, `${identifier}.metadata`);
+		return this.resolveStoragePath(`${identifier}.metadata`);
 	}
 
 	async markDataForDeletionByExecutionId(executionId: string): Promise<void> {
 		const tt = new Date(new Date().getTime() + this.binaryDataTTL * 60000);
 		return fs.writeFile(
-			path.join(this.getBinaryDataMetaPath(), `${PREFIX_METAFILE}_${executionId}_${tt.valueOf()}`),
+			this.resolveStoragePath('meta', `${PREFIX_METAFILE}_${executionId}_${tt.valueOf()}`),
 			'',
 		);
 	}
@@ -116,8 +117,8 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
 		const timeAtNextHour = currentTime + 3600000 - (currentTime % 3600000);
 		const timeoutTime = timeAtNextHour + this.persistedBinaryDataTTL * 60000;
 
-		const filePath = path.join(
-			this.getBinaryDataPersistMetaPath(),
+		const filePath = this.resolveStoragePath(
+			'persistMeta',
 			`${PREFIX_PERSISTED_METAFILE}_${executionId}_${timeoutTime}`,
 		);
 
@@ -170,21 +171,18 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
 		const newBinaryDataId = this.generateFileName(prefix);
 
 		return fs
-			.copyFile(
-				path.join(this.storagePath, binaryDataId),
-				path.join(this.storagePath, newBinaryDataId),
-			)
+			.copyFile(this.resolveStoragePath(binaryDataId), this.resolveStoragePath(newBinaryDataId))
 			.then(() => newBinaryDataId);
 	}
 
 	async deleteBinaryDataByExecutionId(executionId: string): Promise<void> {
 		const regex = new RegExp(`${executionId}_*`);
-		const filenames = await fs.readdir(path.join(this.storagePath));
+		const filenames = await fs.readdir(this.storagePath);
 
 		const proms = filenames.reduce(
 			(allProms, filename) => {
 				if (regex.test(filename)) {
-					allProms.push(fs.rm(path.join(this.storagePath, filename)));
+					allProms.push(fs.rm(this.resolveStoragePath(filename)));
 				}
 
 				return allProms;
@@ -253,4 +251,11 @@ export class BinaryDataFileSystem implements IBinaryDataManager {
 			throw new Error(`Error finding file: ${filePath}`);
 		}
 	}
+
+	private resolveStoragePath(...args: string[]) {
+		const returnPath = path.join(this.storagePath, ...args);
+		if (path.relative(this.storagePath, returnPath).startsWith('..'))
+			throw new FileNotFoundError('Invalid path detected');
+		return returnPath;
+	}
 }

packages/core/src/errors.ts

@@ -0,0 +1,5 @@
+export class FileNotFoundError extends Error {
+	constructor(readonly filePath: string) {
+		super(`File not found: ${filePath}`);
+	}
+}

packages/core/src/index.ts

@@ -15,6 +15,7 @@ export * from './LoadNodeListSearch';
 export * from './NodeExecuteFunctions';
 export * from './WorkflowExecute';
 export { eventEmitter, NodeExecuteFunctions, UserSettings };
+export * from './errors';
 
 declare module 'http' {
 	export interface IncomingMessage {