From 74cba6339c7f6328d7d27766b64762e1ab3c2785 Mon Sep 17 00:00:00 2001 From: Vantz Stockwell Date: Sat, 14 Mar 2026 14:14:31 -0400 Subject: [PATCH] fix(security): SFTP session ownership + Guacamole instruction validation Co-Authored-By: Claude Sonnet 4.6 --- backend/src/rdp/guacamole.service.ts | 33 ++++++++++++++++++++++++++++ backend/src/rdp/rdp.gateway.ts | 15 ++++++++++++- backend/src/terminal/sftp.gateway.ts | 26 ++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/backend/src/rdp/guacamole.service.ts b/backend/src/rdp/guacamole.service.ts index 0fe1769..7cbcba8 100644 --- a/backend/src/rdp/guacamole.service.ts +++ b/backend/src/rdp/guacamole.service.ts @@ -201,6 +201,39 @@ export class GuacamoleService { return this.encode('connect', ...values); } + // Opcodes that clients are allowed to send to guacd. + // Server-originating opcodes (img, blob, end, ack, etc.) are intentionally excluded. + private static ALLOWED_CLIENT_OPCODES = new Set([ + 'key', 'mouse', 'size', 'clipboard', 'sync', 'disconnect', 'nop', + ]); + + /** + * Validate a raw Guacamole instruction received from a browser client. + * + * Checks: + * 1. The instruction matches the Guacamole wire format: .[,...] + * 2. The declared length matches the actual opcode length (prevents injection + * via length field manipulation). + * 3. The opcode is in the client-allowed allowlist. + * + * Returns true if the instruction is safe to forward to guacd. + */ + validateClientInstruction(instruction: string): boolean { + try { + // Must begin with a length-prefixed opcode: "." + const match = instruction.match(/^(\d+)\.([\w-]+)/); + if (!match) return false; + const declaredLen = parseInt(match[1], 10); + const opcode = match[2]; + // The declared length must equal the actual opcode length to prevent + // smuggling a different opcode via a mismatched length prefix. + if (declaredLen !== opcode.length) return false; + return GuacamoleService.ALLOWED_CLIENT_OPCODES.has(opcode); + } catch { + return false; + } + } + /** * Encode a Guacamole instruction. * Each part is length-prefixed: "." diff --git a/backend/src/rdp/rdp.gateway.ts b/backend/src/rdp/rdp.gateway.ts index b7a0002..ac27947 100644 --- a/backend/src/rdp/rdp.gateway.ts +++ b/backend/src/rdp/rdp.gateway.ts @@ -40,7 +40,20 @@ export class RdpGateway { if (msg.type === 'connect') { await this.handleConnect(client, msg); } else if (msg.type === 'guac') { - // Forward raw Guacamole instruction from browser to guacd TCP socket + // Validate before forwarding raw Guacamole instruction to guacd TCP socket + if (typeof msg.instruction !== 'string') { + this.send(client, { type: 'error', message: 'Invalid instruction' }); + return; + } + if (msg.instruction.length > 65536) { + this.send(client, { type: 'error', message: 'Instruction too large' }); + return; + } + if (!this.guacamole.validateClientInstruction(msg.instruction)) { + this.logger.warn(`[RDP] Blocked invalid Guacamole instruction: ${msg.instruction.substring(0, 80)}`); + this.send(client, { type: 'error', message: 'Invalid instruction' }); + return; + } const socket = this.clientSockets.get(client); if (socket && !socket.destroyed) { socket.write(msg.instruction); diff --git a/backend/src/terminal/sftp.gateway.ts b/backend/src/terminal/sftp.gateway.ts index 745b4a9..2126927 100644 --- a/backend/src/terminal/sftp.gateway.ts +++ b/backend/src/terminal/sftp.gateway.ts @@ -8,6 +8,9 @@ const MAX_EDIT_SIZE = 5 * 1024 * 1024; // 5MB export class SftpGateway { private readonly logger = new Logger(SftpGateway.name); + // Maps WebSocket client → set of session IDs they own + private clientSessions = new Map>(); + constructor( private ssh: SshConnectionService, private wsAuth: WsAuthGuard, @@ -21,6 +24,9 @@ export class SftpGateway { } this.logger.log(`SFTP WS connected: ${user.email}`); + // Initialize an empty session ownership set for this client + this.clientSessions.set(client, new Set()); + client.on('message', async (raw: Buffer) => { try { const msg = JSON.parse(raw.toString()); @@ -31,6 +37,10 @@ export class SftpGateway { this.send(client, { type: 'error', message: err.message }); } }); + + client.on('close', () => { + this.clientSessions.delete(client); + }); } private async handleMessage(client: any, msg: any) { @@ -39,6 +49,16 @@ export class SftpGateway { return this.send(client, { type: 'error', message: 'sessionId required' }); } + const ownedSessions = this.clientSessions.get(client); + + // If this client has already claimed at least one session, enforce ownership + // for all subsequent requests. A client claims a sessionId on first successful + // SFTP channel open (see registration below). + if (ownedSessions && ownedSessions.size > 0 && !ownedSessions.has(sessionId)) { + this.logger.warn(`[SFTP] Session access denied: client does not own sessionId=${sessionId}`); + return this.send(client, { type: 'error', message: 'Session access denied' }); + } + this.logger.log(`[SFTP] Getting SFTP channel for session ${sessionId}...`); let sftp: any; try { @@ -49,6 +69,12 @@ export class SftpGateway { return this.send(client, { type: 'error', message: `SFTP channel failed: ${err.message}` }); } + // Register this sessionId as owned by this client on first successful channel open + if (ownedSessions && !ownedSessions.has(sessionId)) { + ownedSessions.add(sessionId); + this.logger.log(`[SFTP] Registered sessionId=${sessionId} for client`); + } + // Listen for SFTP channel errors sftp.on('error', (err: any) => { this.logger.error(`[SFTP] Channel error event: ${err.message}`);